[PATCH] ima: fix calculating the boot_aggregate

[PATCH] ima: fix calculating the boot_aggregate

[Mimi Zohar]

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(+)

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

Re: [PATCH] ima: fix calculating the boot_aggregate

[James Bottomley]

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

Re: [PATCH] ima: fix calculating the boot_aggregate

[Mimi Zohar]

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