Hi James, On Sun, 2020-01-26 at 09:45 -0800, James Bottomley wrote: > On Sun, 2020-01-26 at 08:13 -0500, Mimi Zohar wrote: > > Calculating the boot_aggregate assumes that the TPM SHA1 bank is > > enabled. Before trying to read the TPM SHA1 bank, ensure it is > > enabled. If it isn't enabled, calculate the boot_aggregate using the > > first bank enabled. > > Isn't it about time we shifted IMA away from SHA1 as a NIST deprecated > algorithm especially as in this case if someone can manufacture a sha1 > hash collision, they can fake the TCB? I think we should always try > use SHA256 if we have a TPM2, then fall back to whatever bank0 is if > SHA256 can't be found (that will cope with DELLs that violate the TPM2 > spec by disabling the sha256 bank if the bios setting is sha1). This > should also cope with other ODMs who violate the spec in other ways, > like not updating the sha1 bank but still leaving it allocated. > > Mechanically, also, you don't need the found variable, you can see if i > reaches the max value. Agreed, in general we should be moving away from SHA1, but this change only addresses calculating the boot_aggregate hash, not the bigger issue of calculating multiple file hashes and extending the TPM banks with the appropriate file hash values. The boot_aggregate is the hash of PCRs 0 - 7, which links the pre-boot event log with the IMA measurement list. I would think manufacturing a SHA1 hash collision in this specific use case scenario would be more difficult. Assuming changing the boot_aggregate hash algorithm doesn't break userspace, instead of hard coding the algorithm, we probably should use the Kconfig IMA_DEFAULT_HASH algorithm. Mimi > > --- > > diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c > index 73044fc6a952..f5f7a3aec826 100644 > --- a/security/integrity/ima/ima_crypto.c > +++ b/security/integrity/ima/ima_crypto.c > @@ -665,12 +665,29 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest, > u32 i; > SHASH_DESC_ON_STACK(shash, tfm); > > + if (ima_tpm_chip->flags & TPM_CHIP_FLAG_TPM2) > + /* TPM2 default should be sha256 */ > + d.alg_id = TPM_ALG_SHA256; > + > shash->tfm = tfm; > > rc = crypto_shash_init(shash); > if (rc != 0) > return rc; > > + /* > + * Check the TPM default bank is allocated otherwise use the first one > + */ > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) > + if (ima_tpm_chip->allocated_banks[i].alg_id == d.alg_id) > + break; > + > + if (i == ima_tpm_chip->nr_allocated_banks) { > + d.alg_id = ima_tpm_chip->allocated_banks[0].alg_id; > + pr_info("Calculating the boot-aggregregate (TPM algorithm: %d)", > + d.alg_id); > + } > + > /* cumulative sha1 over tpm registers 0-7 */ > for (i = TPM_PCR0; i < TPM_PCR8; i++) { > ima_pcrread(i, &d);