[PATCH v3 ima-evm-utils] extend ima_measurement --pcrs option to support per-bank pcr files

[PATCH v3 ima-evm-utils] extend ima_measurement --pcrs option to support per-bank pcr files

[Stephen Smalley]

Extend the ima_measurement --pcrs option to support per-bank pcr files.
The extended syntax is "--pcrs algorithm,pathname".  If no algorithm
is specified, it defaults to sha1 as before.  Multiple --pcrs options
are now supported, one per bank of PCRs. The file format remains
unchanged.  If --pcrs is specified, only try to read PCRs from the
specified file(s); do not fall back to trying to read from sysfs
or the TPM itself in this case since the user requested use of
the files.

Create per-bank pcr files, depends on "tpm: add sysfs exports for all
banks of PCR registers" kernel patch:
$ cat tpm2pcrread.sh
#!/bin/sh
for alg in sha1 sha256
do
  rm -f pcr-$alg
  pcr=0;
  while [ $pcr -lt 24 ];
  do
    printf "PCR-%02d: " $pcr >> pcr-$alg;
    cat /sys/class/tpm/tpm0/pcr-$alg/$pcr >> pcr-$alg;
    pcr=$[$pcr+1];
  done
done
$ sh ./tpm2pcrread.sh

Pass only the sha1 PCRs to evmctl defaulting to sha1:
$ sudo evmctl ima_measurement --pcrs pcr-sha1 /sys/kernel/security/integrity/ima/binary_runtime_measurements

Pass only the sha1 PCRs to evmctl with explicit selection of sha1:
$ sudo evmctl ima_measurement --pcrs sha1,pcr-sha1 /sys/kernel/security/integrity/ima/binary_runtime_measurements

Pass both sha1 and sha256 PCRs to evmctl:
$ sudo evmctl ima_measurement --pcrs sha1,pcr-sha1 --pcrs sha256,pcr-sha256 /sys/kernel/security/integrity/ima/binary_runtime_measurements

Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
v3 splits reading the pcr files and the sysfs files into two separate functions
each called from read_tpm_banks() with different error handling. Thus, if
--pcrs is specified, then evmctl will only try to read from the specified
file(s) and any errors will cause it to fail rather than trying to read
directly from the TPM.  If --pcrs is not specified, then evmctl will try
to read from the sysfs files and then fall back to reading directly
from the TPM if that fails. v3 also reduces the limit on the number of
PCRs read from the files to NUM_PCRS since otherwise it will overflow
the pcrs[] array.  Further improvements to the file parser are deferred
to future patches.

 src/evmctl.c | 168 +++++++++++++++++++++++++++++++++------------------
 src/imaevm.h |   2 +-
 2 files changed, 110 insertions(+), 60 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index faddc3c..4793b3e 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -160,7 +160,10 @@ struct tpm_bank_info {
 	uint8_t pcr[NUM_PCRS][MAX_DIGEST_SIZE];
 };
 
-static char *pcrfile;
+
+#define MAX_PCRFILE 16
+static char *pcrfile[MAX_PCRFILE];
+static unsigned npcrfile;
 
 static int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
 {
@@ -1373,55 +1376,6 @@ static int cmd_ima_clear(struct command *cmd)
 	return do_cmd(cmd, ima_clear);
 }
 
-static char *pcrs = "/sys/class/tpm/tpm0/device/pcrs";  /* Kernels >= 4.0 */
-static char *misc_pcrs = "/sys/class/misc/tpm0/device/pcrs";
-
-/* Read all of the TPM 1.2 PCRs */
-static int tpm_pcr_read(struct tpm_bank_info *tpm_banks, int len)
-{
-	struct stat s;
-	FILE *fp = NULL;
-	char *p, pcr_str[8], buf[70]; /* length of the TPM string */
-	int result = -1;
-	int i = 0;
-
-	/* Use the provided TPM 1.2 pcrs file */
-	if (pcrfile) {
-		if (stat(pcrfile, &s) == -1) {
-			errno = 0;
-			return 1;
-		}
-
-		if (!S_ISREG(s.st_mode)) {
-			log_info("TPM 1.2 PCR file: not a regular file or link to regular file\n");
-			return 1;
-		}
-
-		fp = fopen(pcrfile, "r");
-	}
-
-	if (!fp)
-		fp = fopen(pcrs, "r");
-
-	if (!fp)
-		fp = fopen(misc_pcrs, "r");
-
-	if (!fp)
-		return -1;
-
-	for (;;) {
-		p = fgets(buf, sizeof(buf), fp);
-		if (!p || i > 99)
-			break;
-		sprintf(pcr_str, "PCR-%2.2d", i);
-		if (!strncmp(p, pcr_str, 6))
-			hex2bin(tpm_banks[0].pcr[i++], p + 7, len);
-		result = 0;
-	}
-	fclose(fp);
-	return result;
-}
-
 #define TCG_EVENT_NAME_LEN_MAX	255
 
 struct template_entry {
@@ -1829,20 +1783,108 @@ static void extend_tpm_banks(struct template_entry *entry, int num_banks,
 #endif
 }
 
-/* Read TPM 1.2 PCRs */
-static int read_tpm_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
+static int read_one_bank(struct tpm_bank_info *tpm_bank, FILE *fp)
 {
-	int i;
+	char *p, pcr_str[8], buf[80];
+	int i = 0;
+	int result = -1;
+	for (;;) {
+		p = fgets(buf, sizeof(buf), fp);
+		if (!p || i >= NUM_PCRS)
+			break;
+		sprintf(pcr_str, "PCR-%2.2d", i);
+		if (!strncmp(p, pcr_str, 6))
+			hex2bin(tpm_bank->pcr[i++], p + 7, tpm_bank->digest_size);
+		result = 0;
+	}
+	return result;
+}
+
+static char *pcrs = "/sys/class/tpm/tpm0/device/pcrs";  /* Kernels >= 4.0 */
+static char *misc_pcrs = "/sys/class/misc/tpm0/device/pcrs";
+
+/* Read one of the TPM 1.2 sysfs files if present */
+static int read_sysfs_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
+{
+	FILE *fp;
+	int i, result;
 
-	if (tpm_pcr_read(tpm_banks, SHA_DIGEST_LENGTH)) {
-		log_debug("Failed to read TPM 1.2 PCRs.\n");
+	fp = fopen(pcrs, "r");
+	if (!fp)
+		fp = fopen(misc_pcrs, "r");
+	if (!fp)
 		return -1;
-	}
 
+	result = read_one_bank(&tpm_banks[0], fp);
+	fclose(fp);
+	if (result < 0)
+		return result;
 	tpm_banks[0].supported = 1;
 	for (i = 1; i < num_banks; i++)
 		tpm_banks[i].supported = 0;
 	return 0;
+
+}
+
+/* Read PCRs from per-bank file(s) specified via --pcrs */
+static int read_file_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
+{
+	struct stat s;
+	FILE *fp;
+	char *p;
+	const char *alg, *path;
+	int i, j, bank, result;
+
+	for (i = 0; i < num_banks; i++)
+		tpm_banks[i].supported = 0;
+
+	for (i = 0; i < npcrfile; i++) {
+		p = strchr(pcrfile[i], ',');
+		if (p) {
+			*p = 0;
+			alg = pcrfile[i];
+			path = ++p;
+		} else {
+			alg = "sha1";
+			path = pcrfile[i];
+		}
+
+		bank = -1;
+		for (j = 0; j < num_banks; j++) {
+			if (!strcmp(tpm_banks[j].algo_name, alg)) {
+				bank = j;
+				break;
+			}
+		}
+		if (bank < 0) {
+			log_err("Unknown algorithm '%s'\n", alg);
+			return -1;
+		}
+
+		if (stat(path, &s) == -1) {
+			log_err("Could not stat '%s'\n", path);
+			return -1;
+		}
+
+		if (!S_ISREG(s.st_mode)) {
+			log_err("PCR file: not a regular file or link to regular file\n");
+			return -1;
+		}
+
+		fp = fopen(path, "r");
+		if (!fp) {
+			log_err("Could not open '%s'\n", path);
+			return -1;
+		}
+
+		result = read_one_bank(&tpm_banks[bank], fp);
+		fclose(fp);
+		if (result < 0)
+			return result;
+		tpm_banks[bank].supported = 1;
+	}
+
+	return 0;
 }
 
 /*
@@ -1857,8 +1899,12 @@ static int read_tpm_banks(int num_banks, struct tpm_bank_info *bank)
 	int i, j;
 	int err;
 
-	/* First try reading PCRs from exported TPM 1.2 sysfs file */
-	if (read_tpm_pcrs(num_banks, bank) == 0)
+	/* If --pcrs was specified, read only from the specified file(s) */
+	if (npcrfile)
+		return read_file_pcrs(num_banks, bank);
+
+	/* Else try reading PCRs from the sysfs file if present */
+	if (read_sysfs_pcrs(num_banks, bank) == 0)
 		return 0;
 
 	/* Any userspace applications available for reading TPM 2.0 PCRs? */
@@ -2667,7 +2713,11 @@ int main(int argc, char *argv[])
 			verify = 1;
 			break;
 		case 143:
-			pcrfile = optarg;
+			if (npcrfile >= MAX_PCRFILE) {
+				log_err("too many --pcrfile options\n");
+				exit(1);
+			}
+			pcrfile[npcrfile++] = optarg;
 			break;
 		case '?':
 			exit(1);
diff --git a/src/imaevm.h b/src/imaevm.h
index 3f1db97..4503919 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -203,7 +203,7 @@ struct RSA_ASN1_template {
 	size_t size;
 };
 
-#define	NUM_PCRS 20
+#define	NUM_PCRS 24
 #define DEFAULT_PCR 10
 
 extern struct libimaevm_params imaevm_params;
-- 
2.25.1

Re: [PATCH v3 ima-evm-utils] extend ima_measurement --pcrs option to support per-bank pcr files

[Mimi Zohar]

On Fri, 2020-07-24 at 10:52 -0400, Stephen Smalley wrote:
> Extend the ima_measurement --pcrs option to support per-bank pcr files.
> The extended syntax is "--pcrs algorithm,pathname".  If no algorithm
> is specified, it defaults to sha1 as before.  Multiple --pcrs options
> are now supported, one per bank of PCRs. The file format remains
> unchanged.  If --pcrs is specified, only try to read PCRs from the
> specified file(s); do not fall back to trying to read from sysfs
> or the TPM itself in this case since the user requested use of
> the files.
> 
> Create per-bank pcr files, depends on "tpm: add sysfs exports for all
> banks of PCR registers" kernel patch:
> $ cat tpm2pcrread.sh
> #!/bin/sh
> for alg in sha1 sha256
> do
>   rm -f pcr-$alg
>   pcr=0;
>   while [ $pcr -lt 24 ];
>   do
>     printf "PCR-%02d: " $pcr >> pcr-$alg;
>     cat /sys/class/tpm/tpm0/pcr-$alg/$pcr >> pcr-$alg;
>     pcr=$[$pcr+1];
>   done
> done
> $ sh ./tpm2pcrread.sh
> 
> Pass only the sha1 PCRs to evmctl defaulting to sha1:
> $ sudo evmctl ima_measurement --pcrs pcr-sha1 /sys/kernel/security/integrity/ima/binary_runtime_measurements
> 
> Pass only the sha1 PCRs to evmctl with explicit selection of sha1:
> $ sudo evmctl ima_measurement --pcrs sha1,pcr-sha1 /sys/kernel/security/integrity/ima/binary_runtime_measurements
> 
> Pass both sha1 and sha256 PCRs to evmctl:
> $ sudo evmctl ima_measurement --pcrs sha1,pcr-sha1 --pcrs sha256,pcr-sha256 /sys/kernel/security/integrity/ima/binary_runtime_measurements
> 
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

I reviewed the patch and everything looks good, other than a few
comments below.  Weirdly the order in which the TPM bank files
containing the PCRs are supplied on the command line is affecting
being able to verify the measurement list - sha1, sha256 versus
sha256, sha1.  Perhaps during the time it takes to read the different
banks, the PCRs have changed?  Not quite sure what is happening.
 
> ---
> v3 splits reading the pcr files and the sysfs files into two separate functions
> each called from read_tpm_banks() with different error handling. Thus, if
> --pcrs is specified, then evmctl will only try to read from the specified
> file(s) and any errors will cause it to fail rather than trying to read
> directly from the TPM.  If --pcrs is not specified, then evmctl will try
> to read from the sysfs files and then fall back to reading directly
> from the TPM if that fails. v3 also reduces the limit on the number of
> PCRs read from the files to NUM_PCRS since otherwise it will overflow
> the pcrs[] array.  Further improvements to the file parser are deferred
> to future patches.
> 
> @@ -1829,20 +1783,108 @@ static void extend_tpm_banks(struct template_entry *entry, int num_banks,
>  #endif
>  }
>  
> -/* Read TPM 1.2 PCRs */
> -static int read_tpm_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
> +static int read_one_bank(struct tpm_bank_info *tpm_bank, FILE *fp)
>  {
> -	int i;
> +	char *p, pcr_str[8], buf[80];

As log as we're changing this from 70 to 80, might as well define it
as MAX_DITEST_SIZE * 2 + X.  I'm pretty sure support for additional,
larger digests will be added.  This can be deferred to the subsequent
patch. 

> +	int i = 0;
> +	int result = -1;
> +	for (;;) {
> +		p = fgets(buf, sizeof(buf), fp);
> +		if (!p || i >= NUM_PCRS)
> +			break;
> +		sprintf(pcr_str, "PCR-%2.2d", i);
> +		if (!strncmp(p, pcr_str, 6))
> +			hex2bin(tpm_bank->pcr[i++], p + 7, tpm_bank->digest_size);
> +		result = 0;
> +	}
> +	return result;
> +}
> +
> +static char *pcrs = "/sys/class/tpm/tpm0/device/pcrs";  /* Kernels >= 4.0 */
> +static char *misc_pcrs = "/sys/class/misc/tpm0/device/pcrs";
> +
> +/* Read one of the TPM 1.2 sysfs files if present */
> +static int read_sysfs_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
> +{
> +	FILE *fp;
> +	int i, result;
>  
> -	if (tpm_pcr_read(tpm_banks, SHA_DIGEST_LENGTH)) {
> -		log_debug("Failed to read TPM 1.2 PCRs.\n");
> +	fp = fopen(pcrs, "r");
> +	if (!fp)
> +		fp = fopen(misc_pcrs, "r");
> +	if (!fp)
>  		return -1;
> -	}
>  
> +	result = read_one_bank(&tpm_banks[0], fp);
> +	fclose(fp);
> +	if (result < 0)
> +		return result;
>  	tpm_banks[0].supported = 1;
>  	for (i = 1; i < num_banks; i++)
>  		tpm_banks[i].supported = 0;
>  	return 0;
> +
> +}
> +
> +/* Read PCRs from per-bank file(s) specified via --pcrs */
> +static int read_file_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
> +{
> +	struct stat s;
> +	FILE *fp;
> +	char *p;
> +	const char *alg, *path;
> +	int i, j, bank, result;
> +
> +	for (i = 0; i < num_banks; i++)
> +		tpm_banks[i].supported = 0;
> +
> +	for (i = 0; i < npcrfile; i++) {

Being able to provide more PCR files than banks is a bit strange.  I
guess the TPM could export additional banks, than what is currently
supported by ima-evm-utils, and programmatically supply them to ima-
evm-utils.

Mimi

> +		p = strchr(pcrfile[i], ',');
> +		if (p) {
> +			*p = 0;
> +			alg = pcrfile[i];
> +			path = ++p;
> +		} else {
> 

Re: [PATCH v3 ima-evm-utils] extend ima_measurement --pcrs option to support per-bank pcr files

[Mimi Zohar]

On Fri, 2020-07-24 at 15:12 -0400, Mimi Zohar wrote:
> On Fri, 2020-07-24 at 10:52 -0400, Stephen Smalley wrote:
> > Extend the ima_measurement --pcrs option to support per-bank pcr files.
> > The extended syntax is "--pcrs algorithm,pathname".  If no algorithm
> > is specified, it defaults to sha1 as before.  Multiple --pcrs options
> > are now supported, one per bank of PCRs. The file format remains
> > unchanged.  If --pcrs is specified, only try to read PCRs from the
> > specified file(s); do not fall back to trying to read from sysfs
> > or the TPM itself in this case since the user requested use of
> > the files.
> > 
> > Create per-bank pcr files, depends on "tpm: add sysfs exports for all
> > banks of PCR registers" kernel patch:
> > $ cat tpm2pcrread.sh
> > #!/bin/sh
> > for alg in sha1 sha256
> > do
> >   rm -f pcr-$alg
> >   pcr=0;
> >   while [ $pcr -lt 24 ];
> >   do
> >     printf "PCR-%02d: " $pcr >> pcr-$alg;
> >     cat /sys/class/tpm/tpm0/pcr-$alg/$pcr >> pcr-$alg;
> >     pcr=$[$pcr+1];
> >   done
> > done
> > $ sh ./tpm2pcrread.sh
> > 
> > Pass only the sha1 PCRs to evmctl defaulting to sha1:
> > $ sudo evmctl ima_measurement --pcrs pcr-sha1 /sys/kernel/security/integrity/ima/binary_runtime_measurements
> > 
> > Pass only the sha1 PCRs to evmctl with explicit selection of sha1:
> > $ sudo evmctl ima_measurement --pcrs sha1,pcr-sha1 /sys/kernel/security/integrity/ima/binary_runtime_measurements
> > 
> > Pass both sha1 and sha256 PCRs to evmctl:
> > $ sudo evmctl ima_measurement --pcrs sha1,pcr-sha1 --pcrs sha256,pcr-sha256 /sys/kernel/security/integrity/ima/binary_runtime_measurements
> > 
> > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> 
> I reviewed the patch and everything looks good, other than a few
> comments below.  Weirdly the order in which the TPM bank files
> containing the PCRs are supplied on the command line is affecting
> being able to verify the measurement list - sha1, sha256 versus
> sha256, sha1.  Perhaps during the time it takes to read the different
> banks, the PCRs have changed?  Not quite sure what is happening.

Never mind, problem solved.

Mimi