Message-ID: <34ebd15aae07402d19279ef4286197112c4afc01.camel@linux.ibm.com>
Date: Mon, 17 Mar 2025 21:46:43 -0400
From: Mimi Zohar <zohar@...ux.ibm.com>
To: Nicolai Stange <nstange@...e.de>,
        Roberto Sassu
 <roberto.sassu@...wei.com>,
        Dmitry Kasatkin <dmitry.kasatkin@...il.com>
Cc: Eric Snowberg <eric.snowberg@...cle.com>, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v1 6/7] ima: invalidate unsupported PCR banks once
 at first use

On Thu, 2025-03-13 at 18:33 +0100, Nicolai Stange wrote:
> Normally IMA would extend a template hash of each bank's associated
> algorithm into a PCR. However, if a bank's hash algorithm is unavailable
> to the kernel at IMA init time, it would fallback to extending padded
> SHA1 hashes instead.
> 
> That is, if e.g. SHA-256 was missing at IMA init, it would extend padded
> SHA1 template hashes into a PCR's SHA-256 bank.
> 
> The ima_measurement command (marked as experimental) from ima-evm-utils
> would accordingly try both variants when attempting to verify a measurement
> list against PCRs. keylime OTOH doesn't seem to -- it expects the template
> hash type to match the PCR bank algorithm. I would argue that for the
> latter case, the fallback scheme could potentially cause hard to debug
> verification failures.
> 
> There's another problem with the fallback scheme: right now, SHA-1
> availability is a hard requirement for IMA, and it would be good for a
> number of reasons to get rid of that. However, if SHA-1 is not available to
> the kernel, it can hardly provide padded SHA-1 template hashes for PCR
> banks with unsupported algos.
> 
> There are several more or less reasonable alternatives possible, among
> them are:
> a.) Instead of padded SHA-1, use padded/truncated ima_hash template
>     hashes.
> b.) Record every event as a violation, i.e. extend unsupported banks
>     with 0xffs.
> c.) Don't extend unsupported banks at all.
> d.) Invalidate unsupported banks only once (e.g. with 0xffs) at first
>     use.
> 
> a.) would make verification from tools like ima_measurement nearly
>     impossible, as it would have to guess or somehow determine ima_hash.
> b.) would still put an significant and unnecessary burden on tools like
>     ima_measurement, because it would then have to exercise three
>     possible variants on the measurement list:
>     - the template hash matches the bank algorithm,
>     - the template hash is padded SHA-1,
>     - the template hash is all-ones.
> c.) is a security risk, because the bank would validate an empty
>     measurement list.
> 
> AFAICS, d.) is the best option to proceed, as it allows for determining
> from the PCR bank value in O(1) whether the bank had been maintained by
> IMA or not and also, it would not validate any measurement list (except
> one with a single violation entry at the head).

Hi Nicolai,

What a pleasure reviewing your patch set.  Nicely organized.  Well written patch
descriptions.

Currently with the SHA1 hash algorithm, whether it is being extended into the
TPM or not, the measurement list is complete.  Relying on the ima_hash in the
current kernel and the subsequent kexec'ed kernel should be fine, assuming if
they're different hash algorithms both TPM banks are enabled.  Otherwise, the
measurement lists will be incomplete.

This patch set introduces a new definition of integrity violation. Previously it
was limited to open-writers and ToMToU integrity violations.  Now it could also
mean no kernel hash algorithm available.  Unfortunately some attestation
services simply ignore integrity violations.

Mimi

