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
