Re: [PATCH] ima: fix calculating the boot_aggregate

Mimi Zohar
2020-01-26T23:53:41Z

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);