[PATCH] extend IMA boot_aggregate with kernel measurements

[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 | 11 ++++++++++-
 2 files changed, 11 insertions(+), 2 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..6f0137bdaf61 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);
@@ -830,6 +830,15 @@ static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
 		rc = crypto_shash_update(shash, d.digest,
 					 crypto_shash_digestsize(tfm));
 	}
+	/* extend cumulative sha1 over tpm registers 8-9 */
+	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

Re: [PATCH] extend IMA boot_aggregate with kernel measurements

[Mimi Zohar]

Hi Maurizo,

On Thu, 2020-06-11 at 15:54 -0400, Maurizio Drocco wrote:
> 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>

Looks good.  Just a minor comment below.  Could you be a bit more
specific as to what is being measured into which PCR.  Perhaps include
a reference to some doc or spec.

In order to test, ima-evm-utils needs to be updated as well.  Could
you post the corresponding evmctl change?  Please post the patch
against the ima-evm-utils next-testing branch.

> ---
>  security/integrity/ima/ima.h        |  2 +-
>  security/integrity/ima/ima_crypto.c | 11 ++++++++++-
>  2 files changed, 11 insertions(+), 2 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..6f0137bdaf61 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);
> @@ -830,6 +830,15 @@ static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
>  		rc = crypto_shash_update(shash, d.digest,
>  					 crypto_shash_digestsize(tfm));
>  	}
> +	/* extend cumulative sha1 over tpm registers 8-9 */
> +	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))

The formatting here is a bit off.

thanks,

Mimi

> +			rc = crypto_shash_update(shash, d.digest,
> +					crypto_shash_digestsize(tfm));
> +	}
>  	if (!rc)
>  		crypto_shash_final(shash, digest);
>  	return rc;

[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 | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 2 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..64f5e3151e18 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);
@@ -830,6 +830,19 @@ static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
 		rc = crypto_shash_update(shash, d.digest,
 					 crypto_shash_digestsize(tfm));
 	}
+	/*
+	 * extend cumulative sha1 over tpm registers 8-9, which contain
+	 * measurement for the kernel command line (reg. 8) and image (reg. 9)
+	 * in a typical PCR allocation.
+	 */
+	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

RE: [PATCH] extend IMA boot_aggregate with kernel measurements

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 3082 bytes --]

> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> owner@vger.kernel.org] On Behalf Of Maurizio Drocco
> Sent: Friday, June 12, 2020 4:38 PM
> 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 | 15 ++++++++++++++-
>  2 files changed, 15 insertions(+), 2 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..64f5e3151e18 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);
> @@ -830,6 +830,19 @@ static int ima_calc_boot_aggregate_tfm(char
> *digest, u16 alg_id,
>  		rc = crypto_shash_update(shash, d.digest,
>  					 crypto_shash_digestsize(tfm));
>  	}
> +	/*
> +	 * extend cumulative sha1 over tpm registers 8-9, which contain

Hi Maurizio

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?

Thanks

Roberto

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

> +	 * measurement for the kernel command line (reg. 8) and image
> (reg. 9)
> +	 * in a typical PCR allocation.
> +	 */
> +	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


[-- Attachment #2: Type: message/rfc822, Size: 2632 bytes --]

From: pmail_patchwork <patchwork@huawei.com>
To: Roberto Sassu <roberto.sassu@huawei.com>
Cc: pmail_hulkcommits <hulkcommits@huawei.com>, pmail_hulkcommits <hulkcommits@huawei.com>
Subject: re: [PATCH 9/9] ima: Don't remove security.ima if file must not be appraised
Date: Thu, 28 May 2020 08:03:43 +0000
Message-ID: <20200528080343.54677.49365@hulknmm>

Total: 0 warnings, 0 errors, 3 items checked

All 3 test items SUCCESS.

Link: http://patchwork.huawei.com/patch/52890/

---
Hulk Robot

Re: [PATCH] extend IMA boot_aggregate with kernel measurements

[James Bottomley]

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?

James

RE: [PATCH] extend IMA boot_aggregate with kernel measurements

[Roberto Sassu]

> 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.

Roberto

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

Re: [PATCH] extend IMA boot_aggregate with kernel measurements

[Mimi Zohar]

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.

Mimi

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

[PATCH] ima: extend 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. Registers
8-9 are only included in non-SHA1 boot_aggregate digests to avoid
ambiguity.

Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
---
 security/integrity/ima/ima.h        |  2 +-
 security/integrity/ima/ima_crypto.c | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 2 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..d02917d85033 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -823,13 +823,26 @@ 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);
+			rc = crypto_shash_update(shash, d.digest,
+						crypto_shash_digestsize(tfm));
+		}
+	}
 	if (!rc)
 		crypto_shash_final(shash, digest);
 	return rc;
