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

[Mimi Zohar]

On Tue, 2025-03-18 at 16:55 +0100, Nicolai Stange wrote:
> Mimi Zohar <zohar@xxxxxxxxxxxxx> writes:
> 
> > 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.
> 
> 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@xxxxxxx>
> > > > > ---
> > > 
> > 
>