| Message ID | 20250313173339.3815589-7-nstange@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: get rid of hard dependency on SHA-1 | expand |
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@suse.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); > }
Mimi Zohar <zohar@linux.ibm.com> writes: > 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 Mimi, > What a pleasure reviewing your patch set. Nicely organized. Well written patch > descriptions. thank you :) > 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. Yes. However with your comment I'm now realizing there's an issue if the set of supported hash algorithms differs between the previous and the next, kexeced kernel -- something I admittedly hadn't thought of before. The current behavior as implemented in this RFC is that an unsupported PCR bank would get invalidated *once* upon first use, i.e. extended once with e.g. all 0xFEs. (Note that the actual patch implements invalidation with all 0xFFs, for the choice of the exact invalidation value see below). The idea is that a.) tools could easily recognize this by comparing the PCR bank value against constant HASH(00 .. 00 | fe ... fe) b.) and they would fail to verify any non-trivial event log against such a PCR bank if they did not do that comparison ahead. In order to implement this invalidate-once logic, there's that ima_extended_pcrs_mask you asked about in reply to [3/7], the preparatory patch for [4/7] ("ima: track the set of PCRs ever extended"). As the set of PCRs ever to be found in any policy rule cannot be predicted, their unsupported banks cannot get invalidated once at __init. Hence this inalidate-at-first-extend logic, which needs that tracking of PCRs ever extended as maintained in ima_extended_pcrs_mask. Upon kexec, the current patchset attempts to restore the ima_extended_pcrs_mask from the previous kernel by walking through the measurement list, setting a bit for each PCR found in any event. Now consider the following: - some hash algorithm is supported by the initially booted kernel, - but not in the subsequently kexeced one. The initially booted kernel would not invalidate the given hash algorithm's bank for any PCR, and the kexeced one would neither, because it would restore the ima_extended_pcrs_mask from the initially booted one. However, the kexeced kernel would also not extend any further events into the now unsupported PCR banks then. That means that these PCR banks would happily verify a measurement list truncated to the point before the kexec, which is of course bad. I can see two ways around this: a.) Give up on the invalidate-once scheme, unconditionally invalidate unsupported banks (with 0xfe .. fe) for every new measurement list entry. b.) Make the kexeced kernel to read back PCR banks it doesn't support from the TPM at __init and see if they had been invalidated by the previous kernel. Set the bit in ima_extended_pcrs_mask *only* if so. That is, invalidate unsupported and not yet invalidated PCR banks upon first use. Also, make it read PCR banks it does support and refrain from further extending any found to have been invalidated before (for all PCRs mentioned in the measurement list). That is, leave previously invalidated PCR banks alone. Going with a.) would mean that verifiers would not be able to recognize in O(1) anymore that some bank was unsupported and had not been maintained by the kernel. It would still be possible to figure in linear time whether neither of the kernels in a kexec chain covered by a single measurement list did support a given PCR bank hash. For implementing b.), one would have to store a table of precomputed HASH(00 .. 00 | fe .. fe) values for every recognized hash possible in .rodata for comparison purposes, i.e. for every entry in tpm2_hash_map[5] at least -- after all, the whole point is to deal with hashes for which no implementation is available to the kernel, so these values cannot get computed dynamically at runtime. With that, if the initially booted kernel did not support some hash algorithm, it would be recognizable by verifiers in O(1) time. If the initially booted kernel did support a given hash, but a subsequent kernel in the kexec chain would not, the PCR would get invalidated by the latter. This sitatuation cannot be detected at all (with reasonable effort) from the final PCR hash bank value alone and verification against it would fail then. Perhaps it's noteworthy that this is true with any possible scheme, including the currently implemented one extending with padded SHA1 into unsupported banks. I think that the decision about what to do now boils down to whether there's any value in verifiers being able to tell that a PCR bank had been unsupported and not been maintained rather than to simply fail its verification if attempted. If it is not important, or linear time + the additional implementation complexity burden at the verifier side is acceptable, the much simpler a.) would do. Otherwise I could give implementing b.) a try and see how bad the resulting code would get. What do you think? > 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. Yeah, there's indeed an ambiguity. I think the right thing to do is to make measurement lists unverifiable against unsupported banks and would propose to use 0xfe ... fe for the associated invalidations instead of the 0xff .. ff used for violation events already. Thanks a lot! Nicolai >> >> 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@suse.de> >> ---
On Tue, 2025-03-18 at 11:26 +0100, Nicolai Stange wrote: > Mimi Zohar <zohar@linux.ibm.com> writes: > > > 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 Mimi, > > > What a pleasure reviewing your patch set. Nicely organized. Well written patch > > descriptions. > > thank you :) > > > 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. > > Yes. However with your comment I'm now realizing there's an issue if the > set of supported hash algorithms differs between the previous and the > next, kexeced kernel -- something I admittedly hadn't thought of before. > > The current behavior as implemented in this RFC is that an unsupported > PCR bank would get invalidated *once* upon first use, i.e. extended once > with e.g. all 0xFEs. (Note that the actual patch implements invalidation > with all 0xFFs, for the choice of the exact invalidation value see > below). The idea is that > a.) tools could easily recognize this by comparing the PCR bank value > against constant HASH(00 .. 00 | fe ... fe) > b.) and they would fail to verify any non-trivial event log against such > a PCR bank if they did not do that comparison ahead. > > In order to implement this invalidate-once logic, there's that > ima_extended_pcrs_mask you asked about in reply to [3/7], the > preparatory patch for [4/7] ("ima: track the set of PCRs ever > extended"). As the set of PCRs ever to be found in any policy rule > cannot be predicted, their unsupported banks cannot get invalidated once > at __init. Hence this inalidate-at-first-extend logic, which needs that > tracking of PCRs ever extended as maintained in ima_extended_pcrs_mask. > > Upon kexec, the current patchset attempts to restore the > ima_extended_pcrs_mask from the previous kernel by walking through the > measurement list, setting a bit for each PCR found in any event. > > Now consider the following: > - some hash algorithm is supported by the initially booted kernel, > - but not in the subsequently kexeced one. > > The initially booted kernel would not invalidate the given hash > algorithm's bank for any PCR, and the kexeced one would neither, because > it would restore the ima_extended_pcrs_mask from the initially booted > one. However, the kexeced kernel would also not extend any further > events into the now unsupported PCR banks then. That means that these > PCR banks would happily verify a measurement list truncated to the point > before the kexec, which is of course bad. > > > I can see two ways around this: > a.) Give up on the invalidate-once scheme, unconditionally invalidate > unsupported banks (with 0xfe .. fe) for every new measurement list > entry. > > b.) Make the kexeced kernel to read back PCR banks it doesn't support > from the TPM at __init and see if they had been invalidated by the > previous kernel. Set the bit in ima_extended_pcrs_mask *only* if so. > That is, invalidate unsupported and not yet invalidated PCR banks > upon first use. > > Also, make it read PCR banks it does support and refrain from > further extending any found to have been invalidated before (for all > PCRs mentioned in the measurement list). That is, leave previously > invalidated PCR banks alone. > > Going with a.) would mean that verifiers would not be able to recognize > in O(1) anymore that some bank was unsupported and had not been > maintained by the kernel. It would still be possible to figure in linear > time whether neither of the kernels in a kexec chain covered by a single > measurement list did support a given PCR bank hash. > > For implementing b.), one would have to store a table of precomputed > HASH(00 .. 00 | fe .. fe) values for every recognized hash possible in > .rodata for comparison purposes, i.e. for every entry in > tpm2_hash_map[5] at least -- after all, the whole point is to deal with > hashes for which no implementation is available to the kernel, so these > values cannot get computed dynamically at runtime. > > With that, if the initially booted kernel did not support some hash > algorithm, it would be recognizable by verifiers in O(1) time. > > If the initially booted kernel did support a given hash, but a > subsequent kernel in the kexec chain would not, the PCR would get > invalidated by the latter. This sitatuation cannot be detected at all > (with reasonable effort) from the final PCR hash bank value alone and > verification against it would fail then. Perhaps it's noteworthy that > this is true with any possible scheme, including the currently > implemented one extending with padded SHA1 into unsupported banks. > > > I think that the decision about what to do now boils down to whether > there's any value in verifiers being able to tell that a PCR bank had > been unsupported and not been maintained rather than to simply fail its > verification if attempted. > > If it is not important, or linear time + the additional implementation > complexity burden at the verifier side is acceptable, the much simpler > a.) would do. > > Otherwise I could give implementing b.) a try and see how bad the > resulting code would get. > > What do you think? Let me try to summarize 'b'. The initial unsupported hash algorithms would continue to be unsupported in subsequent kexec's. However this does not address the case where the initial kernel image supported a hash algorithm, but the subsequent kexec'ed image does not. The TPM bank has already been extended with other values. In this case, like the original violation the attestation service would not verify. If I'm understanding it correctly, 'b' is thus a partial solution. My concern with 'b' is the ability to read the multiple TPM bank PCRs so early during kernel initialization. Will it succeed? If it does succeed, will it introduce initialization delays? FYI, because the IMA Kconfig selects SHA1, we're guaranteed that SHA1 exists in the kernel and the subsequent kexec'ed kernel. For this reason we're guaranteed that the measurement list is complete. The simplest solution, not necessarily the best, would be to punt the problem for the time being by replacing the "select" with a different hash algorithm. > > > > 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. > > Yeah, there's indeed an ambiguity. I think the right thing to do is to > make measurement lists unverifiable against unsupported banks and would > propose to use 0xfe ... fe for the associated invalidations instead of > the 0xff .. ff used for violation events already. I just realized that unlike the existing open-writers/ToMToU violations, by definition the new unsupported bank violation would not be included in the measurement list, but just extended into the TPM. 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@suse.de> > > > --- >
Mimi Zohar <zohar@linux.ibm.com> writes: > On Tue, 2025-03-18 at 11:26 +0100, Nicolai Stange wrote: >> Mimi Zohar <zohar@linux.ibm.com> writes: >> >> > 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 Mimi, >> >> > What a pleasure reviewing your patch set. Nicely organized. Well written patch >> > descriptions. >> >> thank you :) >> >> > 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. >> >> Yes. However with your comment I'm now realizing there's an issue if the >> set of supported hash algorithms differs between the previous and the >> next, kexeced kernel -- something I admittedly hadn't thought of before. >> >> The current behavior as implemented in this RFC is that an unsupported >> PCR bank would get invalidated *once* upon first use, i.e. extended once >> with e.g. all 0xFEs. (Note that the actual patch implements invalidation >> with all 0xFFs, for the choice of the exact invalidation value see >> below). The idea is that >> a.) tools could easily recognize this by comparing the PCR bank value >> against constant HASH(00 .. 00 | fe ... fe) >> b.) and they would fail to verify any non-trivial event log against such >> a PCR bank if they did not do that comparison ahead. >> >> In order to implement this invalidate-once logic, there's that >> ima_extended_pcrs_mask you asked about in reply to [3/7], the >> preparatory patch for [4/7] ("ima: track the set of PCRs ever >> extended"). As the set of PCRs ever to be found in any policy rule >> cannot be predicted, their unsupported banks cannot get invalidated once >> at __init. Hence this inalidate-at-first-extend logic, which needs that >> tracking of PCRs ever extended as maintained in ima_extended_pcrs_mask. >> >> Upon kexec, the current patchset attempts to restore the >> ima_extended_pcrs_mask from the previous kernel by walking through the >> measurement list, setting a bit for each PCR found in any event. >> >> Now consider the following: >> - some hash algorithm is supported by the initially booted kernel, >> - but not in the subsequently kexeced one. >> >> The initially booted kernel would not invalidate the given hash >> algorithm's bank for any PCR, and the kexeced one would neither, because >> it would restore the ima_extended_pcrs_mask from the initially booted >> one. However, the kexeced kernel would also not extend any further >> events into the now unsupported PCR banks then. That means that these >> PCR banks would happily verify a measurement list truncated to the point >> before the kexec, which is of course bad. >> >> >> I can see two ways around this: >> a.) Give up on the invalidate-once scheme, unconditionally invalidate >> unsupported banks (with 0xfe .. fe) for every new measurement list >> entry. >> >> b.) Make the kexeced kernel to read back PCR banks it doesn't support >> from the TPM at __init and see if they had been invalidated by the >> previous kernel. Set the bit in ima_extended_pcrs_mask *only* if so. >> That is, invalidate unsupported and not yet invalidated PCR banks >> upon first use. >> >> Also, make it read PCR banks it does support and refrain from >> further extending any found to have been invalidated before (for all >> PCRs mentioned in the measurement list). That is, leave previously >> invalidated PCR banks alone. >> >> Going with a.) would mean that verifiers would not be able to recognize >> in O(1) anymore that some bank was unsupported and had not been >> maintained by the kernel. It would still be possible to figure in linear >> time whether neither of the kernels in a kexec chain covered by a single >> measurement list did support a given PCR bank hash. >> >> For implementing b.), one would have to store a table of precomputed >> HASH(00 .. 00 | fe .. fe) values for every recognized hash possible in >> .rodata for comparison purposes, i.e. for every entry in >> tpm2_hash_map[5] at least -- after all, the whole point is to deal with >> hashes for which no implementation is available to the kernel, so these >> values cannot get computed dynamically at runtime. >> >> With that, if the initially booted kernel did not support some hash >> algorithm, it would be recognizable by verifiers in O(1) time. >> >> If the initially booted kernel did support a given hash, but a >> subsequent kernel in the kexec chain would not, the PCR would get >> invalidated by the latter. This sitatuation cannot be detected at all >> (with reasonable effort) from the final PCR hash bank value alone and >> verification against it would fail then. Perhaps it's noteworthy that >> this is true with any possible scheme, including the currently >> implemented one extending with padded SHA1 into unsupported banks. >> >> >> I think that the decision about what to do now boils down to whether >> there's any value in verifiers being able to tell that a PCR bank had >> been unsupported and not been maintained rather than to simply fail its >> verification if attempted. >> >> If it is not important, or linear time + the additional implementation >> complexity burden at the verifier side is acceptable, the much simpler >> a.) would do. >> >> Otherwise I could give implementing b.) a try and see how bad the >> resulting code would get. >> >> What do you think? > > Let me try to summarize 'b'. The initial unsupported hash algorithms would > continue to be unsupported in subsequent kexec's. However this does not address > the case where the initial kernel image supported a hash algorithm, but the > subsequent kexec'ed image does not. The TPM bank has already been extended with > other values. In this case, like the original violation the attestation service > would not verify. If I'm understanding it correctly, 'b' is thus a partial > solution. Yes, that all matches exactly what I was trying to say. FWIW, I might be way too naive, but I would expect two categories of existing verifier behaviors: - "Real" ones like keylime or so, which are being asked to verify against a single specific bank and the result is either yes or no with no inbetween. In particular, these wouldn't fallback to checking whether something else like padded SHA1s would perhaps verify. - The ones more in the development/debugging/testsuite realm, which would attempt to verify against all banks and fail (the test?) if any does not verify. These would try harder to avoid false negatives by testing for the alternatives like padded SHA1s as well. I suppose ima-evm-utils' ima_measurement would qualify as such one. For the first class, simply invalidating the unsupported PCR banks somehow, i.e. option a.), is good enough. For the second kind however, the question is whether something like b.) would be helpful and the additional complexity on the kernel side warranted. But agreed, it's a partial and best-effort solution. If one kexecs into a kernel with a proper subset of supported TPM bank hashes, it wouldnt't work. > My concern with 'b' is the ability to read the multiple TPM bank PCRs so early > during kernel initialization. Will it succeed? If it does succeed, will it > introduce initialization delays? As the boot aggregate gets extended already at that point in time (IIRC), I'd expect that reading the PCRs would probably succeed as well. For the delays imposed on the kexec restore path -- I can't tell unfortunately. But I would only do it on kexec restore if the measurement list is non-empty anyway, so systems not having IMA enabled or ones which wouldn't kexec are not affected. > FYI, because the IMA Kconfig selects SHA1, we're guaranteed that SHA1 exists in > the kernel and the subsequent kexec'ed kernel. For this reason we're guaranteed > that the measurement list is complete. The simplest solution, not necessarily > the best, would be to punt the problem for the time being by replacing the > "select" with a different hash algorithm. Yes, that would work as well. IIUC, it would mean that we would e.g. extend truncated SHA-256 template hashes into a SHA-1 bank, right? However, since no existing tool like 'ima_measurement' is expecting that, and would fail a verification then, I'm currently struggling to see the advantage over just doing a.) and invalidating the PCR banks with a fixed value right away? >> > 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. >> >> Yeah, there's indeed an ambiguity. I think the right thing to do is to >> make measurement lists unverifiable against unsupported banks and would >> propose to use 0xfe ... fe for the associated invalidations instead of >> the 0xff .. ff used for violation events already. > > I just realized that unlike the existing open-writers/ToMToU violations, by > definition the new unsupported bank violation would not be included in the > measurement list, but just extended into the TPM. That's true, but when invalidating unsupported banks with a single ff ... ff, one could successfully verify a measurement list having only a single violation entry against an invalidated bank AFAICS. I think it would be more robust to use a different constant for the invalidation. Thanks! Nicolai >> > > 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@suse.de> >> > > --- >> >
On Tue, 2025-03-18 at 16:55 +0100, Nicolai Stange wrote: > Mimi Zohar <zohar@linux.ibm.com> writes: > > > On Tue, 2025-03-18 at 11:26 +0100, Nicolai Stange wrote: > > > Mimi Zohar <zohar@linux.ibm.com> writes: > > > > > > > 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 Mimi, > > > > > > > What a pleasure reviewing your patch set. Nicely organized. Well written patch > > > > descriptions. > > > > > > thank you :) > > > > > > > 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. > > > > > > Yes. However with your comment I'm now realizing there's an issue if the > > > set of supported hash algorithms differs between the previous and the > > > next, kexeced kernel -- something I admittedly hadn't thought of before. > > > > > > The current behavior as implemented in this RFC is that an unsupported > > > PCR bank would get invalidated *once* upon first use, i.e. extended once > > > with e.g. all 0xFEs. (Note that the actual patch implements invalidation > > > with all 0xFFs, for the choice of the exact invalidation value see > > > below). The idea is that > > > a.) tools could easily recognize this by comparing the PCR bank value > > > against constant HASH(00 .. 00 | fe ... fe) > > > b.) and they would fail to verify any non-trivial event log against such > > > a PCR bank if they did not do that comparison ahead. > > > > > > In order to implement this invalidate-once logic, there's that > > > ima_extended_pcrs_mask you asked about in reply to [3/7], the > > > preparatory patch for [4/7] ("ima: track the set of PCRs ever > > > extended"). As the set of PCRs ever to be found in any policy rule > > > cannot be predicted, their unsupported banks cannot get invalidated once > > > at __init. Hence this inalidate-at-first-extend logic, which needs that > > > tracking of PCRs ever extended as maintained in ima_extended_pcrs_mask. > > > > > > Upon kexec, the current patchset attempts to restore the > > > ima_extended_pcrs_mask from the previous kernel by walking through the > > > measurement list, setting a bit for each PCR found in any event. > > > > > > Now consider the following: > > > - some hash algorithm is supported by the initially booted kernel, > > > - but not in the subsequently kexeced one. > > > > > > The initially booted kernel would not invalidate the given hash > > > algorithm's bank for any PCR, and the kexeced one would neither, because > > > it would restore the ima_extended_pcrs_mask from the initially booted > > > one. However, the kexeced kernel would also not extend any further > > > events into the now unsupported PCR banks then. That means that these > > > PCR banks would happily verify a measurement list truncated to the point > > > before the kexec, which is of course bad. > > > > > > > > > I can see two ways around this: > > > a.) Give up on the invalidate-once scheme, unconditionally invalidate > > > unsupported banks (with 0xfe .. fe) for every new measurement list > > > entry. > > > > > > b.) Make the kexeced kernel to read back PCR banks it doesn't support > > > from the TPM at __init and see if they had been invalidated by the > > > previous kernel. Set the bit in ima_extended_pcrs_mask *only* if so. > > > That is, invalidate unsupported and not yet invalidated PCR banks > > > upon first use. > > > > > > Also, make it read PCR banks it does support and refrain from > > > further extending any found to have been invalidated before (for all > > > PCRs mentioned in the measurement list). That is, leave previously > > > invalidated PCR banks alone. > > > > > > Going with a.) would mean that verifiers would not be able to recognize > > > in O(1) anymore that some bank was unsupported and had not been > > > maintained by the kernel. It would still be possible to figure in linear > > > time whether neither of the kernels in a kexec chain covered by a single > > > measurement list did support a given PCR bank hash. > > > > > > For implementing b.), one would have to store a table of precomputed > > > HASH(00 .. 00 | fe .. fe) values for every recognized hash possible in > > > .rodata for comparison purposes, i.e. for every entry in > > > tpm2_hash_map[5] at least -- after all, the whole point is to deal with > > > hashes for which no implementation is available to the kernel, so these > > > values cannot get computed dynamically at runtime. > > > > > > With that, if the initially booted kernel did not support some hash > > > algorithm, it would be recognizable by verifiers in O(1) time. > > > > > > If the initially booted kernel did support a given hash, but a > > > subsequent kernel in the kexec chain would not, the PCR would get > > > invalidated by the latter. This sitatuation cannot be detected at all > > > (with reasonable effort) from the final PCR hash bank value alone and > > > verification against it would fail then. Perhaps it's noteworthy that > > > this is true with any possible scheme, including the currently > > > implemented one extending with padded SHA1 into unsupported banks. > > > > > > > > > I think that the decision about what to do now boils down to whether > > > there's any value in verifiers being able to tell that a PCR bank had > > > been unsupported and not been maintained rather than to simply fail its > > > verification if attempted. > > > > > > If it is not important, or linear time + the additional implementation > > > complexity burden at the verifier side is acceptable, the much simpler > > > a.) would do. > > > > > > Otherwise I could give implementing b.) a try and see how bad the > > > resulting code would get. > > > > > > What do you think? > > > > Let me try to summarize 'b'. The initial unsupported hash algorithms would > > continue to be unsupported in subsequent kexec's. However this does not address > > the case where the initial kernel image supported a hash algorithm, but the > > subsequent kexec'ed image does not. The TPM bank has already been extended with > > other values. In this case, like the original violation the attestation service > > would not verify. If I'm understanding it correctly, 'b' is thus a partial > > solution. > > Yes, that all matches exactly what I was trying to say. FWIW, I might be > way too naive, but I would expect two categories of existing verifier > behaviors: > - "Real" ones like keylime or so, which are being asked to verify > against a single specific bank and the result is either yes or no with > no inbetween. In particular, these wouldn't fallback to checking > whether something else like padded SHA1s would perhaps verify. > - The ones more in the development/debugging/testsuite realm, which > would attempt to verify against all banks and fail (the test?) if any > does not verify. These would try harder to avoid false negatives by > testing for the alternatives like padded SHA1s as well. I suppose > ima-evm-utils' ima_measurement would qualify as such one. > > For the first class, simply invalidating the unsupported PCR banks > somehow, i.e. option a.), is good enough. For the second kind however, > the question is whether something like b.) would be helpful and the > additional complexity on the kernel side warranted. But agreed, it's a > partial and best-effort solution. If one kexecs into a kernel with a > proper subset of supported TPM bank hashes, it wouldnt't work. Refer to comment on "Replacing the "Kconfig select". > > > My concern with 'b' is the ability to read the multiple TPM bank PCRs so early > > during kernel initialization. Will it succeed? If it does succeed, will it > > introduce initialization delays? > > As the boot aggregate gets extended already at that point in time > (IIRC), I'd expect that reading the PCRs would probably succeed as > well. For the delays imposed on the kexec restore path -- I can't tell > unfortunately. But I would only do it on kexec restore if the > measurement list is non-empty anyway, so systems not having IMA enabled > or ones which wouldn't kexec are not affected. Ok > > > > FYI, because the IMA Kconfig selects SHA1, we're guaranteed that SHA1 exists in > > the kernel and the subsequent kexec'ed kernel. For this reason we're guaranteed > > that the measurement list is complete. The simplest solution, not necessarily > > the best, would be to punt the problem for the time being by replacing the > > "select" with a different hash algorithm. > > Yes, that would work as well. IIUC, it would mean that we would > e.g. extend truncated SHA-256 template hashes into a SHA-1 bank, right? > However, since no existing tool like 'ima_measurement' is expecting > that, and would fail a verification then, I'm currently struggling to > see the advantage over just doing a.) and invalidating the PCR banks > with a fixed value right away? Replacing the "Kconfig select" has more to do with having at least one guaranteed complete measurement list. I'm fine with extending a TPM bank with an unknown kernel hash algorithm violation (either option a or b). > > > > > 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. > > > > > > Yeah, there's indeed an ambiguity. I think the right thing to do is to > > > make measurement lists unverifiable against unsupported banks and would > > > propose to use 0xfe ... fe for the associated invalidations instead of > > > the 0xff .. ff used for violation events already. > > > > I just realized that unlike the existing open-writers/ToMToU violations, by > > definition the new unsupported bank violation would not be included in the > > measurement list, but just extended into the TPM. > > That's true, but when invalidating unsupported banks with a single ff > ... ff, one could successfully verify a measurement list having only a > single violation entry against an invalidated bank AFAICS. I think it > would be more robust to use a different constant for the invalidation. Agreed. thanks, 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@suse.de> > > > > > --- > > > > > >
Mimi Zohar <zohar@linux.ibm.com> writes: > On Tue, 2025-03-18 at 16:55 +0100, Nicolai Stange wrote: >> Mimi Zohar <zohar@linux.ibm.com> writes: >> > FYI, because the IMA Kconfig selects SHA1, we're guaranteed that SHA1 exists in >> > the kernel and the subsequent kexec'ed kernel. For this reason we're guaranteed >> > that the measurement list is complete. The simplest solution, not necessarily >> > the best, would be to punt the problem for the time being by replacing the >> > "select" with a different hash algorithm. >> >> Yes, that would work as well. IIUC, it would mean that we would >> e.g. extend truncated SHA-256 template hashes into a SHA-1 bank, right? >> However, since no existing tool like 'ima_measurement' is expecting >> that, and would fail a verification then, I'm currently struggling to >> see the advantage over just doing a.) and invalidating the PCR banks >> with a fixed value right away? > > Replacing the "Kconfig select" has more to do with having at least one > guaranteed complete measurement list. I'm fine with extending a TPM bank with > an unknown kernel hash algorithm violation (either option a or b). Ok, I think I got it now. FWIW, a v2 can be found at https://lore.kernel.org/r/20250323140911.226137-1-nstange@suse.de , including a patch for selecting SHA256 now. Thanks a lot for all your feedback! Nicolai
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); }
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). 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@suse.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(-)