-- 
2.17.1

Re: [PATCH] ima: extend boot_aggregate with kernel measurements

[Mimi Zohar]

Hi Maurizio,

When re-posting patches, please include the version number (e.g.
[PATCH v4] ima: ... ).

On Mon, 2020-06-22 at 00:50 -0400, Maurizio Drocco wrote:
> IMA is not considering TPM registers 8-9 when calculating the boot
> aggregate.

This line is unnecessary with the following change.

> 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.

The "When" clause makes this sound like PCRs 8 & 9 are not always
included.  I would split this into two sentences.

>  Registers
> 8-9 are only included in non-SHA1 boot_aggregate digests to avoid
> ambiguity.
> 
> Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
> ---

Missing "Changelog:".

Changelog:
v2: 
- Limit including PCRs 8 & 9 to non-sha1 hashes
v1:
- Include non zero PCRs 8 & 9 in the boot_aggregate

>  security/integrity/ima/ima.h        |  2 +-
>  security/integrity/ima/ima_crypto.c | 15 ++++++++++++++-
>  2 files changed, 15 insertions(+), 2 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..d02917d85033 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -823,13 +823,26 @@ 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 */

Please uppercase "tpm" here and below.

>  	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.
> +	 */

Comments that are full sentences should start with an uppercase letter
and end with a period (e.g. Extend).

thanks,

Mimi

> +	if (alg_id != TPM_ALG_SHA1) {
> +		for (i = TPM_PCR8; i < TPM_PCR10; i++) {
> +			ima_pcrread(i, &d);
> +			rc = crypto_shash_update(shash, d.digest,
> +						crypto_shash_digestsize(tfm));
> +		}
> +	}
>  	if (!rc)
>  		crypto_shash_final(shash, digest);
>  	return rc;

[PATCH v4] ima: extend boot_aggregate with kernel measurements

[Maurizio Drocco]

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. Registers 8-9 should be
only included in non-SHA1 digests to avoid ambiguity.

Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
---
Changelog:
v4:
- Reworded comments: PCRs 8 & 9 are always included in non-sha1 digests
v3:
- Limit including PCRs 8 & 9 to non-sha1 hashes
v2:
- Minor comment improvements
v1:
- Include non zero PCRs 8 & 9 in the boot_aggregate

 security/integrity/ima/ima.h        |  2 +-
 security/integrity/ima/ima_crypto.c | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 2 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..011c3c76af86 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -823,13 +823,26 @@ 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);
+			rc = crypto_shash_update(shash, d.digest,
+						crypto_shash_digestsize(tfm));
+		}
+	}
 	if (!rc)
 		crypto_shash_final(shash, digest);
 	return rc;
-- 
2.17.1

Re: [PATCH v4] ima: extend boot_aggregate with kernel measurements

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 2902 bytes --]

On Tue, Jun 23, 2020 at 11:57:32AM -0400, Maurizio Drocco wrote:
> 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. Registers 8-9 should be
> only included in non-SHA1 digests to avoid ambiguity.
> 
> Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
> ---
> Changelog:
> v4:
> - Reworded comments: PCRs 8 & 9 are always included in non-sha1 digests
> v3:
> - Limit including PCRs 8 & 9 to non-sha1 hashes
> v2:
> - Minor comment improvements
> v1:
> - Include non zero PCRs 8 & 9 in the boot_aggregate
> 
>  security/integrity/ima/ima.h        |  2 +-
>  security/integrity/ima/ima_crypto.c | 15 ++++++++++++++-
>  2 files changed, 15 insertions(+), 2 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..011c3c76af86 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -823,13 +823,26 @@ 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);
> +			rc = crypto_shash_update(shash, d.digest,
> +						crypto_shash_digestsize(tfm));
> +		}
> +	}
>  	if (!rc)
>  		crypto_shash_final(shash, digest);
>  	return rc;
> -- 
> 2.17.1
> 

Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>

I've tested this patch with both TPM 1.2 and TPM 2.0 + ima-evm-utils
support patch. Everything seems fine.

Thanks.

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2] 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>
---
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

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

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 1952 bytes --]

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.
> 
> 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>

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

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

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 2663 bytes --]

On Wed, Jun 24, 2020 at 05:17:52PM -0400, Stefan Berger wrote:
> 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?
> 

boot_aggregate in kernel, after the following patch be applied:

https://lkml.org/lkml/2020/6/23/833

is calculated regardless PCR 8 & 9 being zero or not.
Thus evmctl is only reflecting the same behavior.

I think it would be worth changing the commit log here.

> 
>    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>
> > 
> 

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3] 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

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

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 1959 bytes --]

On Wed, Jun 24, 2020 at 05:35:58PM -0400, Maurizio Drocco wrote:
> 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
> 

Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>

Thanks.

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[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