[PATCH] extend IMA boot_aggregate with kernel measurements
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 --- 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_SIZESHA1_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] extend IMA boot_aggregate with kernel measurements
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 --- 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_SIZESHA1_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
[PATCH] extend IMA boot_aggregate with kernel measurements
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 --- 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_SIZESHA1_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
> From: linux-integrity-ow...@vger.kernel.org [mailto:linux-integrity- > ow...@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 > --- > 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 --- Begin Message --- Total: 0 warnings, 0 errors, 3 items checked All 3 test items SUCCESS. Link: http://patchwork.huawei.com/patch/52890/ --- Hulk Robot --- End Message ---
Re: [PATCH] extend IMA boot_aggregate with kernel measurements
On Fri, 2020-06-12 at 15:11 +, 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
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 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;
Re: [PATCH] extend IMA boot_aggregate with kernel measurements
Hi Maurizio, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on integrity/next-integrity] [also build test WARNING on next-20200611] [cannot apply to v5.7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Maurizio-Drocco/extend-IMA-boot_aggregate-with-kernel-measurements/20200612-091504 base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity config: x86_64-allyesconfig (attached as .config) compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 3b43f006294971b8049d4807110032169780e5b8) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>, old ones prefixed by <<): >> security/integrity/ima/ima_crypto.c:838:35: warning: size argument in >> 'memcmp' call is a comparison [-Wmemsize-comparison] crypto_shash_digestsize(tfm) != 0)) ~^~~~ security/integrity/ima/ima_crypto.c:837:7: note: did you mean to compare the result of 'memcmp' instead? if (memcmp(d.digest, d0.digest, ^ security/integrity/ima/ima_crypto.c:838:6: note: explicitly cast the argument to size_t to silence this warning crypto_shash_digestsize(tfm) != 0)) ^ (size_t)() 1 warning generated. vim +/memcmp +838 security/integrity/ima/ima_crypto.c 797 798 /* 799 * The boot_aggregate is a cumulative hash over TPM registers 0 - 7. With 800 * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but with 801 * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks, 802 * allowing firmware to configure and enable different banks. 803 * 804 * Knowing which TPM bank is read to calculate the boot_aggregate digest 805 * needs to be conveyed to a verifier. For this reason, use the same 806 * hash algorithm for reading the TPM PCRs as for calculating the boot 807 * aggregate digest as stored in the measurement list. 808 */ 809 static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id, 810 struct crypto_shash *tfm) 811 { 812 struct tpm_digest d = { .alg_id = alg_id, .digest = {0} }, d0 = d; 813 int rc; 814 u32 i; 815 SHASH_DESC_ON_STACK(shash, tfm); 816 817 shash->tfm = tfm; 818 819 pr_devel("calculating the boot-aggregate based on TPM bank: %04x\n", 820 d.alg_id); 821 822 rc = crypto_shash_init(shash); 823 if (rc != 0) 824 return rc; 825 826 /* cumulative sha1 over tpm registers 0-7 */ 827 for (i = TPM_PCR0; i < TPM_PCR8; i++) { 828 ima_pcrread(i, &d); 829 /* now accumulate with current aggregate */ 830 rc = crypto_shash_update(shash, d.digest, 831 crypto_shash_digestsize(tfm)); 832 } 833 /* extend cumulative sha1 over tpm registers 8-9 */ 834 for (i = TPM_PCR8; i < TPM_PCR10; i++) { 835 ima_pcrread(i, &d); 836 /* if not zero, accumulate with current aggregate */ 837 if (memcmp(d.digest, d0.digest, > 838 crypto_shash_digestsize(tfm) != > 0)) 839 rc = crypto_shash_update(shash, d.digest, 840 crypto_shash_digestsize(tfm)); 841 } 842 if (!rc) 843 crypto_shash_final(shash, digest); 844 return rc; 845 } 846 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
RE: [PATCH] extend IMA boot_aggregate with kernel measurements
> From: Mimi Zohar [mailto:zo...@linux.ibm.com] > Sent: Tuesday, June 16, 2020 8:11 PM > On Tue, 2020-06-16 at 17:29 +, Roberto Sassu wrote: > > > From: James Bottomley [mailto:j...@linux.ibm.com] > > > Sent: Friday, June 12, 2020 7:14 PM > > > On Fri, 2020-06-12 at 15:11 +, 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
RE: [PATCH] extend IMA boot_aggregate with kernel measurements
> From: James Bottomley [mailto:j...@linux.ibm.com] > Sent: Friday, June 12, 2020 7:14 PM > On Fri, 2020-06-12 at 15:11 +, 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
On Tue, 2020-06-16 at 17:29 +, Roberto Sassu wrote: > > From: James Bottomley [mailto:j...@linux.ibm.com] > > Sent: Friday, June 12, 2020 7:14 PM > > On Fri, 2020-06-12 at 15:11 +, 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