[PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9

[PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9

[Maurizio Drocco]

From: Maurizio <maurizio.drocco@ibm.com>

If PCRs 8 - 9 are set (i.e. not all-zeros), cal_bootaggr should include
them into the digest.

Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
---
 src/evmctl.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 1d065ce..701d643 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1930,6 +1930,16 @@ static void calc_bootaggr(struct tpm_bank_info *bank)
 		}
 	}
 
+	for (i = 8; i < 10; i++) {
+		if (memcmp(bank->pcr[i], zero, bank->digest_size) != 0) {
+			err = EVP_DigestUpdate(pctx, bank->pcr[i], bank->digest_size);
+			if (!err) {
+				log_err("EVP_DigestUpdate() failed\n");
+				return;
+			}
+		}
+	}
+
 	err = EVP_DigestFinal(pctx, bank->digest, &mdlen);
 	if (!err) {
 		log_err("EVP_DigestFinal() failed\n");
@@ -1973,7 +1983,8 @@ static int append_bootaggr(char *bootaggr, struct tpm_bank_info *tpm_banks)
  * The IMA measurement list boot_aggregate is the link between the preboot
  * event log and the IMA measurement list.  Read and calculate all the
  * possible per TPM bank boot_aggregate digests based on the existing
- * PCRs 0 - 7 to validate against the IMA boot_aggregate record.
+ * PCRs 0 - 9 to validate against the IMA boot_aggregate record. If PCRs
+ * 8 - 9 are not set (i.e. all-zeros), only PCRs 0 - 7 are considered.
  */
 static int cmd_ima_bootaggr(struct command *cmd)
 {
-- 
2.17.1

Re: [PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9

[Mimi Zohar]

Hi Maurizio,

On Tue, 2020-06-16 at 08:02 -0400, Maurizio Drocco wrote:
> From: Maurizio <maurizio.drocco@ibm.com>
> 
> If PCRs 8 - 9 are set (i.e. not all-zeros), cal_bootaggr should include
> them into the digest.
> 
> Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>

Thank you, this patch seems to be working properly.  As a separate
patch, could you fix the tests/boot_aggregate.test comments "PCRs 0
-7" and other things, like displaying just PCRs 0 - 7?

thanks,

Mimi

[PATCH] ima_evm_utils: tests: boot_aggregate.test spans PCRs 0-9

[Maurizio Drocco]

display_pcrs() should include PCRS 8 - 9 as they are non-zeros on some
systems. boot_aggregate may span PCRs 0 - 9 so check()'s info message
should be fixed accordingly.

Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
---
 tests/boot_aggregate.test | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/boot_aggregate.test b/tests/boot_aggregate.test
index 39bd058..8a5581f 100755
--- a/tests/boot_aggregate.test
+++ b/tests/boot_aggregate.test
@@ -81,12 +81,12 @@ swtpm_init() {
 
 # In VERBOSE mode, display the calculated TPM PCRs for the different banks.
 display_pcrs() {
-	local PCRMAX=7
+	local PCRMAX=9
 	local banks=("sha1" "sha256")
 	local i;
 
 	for bank in "${banks[@]}"; do
-		echo "INFO: Displaying ${bank} TPM bank (PCRs 0 - 7)"
+		echo "INFO: Displaying ${bank} TPM bank (PCRs 0 - 9)"
 		for i in $(seq 0 $PCRMAX); do
 			rc=0
 			pcr=$("${TSSDIR}/tsspcrread" -halg "${bank}" -ha "${i}" -ns)
@@ -107,7 +107,7 @@ display_pcrs() {
 # Verify that the last "boot_aggregate" record in the IMA measurement
 # list matches.
 check() {
-	echo "INFO: Calculating the boot_aggregate (PCRs 0 - 7) for multiple banks"
+	echo "INFO: Calculating the boot_aggregate (PCRs 0 - 9) for multiple banks"
 	bootaggr=$(evmctl ima_boot_aggregate)
 	if [ $? -ne 0 ]; then
 		echo "${CYAN}SKIP: evmctl ima_boot_aggregate: $bootaggr${NORM}"
-- 
2.17.1

RE: [PATCH] extend IMA boot_aggregate with kernel measurements

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Tuesday, June 16, 2020 8:11 PM
> On Tue, 2020-06-16 at 17:29 +0000, Roberto Sassu wrote:
> > > From: James Bottomley [mailto:jejb@linux.ibm.com]
> > > Sent: Friday, June 12, 2020 7:14 PM
> > > On Fri, 2020-06-12 at 15:11 +0000, Roberto Sassu wrote:
> > > > with recent patches, boot_aggregate can be calculated from non-SHA1
> > > > PCR banks. I would replace with:
> > > >
> > > > Extend cumulative digest over ...
> > > >
> > > > Given that with this patch boot_aggregate is calculated differently,
> > > > shouldn't we call it boot_aggregate_v2 and enable it with a new
> > > > option?
> > >
> > > So here's the problem: if your current grub doesn't do any TPM
> > > extensions (as most don't), then the two boot aggregates are the same
> > > because PCRs 8 and 9 are zero and there's a test that doesn't add them
> > > to the aggregate if they are zero.  For these people its a nop so we
> > > shouldn't force them to choose a different version of the same thing.
> > >
> > > If, however, you're on a distribution where grub is automatically
> > > measuring the kernel and command line into PCRs 8 and 9 (I think
> Fedora
> > > 32 does this), your boot aggregate will change.  It strikes me in that
> > > case we can call this a bug fix, since the boot aggregate isn't
> > > properly binding to the previous measurements without PCRs 8 and 9.
> In
> > > this case, do we want to allow people to select an option which doesn't
> > > properly bind the IMA log to the boot measurements?  That sounds like
> a
> > > security hole to me.
> > >
> > > However, since it causes a user visible difference in the grub already
> > > measures case, do you have a current use case that would be affected?
> > > As in are lots of people already running a distro with the TPM grub
> > > updates and relying on the old boot aggregate?
> >
> > I don't know how many people would be affected. However, if an
> > attestation tool processes both measurement lists from unpatched
> kernels
> > and patched kernels, keeping the same name would be a problem as it
> > cannot be determined from the measurement list how boot_aggregate
> > was calculated.
> >
> > Anyway, I agree this should be fixed. At least, I suggest to add a Fixes tag,
> > to ensure that this patch is applied to all stable kernels.
> 
> The boot aggregate on existing systems would be sha1.  Does it make
> sense to limit this change to larger digests?  Anyone backporting
> support for larger digests would also need to backport this change as
> well.

Yes, it would be a safe choice.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

[PATCH] extend IMA boot_aggregate with kernel measurements

[Maurizio Drocco]

IMA is not considering TPM registers 8-9 when calculating the boot
aggregate. When registers 8-9 are used to store measurements of the
kernel and its command line (e.g., grub2 bootloader with tpm module
enabled), IMA should include them in the boot aggregate.

Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
---
 security/integrity/ima/ima.h        |  2 +-
 security/integrity/ima/ima_crypto.c | 20 ++++++++++++++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index df93ac258e01..9d94080bdad8 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -30,7 +30,7 @@
 
 enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN,
 		     IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII };
-enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
+enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
 
 /* digest size for IMA, fits SHA1 or MD5 */
 #define IMA_DIGEST_SIZE		SHA1_DIGEST_SIZE
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 220b14920c37..299b23dad8d9 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -809,7 +809,7 @@ static void ima_pcrread(u32 idx, struct tpm_digest *d)
 static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
 				       struct crypto_shash *tfm)
 {
-	struct tpm_digest d = { .alg_id = alg_id, .digest = {0} };
+	struct tpm_digest d = { .alg_id = alg_id, .digest = {0} }, d0 = d;
 	int rc;
 	u32 i;
 	SHASH_DESC_ON_STACK(shash, tfm);
@@ -823,13 +823,29 @@ static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
 	if (rc != 0)
 		return rc;
 
-	/* cumulative sha1 over tpm registers 0-7 */
+	/* cumulative digest over tpm registers 0-7 */
 	for (i = TPM_PCR0; i < TPM_PCR8; i++) {
 		ima_pcrread(i, &d);
 		/* now accumulate with current aggregate */
 		rc = crypto_shash_update(shash, d.digest,
 					 crypto_shash_digestsize(tfm));
 	}
+	/*
+	 * extend cumulative digest over tpm registers 8-9, which contain
+	 * measurement for the kernel command line (reg. 8) and image (reg. 9)
+	 * in a typical PCR allocation. Registers 8-9 are only included in
+	 * non-SHA1 boot_aggregate digests to avoid ambiguity.
+	 */
+	if (alg_id != TPM_ALG_SHA1) {
+		for (i = TPM_PCR8; i < TPM_PCR10; i++) {
+			ima_pcrread(i, &d);
+			/* if not zero, accumulate with current aggregate */
+			if (memcmp(d.digest, d0.digest,
+				crypto_shash_digestsize(tfm)) != 0)
+				rc = crypto_shash_update(shash, d.digest,
+						crypto_shash_digestsize(tfm));
+		}
+	}
 	if (!rc)
 		crypto_shash_final(shash, digest);
 	return rc;
-- 
2.17.1

[PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9

[Maurizio Drocco]

From: Maurizio <maurizio.drocco@ibm.com>

If PCRs 8 - 9 are set (i.e. not all-zeros), cal_bootaggr should include
them into the digest.

Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
---
 src/evmctl.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 1d065ce..554571e 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1930,6 +1930,18 @@ static void calc_bootaggr(struct tpm_bank_info *bank)
 		}
 	}
 
+	if (strcmp(bank->algo_name, "sha1") != 0) {
+		for (i = 8; i < 10; i++) {
+			if (memcmp(bank->pcr[i], zero, bank->digest_size) != 0) {
+				err = EVP_DigestUpdate(pctx, bank->pcr[i], bank->digest_size);
+				if (!err) {
+					log_err("EVP_DigestUpdate() failed\n");
+					return;
+				}
+			}
+		}
+	}
+
 	err = EVP_DigestFinal(pctx, bank->digest, &mdlen);
 	if (!err) {
 		log_err("EVP_DigestFinal() failed\n");
@@ -1973,7 +1985,9 @@ static int append_bootaggr(char *bootaggr, struct tpm_bank_info *tpm_banks)
  * The IMA measurement list boot_aggregate is the link between the preboot
  * event log and the IMA measurement list.  Read and calculate all the
  * possible per TPM bank boot_aggregate digests based on the existing
- * PCRs 0 - 7 to validate against the IMA boot_aggregate record.
+ * PCRs 0 - 9 to validate against the IMA boot_aggregate record. If PCRs
+ * 8 - 9 are not set (i.e. all-zeros) or the digest algorithm is SHA1, only
+ * PCRs 0 - 7 are considered.
  */
 static int cmd_ima_bootaggr(struct command *cmd)
 {
-- 
2.17.1

Re: [PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9

[Mimi Zohar]

On Thu, 2020-06-18 at 16:11 -0400, Maurizio Drocco wrote:
> From: Maurizio <maurizio.drocco@ibm.com>
> 
> If PCRs 8 - 9 are set (i.e. not all-zeros), cal_bootaggr should include
> them into the digest.
> 
> Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
> ---
>  src/evmctl.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 1d065ce..554571e 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -1930,6 +1930,18 @@ static void calc_bootaggr(struct tpm_bank_info *bank)
>  		}
>  	}
>  
> +	if (strcmp(bank->algo_name, "sha1") != 0) {
> +		for (i = 8; i < 10; i++) {
> +			if (memcmp(bank->pcr[i], zero, bank->digest_size) != 0) {
> +				err = EVP_DigestUpdate(pctx, bank->pcr[i], bank->digest_size);
> +				if (!err) {
> +					log_err("EVP_DigestUpdate() failed\n");
> +					return;
> +				}
> +			}
> +		}
> +	}

Roberto, now that we're only including the PCRs 8 & 9 in the non-sha1
"boot_aggregate", they can always be included.

Please reflect this change in the patch description and, here, in the
code.

thanks,

Mimi

Re: [PATCH v2] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9

[Stefan Berger]

On 6/23/20 2:13 PM, Bruno Meneguele wrote:
> On Tue, Jun 23, 2020 at 02:01:22PM -0400, Maurizio Drocco wrote:
>> From: Maurizio <maurizio.drocco@ibm.com>
>>
>> If PCRs 8 - 9 are set (i.e. not all-zeros), cal_bootaggr should include
>> them into the digest.


Wouldn't you have to check for not all-zeros in your code?


    Stefan


>>
>> Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
>> ---
>> Changelog:
>> v2:
>> - Always include PCRs 8 & 9 to non-sha1 hashes
>> v1:
>> - Include non-zero PCRs 8 & 9 to boot aggregates
>>
>>   src/evmctl.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/evmctl.c b/src/evmctl.c
>> index 1d065ce..46b7092 100644
>> --- a/src/evmctl.c
>> +++ b/src/evmctl.c
>> @@ -1930,6 +1930,16 @@ static void calc_bootaggr(struct tpm_bank_info *bank)
>>   		}
>>   	}
>>   
>> +	if (strcmp(bank->algo_name, "sha1") != 0) {
>> +		for (i = 8; i < 10; i++) {
>> +			err = EVP_DigestUpdate(pctx, bank->pcr[i], bank->digest_size);
>> +			if (!err) {
>> +				log_err("EVP_DigestUpdate() failed\n");
>> +				return;
>> +			}
>> +		}
>> +	}
>> +
>>   	err = EVP_DigestFinal(pctx, bank->digest, &mdlen);
>>   	if (!err) {
>>   		log_err("EVP_DigestFinal() failed\n");
>> @@ -1972,8 +1982,9 @@ static int append_bootaggr(char *bootaggr, struct tpm_bank_info *tpm_banks)
>>   /*
>>    * The IMA measurement list boot_aggregate is the link between the preboot
>>    * event log and the IMA measurement list.  Read and calculate all the
>> - * possible per TPM bank boot_aggregate digests based on the existing
>> - * PCRs 0 - 7 to validate against the IMA boot_aggregate record.
>> + * possible per TPM bank boot_aggregate digests based on the existing PCRs
>> + * 0 - 9 to validate against the IMA boot_aggregate record. If the digest
>> + * algorithm is SHA1, only PCRs 0 - 7 are considered to avoid ambiguity.
>>    */
>>   static int cmd_ima_bootaggr(struct command *cmd)
>>   {
>> -- 
>> 2.17.1
>>
> Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
>

[PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9

[Maurizio Drocco]

From: Maurizio <maurizio.drocco@ibm.com>

cal_bootaggr should include PCRs 8-9 in non-SHA1 digests.

Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
---
Changelog:
v3:
- Fixed patch description
v2:
- Always include PCRs 8 & 9 to non-sha1 hashes
v1:
- Include non-zero PCRs 8 & 9 to boot aggregates

 src/evmctl.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 1d065ce..46b7092 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1930,6 +1930,16 @@ static void calc_bootaggr(struct tpm_bank_info *bank)
 		}
 	}
 
+	if (strcmp(bank->algo_name, "sha1") != 0) {
+		for (i = 8; i < 10; i++) {
+			err = EVP_DigestUpdate(pctx, bank->pcr[i], bank->digest_size);
+			if (!err) {
+				log_err("EVP_DigestUpdate() failed\n");
+				return;
+			}
+		}
+	}
+
 	err = EVP_DigestFinal(pctx, bank->digest, &mdlen);
 	if (!err) {
 		log_err("EVP_DigestFinal() failed\n");
@@ -1972,8 +1982,9 @@ static int append_bootaggr(char *bootaggr, struct tpm_bank_info *tpm_banks)
 /*
  * The IMA measurement list boot_aggregate is the link between the preboot
  * event log and the IMA measurement list.  Read and calculate all the
- * possible per TPM bank boot_aggregate digests based on the existing
- * PCRs 0 - 7 to validate against the IMA boot_aggregate record.
+ * possible per TPM bank boot_aggregate digests based on the existing PCRs
+ * 0 - 9 to validate against the IMA boot_aggregate record. If the digest
+ * algorithm is SHA1, only PCRs 0 - 7 are considered to avoid ambiguity.
  */
 static int cmd_ima_bootaggr(struct command *cmd)
 {
-- 
2.17.1