| Message ID | 20250323140911.226137-4-nstange@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: get rid of hard dependency on SHA-1 | expand |
On Sun, 2025-03-23 at 15:09 +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. I think this was done against the day IMA only supported sha1 and the TPM sha256 and beyond so there'd at least be a record that could be replayed. I think today with most distros defaulting IMAs hash to sha256 that's much less of a problem. > 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.) Don't extend unsupported banks at all. > c.) Record every event as a violation, i.e. extend unsupported banks > with 0xffs. > d.) Invalidate unsupported banks at least once by extending with a > unique > constant (e.g. with 0xfes). Instead of any of that, why not do what the TCG tells us to do for unsupported banks and simply cap them with 0xffffffff record EV_SEPARATOR and stop extending to them? (note this would probably require defining a separator event for IMA) Regards, James
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c > index 6f5696d999d0..a43080fb8edc 100644 > --- a/security/integrity/ima/ima_crypto.c > +++ b/security/integrity/ima/ima_crypto.c > @@ -625,26 +625,43 @@ 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 > + * 0xfes. This is the value to invalidate unsupported > + * PCR banks with. 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, 0xfe, TPM_DIGEST_SIZE); > +#endif Using TPM_DIGEST_SIZE will result in a padded 0xfe value. > continue; > } >
On Sun, 2025-03-23 at 17:18 -0400, James Bottomley wrote: > On Sun, 2025-03-23 at 15:09 +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. > > I think this was done against the day IMA only supported sha1 and the > TPM sha256 and beyond so there'd at least be a record that could be > replayed. I think today with most distros defaulting IMAs hash to > sha256 that's much less of a problem. Commit 1ea973df6e21 ("ima: Calculate and extend PCR with digests in ima_template_entry") added the support for extending multiple banks. It included the support for extending padded SHA1 digests for unknown TPM bank hash algorithms. Clearly it wasn't addressing the case of a TPM sha256 bank. > > > 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.) Don't extend unsupported banks at all. > > c.) Record every event as a violation, i.e. extend unsupported banks > > with 0xffs. > > d.) Invalidate unsupported banks at least once by extending with a > > unique > > constant (e.g. with 0xfes). > > Instead of any of that, why not do what the TCG tells us to do for > unsupported banks and simply cap them with 0xffffffff record > EV_SEPARATOR and stop extending to them? (note this would probably > require defining a separator event for IMA) open-writers and ToMToU integrity violations are added to the IMA measurement list as 0x00's, but are extended into the TPM using 0xFF's. Unfortunately, as mentioned previously, some verifiers ignore these integrity violations by automatically replacing the 0x00's with 0xFF's. What do you mean by "simply cap" them? Does it automatically prevent the PCR from being extended? If not, then this patch set is doing exactly that - preventing the TPM bank from additional extends. Mimi
On Mon, 2025-03-24 at 21:03 -0400, Mimi Zohar wrote: > On Sun, 2025-03-23 at 17:18 -0400, James Bottomley wrote: [...] > > Instead of any of that, why not do what the TCG tells us to do for > > unsupported banks and simply cap them with 0xffffffff record > > EV_SEPARATOR and stop extending to them? (note this would probably > > require defining a separator event for IMA) > > open-writers and ToMToU integrity violations are added to the IMA > measurement list as 0x00's, but are extended into the TPM using > 0xFF's. Unfortunately, as mentioned previously, some verifiers > ignore these integrity violations by automatically replacing the > 0x00's with 0xFF's. That sounds like something that should be fixed ... > What do you mean by "simply cap" them? Does it automatically prevent > the PCR from being extended? If not, then this patch set is doing > exactly that - preventing the TPM bank from additional extends. The idea of separators as understood by the TCG (the EV_SEPARATOR event) is that they divide the log up into different phases. If you see a measurement belonging to a prior phase after a separator you know some violation has occurred, even if the log itself verifies. The point being that if you log a separator in the last phase of boot (and for IMA logs there only is a single phase) there can be no more valid measurements after that event because of the separator, so the PCR is termed capped, meaning you can't validly extend to it and if you do the verifier shows a violation. Regards, James
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index 475c32615006..c8f12a4a4edf 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 "Enable compatibility TPM PCR extend for unsupported banks" + 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_crypto.c b/security/integrity/ima/ima_crypto.c index 6f5696d999d0..a43080fb8edc 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -625,26 +625,43 @@ 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 + * 0xfes. This is the value to invalidate unsupported + * PCR banks with. 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, 0xfe, TPM_DIGEST_SIZE); +#endif continue; } diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 83d53824aa98..0cc1189446a8 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -274,11 +274,23 @@ 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; memset(digests[i].digest, 0xff, digest_size); +#else + if (ima_algo_array[i].tfm) { + memset(digests[i].digest, 0xff, digest_size); + } else { + /* + * Unsupported banks are invalidated with 0xfe ... fe + * to disambiguate from violations. + */ + memset(digests[i].digest, 0xfe, digest_size); + } +#endif } return 0;
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.) Don't extend unsupported banks at all. c.) Record every event as a violation, i.e. extend unsupported banks with 0xffs. d.) Invalidate unsupported banks at least once by extending with a unique constant (e.g. with 0xfes). a.) would make verification from tools like ima_measurement nearly impossible, as it would have to guess or somehow determine ima_hash. b.) is a security risk, because the bank would validate an empty measurement list. c.) isn't ideal security-wise either, because an unsupported bank would then validate an all-violations measurement log. d.) is the only remaining viable option: extending unsupported PCR banks at least once with a unique constant of 0xfe ... fe not used for anything else would make those not to validate anything from that point on. For this last alternative d.), there are some variations possible, which differ in the number of times the magic 0xfe ... fe gets extended into unsupported banks for invalidation purposes. Among the practical ones are: - invalidate each unsupported bank over and over again for each new measurement log entry or - invalidate each unsupported bank exactly once. The second option has the advantage over the first that it would enable tools like ima-evm-utils' ima_measurement to recognize unsupported banks in O(1) time, just by comparing the PCR bank value against the constant HASH(00 .. 00 | fe .. fe). Note that if OTOH a bank got invalidated over and over again for each single log entry, and assuming that it's desired to report unsupported banks as such instead of just failing their validation, ima_measurement would have to try to match yet another PCR extension candidate sequence for the 0xfe .. fe over the complete measurement list, as it's currently being done for the padded SHA1s template hashes already. As appealing as the scheme to invalidate each unsupported bank exactly once might seem at first glance though, there's also the clear drawback of an additional tracking burden with a significant complexity on the kernel side: because IMA can't know ahead of time which out of all possible PCRs would ever get referenced from some policy rules, it cannot simply run the invalidation of unsupported banks upfront at __init, but would have to do it lazily upon a given PCR's first extension. The need for carrying the required state across kexecs, with the possibility of different kernels in the kexec chain potentially supporting different sets of hash algorithms, doesn't exactly make things easier either. So, to move towards the original goal of disentangling IMA from its hard dependency on SHA-1, go with the more straightforward route for now to invalidate unsupported PCR banks over and over again for each new measurement list entry recorded. The more sophisticated "invalidate-exactly-once" scheme will be left to future patches, if to be implemented at all. As this 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 by extending with constant 0xfe ... fe will be effective. 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, - to cover PCR extensions for "regular" measurement list entries, make ima_calc_field_array_hash() to fill the digests corresponding to banks with unsupported hash algorithms with 0xfes, - to cover PCR extensions for violation entries, make ima_init_digest() to likewise provision the digests[] elements corresponding to unsupported banks with 0xfes. [1] https://github.com/linux-integrity/ima-evm-utils Signed-off-by: Nicolai Stange <nstange@suse.de> --- security/integrity/ima/Kconfig | 14 ++++++++++++++ security/integrity/ima/ima_crypto.c | 19 ++++++++++++++++++- security/integrity/ima/ima_queue.c | 12 ++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-)