Message-ID: <CALCETrW6vMYZo-b7N9ojVSeZLVxhZjLBjnMHsULMGP6TaVYRHA@mail.gmail.com>
Date: Mon, 18 Nov 2024 10:43:18 -0800
From: Andy Lutomirski <luto@...capital.net>
To: "Daniel P. Smith" <dpsmith@...rtussolutions.com>
Cc: James Bottomley <James.Bottomley@...senpartnership.com>, 
	Thomas Gleixner <tglx@...utronix.de>, "Eric W. Biederman" <ebiederm@...ssion.com>, 
	Eric Biggers <ebiggers@...nel.org>, Ross Philipson <ross.philipson@...cle.com>, 
	linux-kernel@...r.kernel.org, x86@...nel.org, linux-integrity@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-crypto@...r.kernel.org, 
	kexec@...ts.infradead.org, linux-efi@...r.kernel.org, 
	iommu@...ts.linux-foundation.org, mingo@...hat.com, bp@...en8.de, 
	hpa@...or.com, dave.hansen@...ux.intel.com, ardb@...nel.org, 
	mjg59@...f.ucam.org, peterhuewe@....de, jarkko@...nel.org, jgg@...pe.ca, 
	nivedita@...m.mit.edu, herbert@...dor.apana.org.au, davem@...emloft.net, 
	corbet@....net, dwmw2@...radead.org, baolu.lu@...ux.intel.com, 
	kanth.ghatraju@...cle.com, andrew.cooper3@...rix.com, 
	trenchboot-devel@...glegroups.com
Subject: Re: [PATCH v9 06/19] x86: Add early SHA-1 support for Secure Launch
 early measurements

On Thu, Nov 14, 2024 at 5:17 PM Daniel P. Smith
<dpsmith@...rtussolutions.com> wrote:
>
> On 11/2/24 12:04, James Bottomley wrote:
> > On Sat, 2024-11-02 at 10:53 -0400, Daniel P. Smith wrote:
> >> Hi Luto,
> >>
> >> My apologies, I missed this response and the active on v11 cause me
> >> to
> >> get an inquiry why I hadn't responded.
> >>
> >> On 9/21/24 18:40, Andy Lutomirski wrote:
> > [...]
> >>> I assumed that "deliberately cap" meant that there was an actual
> >>> feature where you write something to the event log (if applicable)
> >>> and extend the PCR in a special way that *turns that PCR off*.
> >>> That is, it does something such that later-loaded software *can't*
> >>> use that PCR to attest or unseal anything, etc.
> >>>
> >>> But it sounds like you're saying that no such feature exists.  And
> >>> a quick skim of the specs doesn't come up with anything.  And the
> >>> SHA1 banks may well be susceptible to a collision attack.
> >>
> >> Correct, the only entity that can disable PCR banks is the firmware.
> >
> > No, that's not correct.  Any user can use TPM_PCR_Allocate to activate
> > or deactivate individual banks.  The caveat is the change is not
> > implemented until the next TPM reset (which should involve a reboot).
> > BIOS also gets to the TPM before the kernel does, so it can, in theory,
> > check what banks a TPM has and call TPM_PCR_Allocate to change them.
> > In practice, because this requires a reboot, this is usually only done
> > from the BIOS menus not on a direct boot ... so you can be reasonably
> > sure that whatever changes were made will stick.
>
> Okay, since there is a desire for exactness. Any system software can
> send the TPM_PCR_Allocate command, specifying which PCRs should be
> activated on next _TPM_init. There are restrictions such that if
> DRTM_PCR is defined, then at least one bank must have a D-RTM PCR
> allocation. In agreement with my statement, this is the mechanism used
> by firmware to select the banks. Depending on the firmware
> implementation, the firmware request will likely override the request
> sent by the system software.
>
> This brings us back to an earlier point, if one disables the SHA1 banks
> in BIOS menu, then TXT will not use them and thus neither will Secure
> Launch. Secure Launch will only use the algorithms used by the CPU and
> the ACM.
>
> >> When it initializes the TPM, it can disable banks/algorithms. After
> >> that, when an extend operation is done, the TPM is expecting an entry
> >> for all active PCR banks and the TPM itself does the extend hash that
> >> is stored into the PCRs.
> >
> > This, also, is not quite correct: an extend is allowed to specify banks
> > that don't exist (in which case nothing happens and no error is
> > reported) and miss banks that do (in which case no extend is done to
> > that bank).  In the early days of TPM2, some BIOS implementations only
> > extended sha1 for instance, meaning the sha256 banks were all zero when
> > the kernel started.
> >
> > Even today, if you activate a bank the BIOS doesn't know about, it
> > likely won't extend it.  You can see this in VM boots with OVMF and
> > software TPMs having esoteric banks like SM3.

