linux-kernel - Re: [PATCH v9 06/19] x86: Add early SHA-1 support for Secure Launch early measurements

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALCETrXXsta0OdgXb5Ti87psaty7gp5WRr-w8vTuEhOLuoGyXg@mail.gmail.com>
Date: Thu, 21 Nov 2024 12:54:44 -0800
From: Andy Lutomirski <luto@...capital.net>
To: ross.philipson@...cle.com
Cc: James Bottomley <James.Bottomley@...senpartnership.com>, 
	"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 Thu, Nov 21, 2024 at 12:11 PM <ross.philipson@...cle.com> wrote:
>
> On 11/18/24 12:02 PM, Andy Lutomirski wrote:

> > 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.

It's absolutely not outside the scope.  Look, this is quoted verbatim
from your patchset (v11, but I don't think this has materially
changed):

+       /* Early SL code ensured there was a max count of 2 digests */
+       for (i = 0; i < event->count; i++) {
+               dptr = (u8 *)alg_id_field + sizeof(u16);
+
+               for (j = 0; j < tpm->nr_allocated_banks; j++) {
+                       if (digests[j].alg_id != *alg_id_field)
+                               continue;

^^^^^^^^^^^^^^^^^^^^^ excuse me?

+
+                       switch (digests[j].alg_id) {
+                       case TPM_ALG_SHA256:
+                               memcpy(&digests[j].digest[0], dptr,
+                                      SHA256_DIGEST_SIZE);
+                               alg_id_field = (u16 *)((u8 *)alg_id_field +
+                                       SHA256_DIGEST_SIZE + sizeof(u16));
+                               break;
+                       case TPM_ALG_SHA1:
+                               memcpy(&digests[j].digest[0], dptr,
+                                      SHA1_DIGEST_SIZE);
+                               alg_id_field = (u16 *)((u8 *)alg_id_field +
+                                       SHA1_DIGEST_SIZE + sizeof(u16));
+                               break;
+                       default:
+                               break;
+                       }
+               }
+       }
+
+       ret = tpm_pcr_extend(tpm, event->pcr_idx, digests);
+       if (ret) {
+               pr_err("Error extending TPM20 PCR, result: %d\n", ret);
+               slaunch_txt_reset(txt, "Failed to extend TPM20 PCR\n",
+                                 SL_ERROR_TPM_EXTEND);
+       }

I haven't even tried to see what happens if there are more than two
allocated banks, but regardless, that 'continue' statement is a
vulnerability, and it's introduced in the patchset.  I'm not the
maintainer of this code, but I would NAK this.

I'm sure there's some reason that the TPM spec even makes code like
this possible, but it sure looks like the TPM2_PCR_Event operation
exists more or less to avoid this vulnerability.  I think you should
either use it or you should explain, convincingly, why Linux should
add code that does not use it and thus has a vulnerability in certain,
entirely plausible, firmware configurations.

This is brand new code that is explicitly security code.  I don't
think it's valid to spell "crud, we can't handle this case at all, and
failing to handle it is a security vulnerability" as "continue".  If
*I* were writing this code, I would use TPM2_PCR_Event, which is
entirely immune to this particular failure as far as I can see.


--Andy