* [PATCH] extend IMA boot_aggregate with kernel measurements @ 2020-06-11 19:54 Maurizio Drocco 2020-06-12 0:29 ` Mimi Zohar 0 siblings, 1 reply; 22+ messages in thread From: Maurizio Drocco @ 2020-06-11 19:54 UTC (permalink / raw) To: linux-integrity Cc: jejb, Maurizio Drocco, Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] extend IMA boot_aggregate with kernel measurements 2020-06-11 19:54 [PATCH] extend IMA boot_aggregate with kernel measurements Maurizio Drocco @ 2020-06-12 0:29 ` Mimi Zohar 2020-06-12 14:38 ` Maurizio Drocco 0 siblings, 1 reply; 22+ messages in thread From: Mimi Zohar @ 2020-06-12 0:29 UTC (permalink / raw) To: Maurizio Drocco, linux-integrity Cc: jejb, Dmitry Kasatkin, James Morris, Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list 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; ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] extend IMA boot_aggregate with kernel measurements 2020-06-12 0:29 ` Mimi Zohar @ 2020-06-12 14:38 ` Maurizio Drocco 2020-06-12 15:11 ` Roberto Sassu 0 siblings, 1 reply; 22+ messages in thread From: Maurizio Drocco @ 2020-06-12 14:38 UTC (permalink / raw) To: zohar Cc: dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, maurizio.drocco, serge 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* RE: [PATCH] extend IMA boot_aggregate with kernel measurements 2020-06-12 14:38 ` Maurizio Drocco @ 2020-06-12 15:11 ` Roberto Sassu 2020-06-12 17:14 ` James Bottomley 0 siblings, 1 reply; 22+ messages in thread From: Roberto Sassu @ 2020-06-12 15:11 UTC (permalink / raw) To: Maurizio Drocco, zohar Cc: dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, serge, Silviu Vlasceanu [-- 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] extend IMA boot_aggregate with kernel measurements 2020-06-12 15:11 ` Roberto Sassu @ 2020-06-12 17:14 ` James Bottomley 2020-06-16 17:29 ` Roberto Sassu 0 siblings, 1 reply; 22+ messages in thread From: James Bottomley @ 2020-06-12 17:14 UTC (permalink / raw) To: Roberto Sassu, Maurizio Drocco, zohar Cc: dmitry.kasatkin, jmorris, linux-integrity, linux-kernel, linux-security-module, serge, Silviu Vlasceanu 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH] extend IMA boot_aggregate with kernel measurements 2020-06-12 17:14 ` James Bottomley @ 2020-06-16 17:29 ` Roberto Sassu 2020-06-16 18:11 ` Mimi Zohar 0 siblings, 1 reply; 22+ messages in thread From: Roberto Sassu @ 2020-06-16 17:29 UTC (permalink / raw) To: jejb, Maurizio Drocco, zohar Cc: dmitry.kasatkin, jmorris, linux-integrity, linux-kernel, linux-security-module, serge, Silviu Vlasceanu > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] extend IMA boot_aggregate with kernel measurements 2020-06-16 17:29 ` Roberto Sassu @ 2020-06-16 18:11 ` Mimi Zohar 2020-06-18 12:38 ` Roberto Sassu 0 siblings, 1 reply; 22+ messages in thread From: Mimi Zohar @ 2020-06-16 18:11 UTC (permalink / raw) To: Roberto Sassu, jejb, Maurizio Drocco Cc: dmitry.kasatkin, jmorris, linux-integrity, linux-kernel, linux-security-module, serge, Silviu Vlasceanu 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH] extend IMA boot_aggregate with kernel measurements 2020-06-16 18:11 ` Mimi Zohar @ 2020-06-18 12:38 ` Roberto Sassu 2020-06-18 20:11 ` Maurizio Drocco 0 siblings, 1 reply; 22+ messages in thread From: Roberto Sassu @ 2020-06-18 12:38 UTC (permalink / raw) To: Mimi Zohar, jejb, Maurizio Drocco Cc: dmitry.kasatkin, jmorris, linux-integrity, linux-kernel, linux-security-module, serge, Silviu Vlasceanu > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] extend IMA boot_aggregate with kernel measurements 2020-06-18 12:38 ` Roberto Sassu @ 2020-06-18 20:11 ` Maurizio Drocco 2020-06-18 20:11 ` [PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 Maurizio Drocco 0 siblings, 1 reply; 22+ messages in thread From: Maurizio Drocco @ 2020-06-18 20:11 UTC (permalink / raw) To: roberto.sassu Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, maurizio.drocco, serge, zohar, mdrocco 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 2020-06-18 20:11 ` Maurizio Drocco @ 2020-06-18 20:11 ` Maurizio Drocco 2020-06-22 20:14 ` Mimi Zohar 0 siblings, 1 reply; 22+ messages in thread From: Maurizio Drocco @ 2020-06-18 20:11 UTC (permalink / raw) To: roberto.sassu Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, maurizio.drocco, serge, zohar, mdrocco 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 2020-06-18 20:11 ` [PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 Maurizio Drocco @ 2020-06-22 20:14 ` Mimi Zohar 2020-06-22 4:50 ` [PATCH] ima: extend boot_aggregate with kernel measurements Maurizio Drocco 2020-06-23 18:01 ` [PATCH v2] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 Maurizio Drocco 0 siblings, 2 replies; 22+ messages in thread From: Mimi Zohar @ 2020-06-22 20:14 UTC (permalink / raw) To: Maurizio Drocco, roberto.sassu Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, serge, mdrocco 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] ima: extend boot_aggregate with kernel measurements 2020-06-22 20:14 ` Mimi Zohar @ 2020-06-22 4:50 ` Maurizio Drocco 2020-06-23 14:03 ` Mimi Zohar 2020-06-23 18:01 ` [PATCH v2] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 Maurizio Drocco 1 sibling, 1 reply; 22+ messages in thread From: Maurizio Drocco @ 2020-06-22 4:50 UTC (permalink / raw) To: zohar Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, maurizio.drocco, mdrocco, roberto.sassu, serge 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ima: extend boot_aggregate with kernel measurements 2020-06-22 4:50 ` [PATCH] ima: extend boot_aggregate with kernel measurements Maurizio Drocco @ 2020-06-23 14:03 ` Mimi Zohar 2020-06-23 15:57 ` [PATCH v4] " Maurizio Drocco 0 siblings, 1 reply; 22+ messages in thread From: Mimi Zohar @ 2020-06-23 14:03 UTC (permalink / raw) To: Maurizio Drocco Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, mdrocco, roberto.sassu, serge 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; ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4] ima: extend boot_aggregate with kernel measurements 2020-06-23 14:03 ` Mimi Zohar @ 2020-06-23 15:57 ` Maurizio Drocco 2020-06-23 18:53 ` Bruno Meneguele 0 siblings, 1 reply; 22+ messages in thread From: Maurizio Drocco @ 2020-06-23 15:57 UTC (permalink / raw) To: zohar Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, maurizio.drocco, mdrocco, roberto.sassu, serge 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4] ima: extend boot_aggregate with kernel measurements 2020-06-23 15:57 ` [PATCH v4] " Maurizio Drocco @ 2020-06-23 18:53 ` Bruno Meneguele 0 siblings, 0 replies; 22+ messages in thread From: Bruno Meneguele @ 2020-06-23 18:53 UTC (permalink / raw) To: Maurizio Drocco Cc: zohar, Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, mdrocco, roberto.sassu, serge [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 2020-06-22 20:14 ` Mimi Zohar 2020-06-22 4:50 ` [PATCH] ima: extend boot_aggregate with kernel measurements Maurizio Drocco @ 2020-06-23 18:01 ` Maurizio Drocco 2020-06-23 18:13 ` Bruno Meneguele 1 sibling, 1 reply; 22+ messages in thread From: Maurizio Drocco @ 2020-06-23 18:01 UTC (permalink / raw) To: zohar Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, maurizio.drocco, mdrocco, roberto.sassu, serge 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 2020-06-23 18:01 ` [PATCH v2] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 Maurizio Drocco @ 2020-06-23 18:13 ` Bruno Meneguele 2020-06-24 21:17 ` Stefan Berger 0 siblings, 1 reply; 22+ messages in thread From: Bruno Meneguele @ 2020-06-23 18:13 UTC (permalink / raw) To: Maurizio Drocco Cc: zohar, Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, mdrocco, roberto.sassu, serge [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 2020-06-23 18:13 ` Bruno Meneguele @ 2020-06-24 21:17 ` Stefan Berger 2020-06-24 21:33 ` [PATCH] " Maurizio Drocco 2020-06-24 21:33 ` [PATCH v2] " Bruno Meneguele 0 siblings, 2 replies; 22+ messages in thread From: Stefan Berger @ 2020-06-24 21:17 UTC (permalink / raw) To: Bruno Meneguele, Maurizio Drocco Cc: zohar, Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, mdrocco, roberto.sassu, serge 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> > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 2020-06-24 21:17 ` Stefan Berger @ 2020-06-24 21:33 ` Maurizio Drocco 2020-06-24 21:33 ` [PATCH v2] " Bruno Meneguele 1 sibling, 0 replies; 22+ messages in thread From: Maurizio Drocco @ 2020-06-24 21:33 UTC (permalink / raw) To: stefanb Cc: Silviu.Vlasceanu, bmeneg, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, maurizio.drocco, mdrocco, roberto.sassu, serge, zohar 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 2020-06-24 21:17 ` Stefan Berger 2020-06-24 21:33 ` [PATCH] " Maurizio Drocco @ 2020-06-24 21:33 ` Bruno Meneguele 2020-06-24 21:35 ` [PATCH v3] " Maurizio Drocco 1 sibling, 1 reply; 22+ messages in thread From: Bruno Meneguele @ 2020-06-24 21:33 UTC (permalink / raw) To: Stefan Berger Cc: Maurizio Drocco, zohar, Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, mdrocco, roberto.sassu, serge [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 2020-06-24 21:33 ` [PATCH v2] " Bruno Meneguele @ 2020-06-24 21:35 ` Maurizio Drocco 2020-06-24 21:50 ` Bruno Meneguele 0 siblings, 1 reply; 22+ messages in thread From: Maurizio Drocco @ 2020-06-24 21:35 UTC (permalink / raw) To: bmeneg Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, maurizio.drocco, mdrocco, roberto.sassu, serge, stefanb, zohar 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 2020-06-24 21:35 ` [PATCH v3] " Maurizio Drocco @ 2020-06-24 21:50 ` Bruno Meneguele 0 siblings, 0 replies; 22+ messages in thread From: Bruno Meneguele @ 2020-06-24 21:50 UTC (permalink / raw) To: Maurizio Drocco Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity, linux-kernel, linux-security-module, mdrocco, roberto.sassu, serge, stefanb, zohar [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-06-24 21:50 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-11 19:54 [PATCH] extend IMA boot_aggregate with kernel measurements Maurizio Drocco 2020-06-12 0:29 ` Mimi Zohar 2020-06-12 14:38 ` Maurizio Drocco 2020-06-12 15:11 ` Roberto Sassu 2020-06-12 17:14 ` James Bottomley 2020-06-16 17:29 ` Roberto Sassu 2020-06-16 18:11 ` Mimi Zohar 2020-06-18 12:38 ` Roberto Sassu 2020-06-18 20:11 ` Maurizio Drocco 2020-06-18 20:11 ` [PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 Maurizio Drocco 2020-06-22 20:14 ` Mimi Zohar 2020-06-22 4:50 ` [PATCH] ima: extend boot_aggregate with kernel measurements Maurizio Drocco 2020-06-23 14:03 ` Mimi Zohar 2020-06-23 15:57 ` [PATCH v4] " Maurizio Drocco 2020-06-23 18:53 ` Bruno Meneguele 2020-06-23 18:01 ` [PATCH v2] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9 Maurizio Drocco 2020-06-23 18:13 ` Bruno Meneguele 2020-06-24 21:17 ` Stefan Berger 2020-06-24 21:33 ` [PATCH] " Maurizio Drocco 2020-06-24 21:33 ` [PATCH v2] " Bruno Meneguele 2020-06-24 21:35 ` [PATCH v3] " Maurizio Drocco 2020-06-24 21:50 ` Bruno Meneguele
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).