How is this not a security hole you could drive a truck through?
Indeed, looking at the docs, TPM2_PCR_Extend says "If no digest value
is specified for a bank, then the PCR in that bank is not modified."

>
> Let me correct myself here and again be extremely precise. When an
> extend operation is done, the TPM driver expects to receive an array of
> digests that is the same size as the number of allocated/active banks.
> Specifically, it loops from 0 to chip->nr_allocated_banks, filling
> TPML_DIGEST_VALUES with an entry for all the active banks, to include
> SHA1 if it is active. Coming back to my response to Luto, we can either
> populate it with 0 or a well-known value for each extend we send.
> Regardless of what the value is, the TPM will use its implementation of
> SHA1 to calculate the resulting extend value.

At least extending unknown/unsupported banks with 0 modifies the bank,
which gives software that might rely on that bank an indication that
something in the chain doesn't support the bank.  But does actual
TPM-using software in the wild actually look up the event log and
notice that it contains a 0?

This sucks.  How on Earth didn't the TPM2 spec do this instead of
having explicit handling for "a PCR got extended, and the code that
extended it didn't support a given bank, and therefore *the resulting
PCR value cannot be relied on*?  It would have been *one single bit
per PCR, bank* indicating that the PCR's value is incomplete, along
with some basic logic that an incomplete PCR cannot magically become
complete, nor can it be used to authorize anything unless the
authorization policy explicitly allows it?

Anyway, other than the fact that everyone (presumably?) expects
software to be aware of SHA-1 and (mostly) SHA256, and presumably
users of SM3 already expect that a lot of things don't support it,
SHA1 doesn't seem very different from SM3 in the sense that (a) people
might not want to support it and (b) the actual behavior of a boot
chain component that doesn't support a cryptosystem is FUNDAMENTALLY
DANGEROUS.

Is there explicit guidance from TCG as to how this is supposed to work?


In any case, I have a strawman suggestion to resolve this issue much
better from Linux's perspective.  It's a strawman because, while I
attempted to read the relevant part of the specs, the specs and the
ecosystem are a mess, so I could be wrong.

Linux should not use TPM2_PCR_Extend *at all*.  Instead, Linux should
exclusively use TPM2_PCR_Event.  I would expect that passing, say, the
entire kernel image to TPM2_PCR_Event would be a big mistake, so
instead Linux should hash the relevant data with a reasonable
suggestion of hashes (which includes, mandatorily, SHA-384 and *does
not* include SHA-1, and may or may not be configurable at build time
to include things like SM3), concatenate them, and pass that to
TPM2_PCR_Event.  And Linux should make the value that it passed to
TPM2_PCR_Event readily accessible to software using it, and should
also include some straightforward tooling to calculate it from a given
input so that software that wants to figure out what value to expect
in a PCR can easily do so.

And then software that wants to use a SHA-1 bank will work every bit
as well as it would if Linux actually implemented it, but Linux can
happily not implement it, and even users of oddball algorithms that
Linux has never heard of will get secure behavior.

(Why SHA-384?  Because it's mandatory in the TPM Client profile, and
anyone who's happy with SHA-256 should also be willing to accept
SHA-384.)

>
> Even with these clarifications, the conclusion does not change. If the
> firmware enables SHA1, there is nothing that can be done to disable or
> block its usage from the user. Linux Secure Launch sending measurements
> to all the banks that the hardware used to start the DRTM chain does not
> create a vulnerability in and of itself. The user is free to leverage
> the SHA1 bank in any of the TPM's Integrity Collection suite of
> operations, regardless of what Secure Launch sends for the SHA1 hash.
> Whereas, neutering the solution of SHA1 breaks the ability for it to
> support any hardware that has a TPM1.2, of which there are still many in
> use.
>
> V/r,
> Daniel P. Smith
>
>


-- 
Andy Lutomirski
AMA Capital Management, LLC

Powered by blists - more mailing lists