Re: [RFC PATCH v1 6/7] ima: invalidate unsupported PCR banks once at first use

[Nicolai Stange]

Mimi Zohar <zohar@xxxxxxxxxxxxx> 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@xxxxxxx>
>> ---

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)