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

[Daniel P. Smith]

Hi Luto.

On 8/28/24 23:17, Andy Lutomirski wrote:
> On Thu, Aug 15, 2024 at 12:10 PM Thomas Gleixner <tglx at linutronix.de> 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 at kernel.org> 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.

Hopefully this answers the outstanding comments on the SHA1 thread.

v/r,
dps