Message-ID: <66fabe21-7d0d-4978-806e-9a4af3e9257a@oracle.com>
Date: Thu, 21 Nov 2024 12:11:22 -0800
From: ross.philipson@...cle.com
To: Andy Lutomirski <luto@...capital.net>,
        James Bottomley <James.Bottomley@...senpartnership.com>
Cc: "Daniel P. Smith" <dpsmith@...rtussolutions.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Eric Biggers <ebiggers@...nel.org>, 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 11/18/24 12:02 PM, Andy Lutomirski wrote:
> On Mon, Nov 18, 2024 at 11:12 AM James Bottomley
> <James.Bottomley@...senpartnership.com> wrote:
>>
>> On Mon, 2024-11-18 at 10:43 -0800, Andy Lutomirski wrote:
>>> 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.
>>
>> Just for clarity, this is about how the agile log format works.  Each
>> event entry in the log contains a list of bank hashes and the extends
>> occur in log event order, so replaying a log should get you to exactly
>> the head PCR value of each bank.  If a log doesn't understand a format,
>> like SM3, then an entry for it doesn't appear in the log and a replay
>> says nothing about the PCR value.
> 
> I have no idea what the "agile log format" is or what all the formats
> in existence are.  I found section 4.2.4 here:
> 
> https://urldefense.com/v3/__https://trustedcomputinggroup.org/wp-content/uploads/TCG_IWG_CEL_v1_r0p41_pub.pdf__;!!ACWV5N9M2RV99hQ!Iw9aAHcJMT6j3t_DSb7cOk8iWy8VJYkJOlGQ_gtLUz0XwPcIZclY4I8GZJ5VP4OScLjBaz3RX1QlGGBWWZw$
> 
> It says:
> 
> This field contains the list of the digest values Extended. The Extend
> method varies with TPM command, so there is
> no uniform meaning of TPM Extend in this instance, and separate
> descriptions are unavoidable. If using the
> TPM2_PCR_Extend command, this field is the data sent to the TPM (i.e.,
> not the resulting value of the PCR after the
> TPM2_PCR_Extend command completes). If using the TPM2_PCR_Event
> command, this field contains the digest
> structure returned by the TPM2_PCR_Event command (that contains the
> digest(s) submitted to each PCR bank as
> the internal Extend operation). This field SHALL contain the
> information from the TPML_DIGEST_VALUES used in
> the Extend operation.
> 
> So we're logging the values with which we extend the PCRs.  Once upon
> a time, someone decided it was okay to skip extending a PCR bank:
> 
> https://urldefense.com/v3/__https://google.github.io/security-research/pocs/bios/tpm-carte-blanche/writeup.html__;!!ACWV5N9M2RV99hQ!Iw9aAHcJMT6j3t_DSb7cOk8iWy8VJYkJOlGQ_gtLUz0XwPcIZclY4I8GZJ5VP4OScLjBaz3RX1QlKxD4S1w$
> 
> and it was not a great idea.
> 
> There seem to be six (!) currently defined hashes: SHA1, SHA256,
> SHA384, SHA512, SM2 and SM3.  I haven't spotted anything promising not
> to add more.  It seems to be that Linux really really ought to:
> 
> (a) extend all banks.  Not all banks that the maintainers like, and
> not all banks that the maintainers knew about when having this
> discussion.  *All* banks.  That means TPM2_PCR_Event().  (Or we refuse
> to boot if there's a bank we don't like.)
> 
> (b) Make a best effort to notice if something is wrong with the TPM
> and/or someone is MITMing us and messing with us.  That means
> computing the hash algorithms we actually support and checking whether
> TPM2_PCR_Event() returns the right thing.  I'm not seeing a specific
> attack that seems likely that this prevents, but it does seem like
> decent defense in depth, and if someone chooses to provision a machine
> by reading its event log and then subsequently getting an attestation
> that a future event log matches what was read, then avoiding letting
> an attacker who temporarily controls the TPM connection from
> corrupting the results seems wise.  And I don't see anything at all
> that we gain by removing a check that (TPM's reported SHA1 == what we
> calculated) in the name of "not supporting SHA1") other than a few
> hundred bytes of object code.  (And yes, SHA1 is much more likely to
> be supported than SM3, so it's not absurd to implement SHA1 and not
> implement SM3.)
> 
>>
>> For some events, the hash is actually the hash of the event entry
>> itself and for others, the entry is just a hint and the hash is of
>> something else.
>>
>> I think part of the confusion stems from the twofold issues of PCRs: at
>> their simplest they were expected to provide the end policy values
>> (this turns out to be problematic because there are quite a few ways,
>> that will produce different end PCR values, that a system could get to
>> the same state).  If you don't trust a bank (or don't know about it),
>> you don't code it into a required policy statement and its value
>> becomes irrelevant.
> 
> I think that "you" refers to multiple entities, and this is a problem.
> 
> If the vendor of an attestation-dependent thing trusts SM3 but *Linux*
> does not like SM3, then the vendor's software should not become wildly
> insecure because Linux does not like SM3.  And, as that 2004 CVE
> shows, even two groups that are nominally associated with Microsoft
> can disagree on which banks they like, causing a vulnerability.

Thanks everyone for all the feedback and discussions on this. I 
understand it is important and perhaps the Linux TPM code should be 
modified to do the extend operations differently but this seems like it 
is outside the scope of our Secure Launch feature patch set.

As far as our patch series goes, we have done the things that were asked 
of us like documenting SHA-1 usage, fixing comments and commit message 
and breaking up the original patch into two (one for SHA-1 and one for 
SHA-256). It seems we should be able to submit our next version at this 
point.

Thanks
Ross

Powered by blists - more mailing lists