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

[Andy Lutomirski]

On Thu, Sep 12, 2024 at 5:34 PM Daniel P. Smith
<dpsmith@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Hey again,
>
> On 9/4/24 21:01, Daniel P. Smith wrote:
> > Hi Luto.
> >
> > On 8/28/24 23:17, Andy Lutomirski wrote:
> >> On Thu, Aug 15, 2024 at 12:10 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> >> wrote:
> >>>
> >>> On Thu, Aug 15 2024 at 13:38, Daniel P. Smith wrote:
> >>>> On 5/31/24 09:54, Eric W. Biederman wrote:
> >>>>> Eric Biggers <ebiggers@xxxxxxxxxx> writes:
> >>>>>> That paragraph is also phrased as a hypothetical, "Even if we'd
> >>>>>> prefer to use
> >>>>>> SHA-256-only".  That implies that you do not, in fact, prefer
> >>>>>> SHA-256 only.  Is
> >>>>>> that the case?  Sure, maybe there are situations where you *have*
> >>>>>> to use SHA-1,
> >>>>>> but why would you not at least *prefer* SHA-256?
> >>>>>
> >>>>> Yes.  Please prefer to use SHA-256.
> >>>>>
> >>>>> Have you considered implementing I think it is SHA1-DC (as git has)
> >>>>> that
> >>>>> is compatible with SHA1 but blocks the known class of attacks where
> >>>>> sha1 is actively broken at this point?
> >>>>
> >>>> We are using the kernel's implementation, addressing what the kernel
> >>>> provides is beyond our efforts. Perhaps someone who is interested in
> >>>> improving the kernel's SHA1 could submit a patch implementing/replacing
> >>>> it with SHA1-DC, as I am sure the maintainers would welcome the help.
> >>>
> >>> Well, someone who is interested to get his "secure" code merged should
> >>> have a vested interested to have a non-broken SHA1 implementation if
> >>> there is a sensible requirement to use SHA1 in that new "secure" code,
> >>> no?
> >>>
> >>> Just for the record. The related maintainers can rightfully decide to
> >>> reject known broken "secure" code on a purely technical argument.
> >>>
> >>
> >> Wait, hold on a second.
> >>
> >> SHA1-DC isn't SHA1.  It's a different hash function that is mostly
> >> compatible with SHA1, is different on some inputs, and is maybe more
> >> secure.  But the _whole point_ of using SHA1 in the TPM code (well,
> >> this really should be the whole point for new applications) is to
> >> correctly cap the SHA1 PCRs so we can correctly _turn them off_ in the
> >> best way without breaking compatibility with everything that might
> >> read the event log.  I think that anyone suggesting using SHA1-DC for
> >> this purpose should give some actual analysis as to why they think
> >> it's an improvement, let alone even valid.
> >
> > I would say at a minimum it is to provide a means to cap the PCRs.
> > Devices with TPM1.2 are still prevalent in the wild for which members of
> > the TrenchBoot community support, and there are still valid (and secure)
> > verification uses for SHA1 that I outlined in my previous response.
> >
> >> Ross et al, can you confirm that your code actually, at least by
> >> default and with a monstrous warning to anyone who tries to change the
> >> default, caps SHA1 PCRs if SHA256 is available?  And then can we maybe
> >> all stop hassling the people trying to develop this series about the
> >> fact that they're doing their best with the obnoxious system that the
> >> TPM designers gave them?
> >
> > Our goal is to keep control in the hands of the user, not making
> > unilateral decisions on their behalf. In the currently deployed
> > solutions it is left to the initrd (user) to cap the PCRs. After some
> > thinking, we can still ensure user control and give an option to cap the
> > PCRs earlier. We hope to post a v11 later this week or early next week
> > that introduces a new policy field to the existing measurement policy
> > framework. Will add/update the kernel docs with respect to the policy
> > expansion. We are also looking the best way we might add a warning to
> > the kernel log if the SHA1 bank is used beyond capping the PCRs.
>
> As the attempt was made to lay in the policy logic, it started to become
> convoluted and unnecessarily complicated. Thus creating more risk with
> all the bookkeeping and yet sha1 hashes still have to be sent, the null
> hash in this case, since the TPM driver will reject extends that do not
> have hashes for all active banks. At this point, we have opted to keep
> the logic simple and add a section to our documentation advising of the
> potential risk should one choose to incorporate SHA1 in their
> attestations of the platform.
>

I've read the TPM standard a bit, but it's been awhile, and it's too
complicated anyway.  So, can you remind me (and probably 3/4 of the
other people on this thread, too):

What, exactly, is your patchset doing that requires hashing at all?
(I assume it's extending a PCR and generating an event log entry.).
What, exactly, does it mean to "cap" a PCR?  How is this different
from what your patchset does?

With that answered, it will hopefully be easy to see that you're
making the right call :)

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC