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

[Mimi Zohar <zohar@linux.ibm.com>]

On Thu, 2020-07-23 at 15:33 -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.
> 
> 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 --validate
> 
> 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 --validate
> 
> Pass both sha1 and sha2 PCRs to evmctl:
> $ sudo evmctl ima_measurement --pcrs sha1,pcr-sha1 --pcrs sha256,pcr-sha256 /sys/kernel/security/integrity/ima/binary_runtime_measurements --validate
> 
> NB The file parser could be improved to be more robust but I left the
> existing code unchanged from the code to read the TPM 1.2 sysfs file other
> than generalizing it to support loading other banks.

Agreed.  As a separate patch, please feel free to update the file
parser.

> 
> NB The error handling is IMHO wrong; if any error occurs during handling of
> the --pcrs option(s), evmctl will try to fall back to reading the PCRs in
> some other way.  I think these should be fatal errors but left it as it
> was for the TPM 1.2 sysfs file.
> 
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Thank you.

<snip>

> @@ -1829,16 +1783,100 @@ static void extend_tpm_banks(struct template_entry *entry, int num_banks,
>  #endif
>  }
>  
> -/* Read TPM 1.2 PCRs */
> +static int read_one_bank(struct tpm_bank_info *tpm_bank, FILE *fp)
> +{
> +	char *p, pcr_str[8], buf[80];
> +	int i = 0;
> +	int result = -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_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 all of the TPM PCRs */
>  static int read_tpm_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
>  {
> -	int i;
> +	struct stat s;
> +	FILE *fp = NULL;
> +	char *p;
> +	const char *alg, *path;
> +	int i = 0, j;
> +	int bank, result;
> +
> +	/* Use the provided TPM bank pcr file(s) */
> +	if (npcrfile) {
> +		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];
> +			}
>  
> -	if (tpm_pcr_read(tpm_banks, SHA_DIGEST_LENGTH)) {
> -		log_debug("Failed to read TPM 1.2 PCRs.\n");
> -		return -1;
> +			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("TPM 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;
>  	}

Previously on failure to read the --pcrs TPM 1.2 file, it attempted to
then read the TPM 1.2 sysfs file.  Changing it like this is fine.  I
agree with NB 2 - either read the PCR files or the TPM PCRs directly,
not both.  Perhaps make that a separate patch either before or after
this one.

I appreciate your limiting the changes to simplify review.  With
reading either the PCR files or the sysfs files, can we get rid of the
npcrfile indentation above?

if (!npcrfile)
     read_sysfs_pcrs()

Mimi


> +	/* Read the TPM 1.2 sysfs file */
> +	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;