| Message ID | 1580044434-9132-1-git-send-email-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: fix calculating the boot_aggregate | expand |
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. James --- 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);
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);
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index 7967a6904851..1253a2c187ef 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -663,6 +663,7 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest, struct crypto_shash *tfm) { struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} }; + int found = 0; int rc; u32 i; SHASH_DESC_ON_STACK(shash, tfm); @@ -673,6 +674,22 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest, if (rc != 0) return rc; + /* + * For backward's compatibility use TPM PCR SHA1 bank if allocated, + * otherwise use first enabled bank. + */ + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) { + if (ima_tpm_chip->allocated_banks[i].alg_id == TPM_ALG_SHA1) { + found = 1; + break; + } + } + if (!found) { + 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);
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. Reported-by: Jerry Snitselaar <jsnitsel@redhat.com> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- security/integrity/ima/ima_crypto.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)