| Message ID | 20200612143812.1609-1-maurizio.drocco@ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | extend IMA boot_aggregate with kernel measurements | expand |
> 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 Total: 0 warnings, 0 errors, 3 items checked All 3 test items SUCCESS. Link: http://patchwork.huawei.com/patch/52890/ --- Hulk Robot
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
> 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
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
> 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
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;
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(-)