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: <CALCETrV=PSLvDn4K6o1qoQLwTQtaPU6ESVPZTwRBJF5Pj_XJwg@mail.gmail.com>
Date: Thu, 21 Nov 2024 14:42:32 -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:54 PM Andy Lutomirski <luto@...capital.net> wrote:
>
> 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):


... I apologize -- I've misread the code.  That code is still wrong, I
think, but for an entirely different reason:

>
> +       /* 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;
> +                       }
> +               }
> +       }

If we fall off the end of the loop, we never increase alg_id_field,
and subsequent iterations will malfunction.  But we apparently will
write zeros (or fail?) if we have an unsupported algorithm, because we
are asking to extend all allocated banks.  I think.  This code is
gross.  It's plausible that this whole sequence is impossible unless
something malicious is going on.

Also, and I'm sort of replying to the wrong patch here, how
trustworthy is the data that's used to populate tpm_algs in the stub?
I don't think the results will be very pretty if tpm_algs ends up
being incorrect.