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

[Mimi Zohar]

On Tue, 2025-03-18 at 11:26 +0100, Nicolai Stange wrote:
> 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?

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@xxxxxxx>
> > > ---
>