> 
> So implement d.). As it potentially breaks existing userspace, i.e.
> the current implementation of ima_measurement, put it behind a Kconfig
> option, "IMA_COMPAT_FALLBACK_TPM_EXTEND". If set to "y", the original
> behavior of extending with padded SHA-1 is retained. Otherwise the new
> scheme to invalidate unsupported PCR banks once upon their first extension
> from IMA is implemented instead. As ima_measurement is marked as
> experimental and I find it unlikely that other existing tools depend on
> the padded SHA-1 fallback scheme, make the IMA_COMPAT_FALLBACK_TPM_EXTEND
> Kconfig option default to "n".
> 
> For IMA_COMPAT_FALLBACK_TPM_EXTEND=n,
> - make ima_calc_field_array_hash() to fill the digests corresponding to
>   banks with unsupported hash algorithms with 0xffs,
> - make ima_pcr_extend() to extend these into the unsupported PCR banks only
>   upon the PCR's first usage, skip them on subsequent updates and
> - let ima_init_ima_crypto() help it with that by populating the new
>   ima_unsupported_tpm_banks_mask with one bit set for each bank with
>   an unavailable hash algorithm at init.
> 
> [1] https://github.com/linux-integrity/ima-evm-utils
> 
> Signed-off-by: Nicolai Stange <nstange@...e.de>
> ---
>  security/integrity/ima/Kconfig      | 14 ++++++++++++++
>  security/integrity/ima/ima.h        |  1 +
>  security/integrity/ima/ima_crypto.c | 27 +++++++++++++++++++++++++--
>  security/integrity/ima/ima_queue.c  | 20 +++++++++++++++++++-
>  4 files changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 475c32615006..d6ba392c0b37 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -122,6 +122,20 @@ config IMA_DEFAULT_HASH
>  	default "wp512" if IMA_DEFAULT_HASH_WP512
>  	default "sm3" if IMA_DEFAULT_HASH_SM3
>  
> +config IMA_COMPAT_FALLBACK_TPM_EXTEND
> +	bool
> +	default n
> +	help
> +	  In case a TPM PCR hash algorithm is not supported by the kernel,
> +	  retain the old behaviour to extend the bank with padded SHA1 template
> +	  digests.
> +
> +	  If Y, IMA will be unavailable when SHA1 is missing from the kernel.
> +	  If N, existing tools may fail to verify IMA measurement lists against
> +	  TPM PCR banks corresponding to hashes not supported by the kernel.
> +
> +	  If unsure, say N.
> +
>  config IMA_WRITE_POLICY
>  	bool "Enable multiple writes to the IMA policy"
>  	default n
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index f99b1f81b35c..58e9a81b3f96 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -62,6 +62,7 @@ extern int ima_sha1_idx __ro_after_init;
>  extern int ima_hash_algo_idx __ro_after_init;
>  extern int ima_extra_slots __ro_after_init;
>  extern struct ima_algo_desc *ima_algo_array __ro_after_init;
> +extern unsigned long ima_unsupported_tpm_banks_mask __ro_after_init;
>  
>  extern unsigned long ima_extended_pcrs_mask;
>  
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 6f5696d999d0..118ea15d737b 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -67,6 +67,8 @@ int ima_extra_slots __ro_after_init;
>  
>  struct ima_algo_desc *ima_algo_array __ro_after_init;
>  
> +unsigned long ima_unsupported_tpm_banks_mask __ro_after_init;
> +
>  static int __init ima_init_ima_crypto(void)
>  {
>  	long rc;
> @@ -150,8 +152,10 @@ int __init ima_init_crypto(void)
>  		ima_algo_array[i].algo = algo;
>  
>  		/* unknown TPM algorithm */
> -		if (algo == HASH_ALGO__LAST)
> +		if (algo == HASH_ALGO__LAST) {
> +			ima_unsupported_tpm_banks_mask |= BIT(i);
>  			continue;
> +		}
>  
>  		if (algo == ima_hash_algo) {
>  			ima_algo_array[i].tfm = ima_shash_tfm;
> @@ -167,6 +171,7 @@ int __init ima_init_crypto(void)
>  			}
>  
>  			ima_algo_array[i].tfm = NULL;
> +			ima_unsupported_tpm_banks_mask |= BIT(i);
>  		}
>  	}
>  
> @@ -625,26 +630,44 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data,
>  	u16 alg_id;
>  	int rc, i;
>  
> +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>  	rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
>  	if (rc)
>  		return rc;
>  
>  	entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
> +#endif
>  
>  	for (i = 0; i < NR_BANKS(ima_tpm_chip) + ima_extra_slots; i++) {
> +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>  		if (i == ima_sha1_idx)
>  			continue;
> +#endif
>  
>  		if (i < NR_BANKS(ima_tpm_chip)) {
>  			alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
>  			entry->digests[i].alg_id = alg_id;
>  		}
>  
> -		/* for unmapped TPM algorithms digest is still a padded SHA1 */
> +		/*
> +		 * For unmapped TPM algorithms, the digest is still a
> +		 * padded SHA1 if backwards-compatibility fallback PCR
> +		 * extension is enabled.  Otherwise fill with
> +		 * 0xffs. This is the value to invalidate unsupported
> +		 * PCR banks with once at first PCR use. Also, a
> +		 * non-all-zeroes value serves as an indicator to
> +		 * kexec measurement restoration that the entry is not
> +		 * a violation and all its template digests need to
> +		 * get recomputed.
> +		 */
>  		if (!ima_algo_array[i].tfm) {
> +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>  			memcpy(entry->digests[i].digest,
>  			       entry->digests[ima_sha1_idx].digest,
>  			       TPM_DIGEST_SIZE);
> +#else
> +			memset(entry->digests[i].digest, 0xff, TPM_DIGEST_SIZE);
> +#endif
>  			continue;
>  		}
>  
> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index f00ba2222c34..4db6c4be58fc 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -150,11 +150,27 @@ unsigned long ima_get_binary_runtime_size(void)
>  static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
>  {
>  	int result = 0;
> +	unsigned long tpm_banks_skip_mask;
>  
>  	if (!ima_tpm_chip)
>  		return result;
>  
> -	result = tpm_pcr_extend(ima_tpm_chip, pcr, digests_arg);
> +#if !IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
> +	tpm_banks_skip_mask = ima_unsupported_tpm_banks_mask;
> +	if (!(ima_extended_pcrs_mask & BIT(pcr))) {
> +		/*
> +		 * Invalidate unsupported banks once upon a PCR's
> +		 * first usage. Note that the digests[] entries for
> +		 * unsupported algorithms have been filled with 0xffs.
> +		 */
> +		tpm_banks_skip_mask = 0;
> +	}
> +#else
> +	tpm_banks_skip_mask = 0;
> +#endif
> +
> +	result = tpm_pcr_extend_sel(ima_tpm_chip, pcr, digests_arg,
> +				    tpm_banks_skip_mask);
>  	if (result != 0)
>  		pr_err("Error Communicating to TPM chip, result: %d\n", result);
>  	ima_extended_pcrs_mask |= BIT(pcr);
> @@ -280,9 +296,11 @@ int __init ima_init_digests(void)
>  		digest_size = ima_tpm_chip->allocated_banks[i].digest_size;
>  		crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
>  
> +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>  		/* for unmapped TPM algorithms digest is still a padded SHA1 */
>  		if (crypto_id == HASH_ALGO__LAST)
>  			digest_size = SHA1_DIGEST_SIZE;
> +#endif
>  
>  		memset(digests[i].digest, 0xff, digest_size);
>  	}

Powered by blists - more mailing lists