Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] ima: fix calculating the boot_aggregate
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     James Bottomley <James.Bottomley@HansenPartnership.com>,
        linux-integrity@vger.kernel.org
Cc:     Jerry Snitselaar <jsnitsel@redhat.com>,
        linux-kernel@vger.kernel.org
Date:   Sun, 26 Jan 2020 18:53:30 -0500
In-Reply-To: <1580060759.4964.12.camel@HansenPartnership.com>
References: <1580044434-9132-1-git-send-email-zohar@linux.ibm.com>
         <1580060759.4964.12.camel@HansenPartnership.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Message-Id: <1580082810.5990.75.camel@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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