diff mbox series

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

Message ID 20240531010331.134441-7-ross.philipson@oracle.com
State Superseded
Headers show
Series x86: Trenchboot secure dynamic launch Linux kernel support | expand

Commit Message

Ross Philipson May 31, 2024, 1:03 a.m. UTC 
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

For better or worse, Secure Launch needs SHA-1 and SHA-256. The
choice of hashes used lie with the platform firmware, not with
software, and is often outside of the users control.

Even if we'd prefer to use SHA-256-only, if firmware elected to start us
with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
to safely use SHA-256 for everything else.

The SHA-1 code here has its origins in the code from the main kernel:

commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")

A modified version of this code was introduced to the lib/crypto/sha1.c
to bring it in line with the SHA-256 code and allow it to be pulled into the
setup kernel in the same manner as SHA-256 is.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
---
 arch/x86/boot/compressed/Makefile     |  2 +
 arch/x86/boot/compressed/early_sha1.c | 12 ++++
 include/crypto/sha1.h                 |  1 +
 lib/crypto/sha1.c                     | 81 +++++++++++++++++++++++++++
 4 files changed, 96 insertions(+)
 create mode 100644 arch/x86/boot/compressed/early_sha1.c

Comments

Eric W. Biederman May 31, 2024, 1:54 p.m. UTC | #1 
Eric Biggers <ebiggers@kernel.org> writes:

> On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>> 
>> For better or worse, Secure Launch needs SHA-1 and SHA-256. The
>> choice of hashes used lie with the platform firmware, not with
>> software, and is often outside of the users control.
>> 
>> Even if we'd prefer to use SHA-256-only, if firmware elected to start us
>> with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
>> the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
>> to safely use SHA-256 for everything else.
>> 
>> The SHA-1 code here has its origins in the code from the main kernel:
>> 
>> commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")
>> 
>> A modified version of this code was introduced to the lib/crypto/sha1.c
>> to bring it in line with the SHA-256 code and allow it to be pulled into the
>> setup kernel in the same manner as SHA-256 is.
>> 
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>
> Thanks.  This explanation doesn't seem to have made it into the actual code or
> documentation.  Can you please get it into a more permanent location?
>
> Also, can you point to where the "deliberately cap the SHA-1 PCRs" thing happens
> in the code?
>
> 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?

No offense to your Trenchboot project but my gut says that anything new
relying on SHA-1 is probably a bad joke at this point.

Firmware can most definitely be upgraded and if the goal is a more
secure boot the usual backwards compatibility concerns for supporting
old firmware really don't apply.

Perhaps hide all of the SHA-1 stuff behind CONFIG_TRENCHBOOT_PROTOTYPE
or something like that to make it clear that SHA-1 is not appropriate
for any kind of production deployment and is only good for prototyping
your implementation until properly implemented firmware comes along.

Eric
Ross Philipson May 31, 2024, 4:18 p.m. UTC | #2 
On 5/30/24 7:16 PM, Eric Biggers wrote:
> On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> For better or worse, Secure Launch needs SHA-1 and SHA-256. The
>> choice of hashes used lie with the platform firmware, not with
>> software, and is often outside of the users control.
>>
>> Even if we'd prefer to use SHA-256-only, if firmware elected to start us
>> with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
>> the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
>> to safely use SHA-256 for everything else.
>>
>> The SHA-1 code here has its origins in the code from the main kernel:
>>
>> commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")
>>
>> A modified version of this code was introduced to the lib/crypto/sha1.c
>> to bring it in line with the SHA-256 code and allow it to be pulled into the
>> setup kernel in the same manner as SHA-256 is.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> 
> Thanks.  This explanation doesn't seem to have made it into the actual code or
> documentation.  Can you please get it into a more permanent location?
> 
> Also, can you point to where the "deliberately cap the SHA-1 PCRs" thing happens
> in the code?
> 
> 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 those are fair points. We will address them and indicate we prefer 
SHA-256 or better.

> 
>> /*
>>   * An implementation of SHA-1's compression function.  Don't use in new code!
>>   * You shouldn't be using SHA-1, and even if you *have* to use SHA-1, this isn't
>>   * the correct way to hash something with SHA-1 (use crypto_shash instead).
>>   */
>> #define SHA1_DIGEST_WORDS	(SHA1_DIGEST_SIZE / 4)
>> #define SHA1_WORKSPACE_WORDS	16
>> void sha1_init(__u32 *buf);
>> void sha1_transform(__u32 *digest, const char *data, __u32 *W);
>> +void sha1(const u8 *data, unsigned int len, u8 *out);
>  > Also, the comment above needs to be updated.

Ack, will address.

Thank you

> 
> - Eric
Jarkko Sakkinen June 4, 2024, 6:52 p.m. UTC | #3 
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> For better or worse, Secure Launch needs SHA-1 and SHA-256. The
> choice of hashes used lie with the platform firmware, not with
> software, and is often outside of the users control.
>
> Even if we'd prefer to use SHA-256-only, if firmware elected to start us
> with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
> the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
> to safely use SHA-256 for everything else.
>
> The SHA-1 code here has its origins in the code from the main kernel:
>
> commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")
>
> A modified version of this code was introduced to the lib/crypto/sha1.c
> to bring it in line with the SHA-256 code and allow it to be pulled into the
> setup kernel in the same manner as SHA-256 is.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> ---
>  arch/x86/boot/compressed/Makefile     |  2 +
>  arch/x86/boot/compressed/early_sha1.c | 12 ++++
>  include/crypto/sha1.h                 |  1 +
>  lib/crypto/sha1.c                     | 81 +++++++++++++++++++++++++++
>  4 files changed, 96 insertions(+)
>  create mode 100644 arch/x86/boot/compressed/early_sha1.c
>
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index e9522c6893be..3307ebef4e1b 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -118,6 +118,8 @@ vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o
>  vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_mixed.o
>  vmlinux-objs-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
>  
> +vmlinux-objs-$(CONFIG_SECURE_LAUNCH) += $(obj)/early_sha1.o
> +
>  $(obj)/vmlinux: $(vmlinux-objs-y) FORCE
>  	$(call if_changed,ld)
>  
> diff --git a/arch/x86/boot/compressed/early_sha1.c b/arch/x86/boot/compressed/early_sha1.c
> new file mode 100644
> index 000000000000..8a9b904a73ab
> --- /dev/null
> +++ b/arch/x86/boot/compressed/early_sha1.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 Apertus Solutions, LLC.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/linkage.h>
> +#include <linux/string.h>
> +#include <asm/boot.h>
> +#include <asm/unaligned.h>
> +
> +#include "../../../../lib/crypto/sha1.c"
}

Yep, make sense. Thinking only that should this be just sha1.c.

Comparing this to mainly drivers/firmware/efi/tpm.c, which is not
early_tpm.c where the early actually probably would make more sense
than here. Here sha1 primitive is just needed.

This is definitely a nitpick but why carry a prefix that is not
that useful, right?

> diff --git a/include/crypto/sha1.h b/include/crypto/sha1.h
> index 044ecea60ac8..d715dd5332e1 100644
> --- a/include/crypto/sha1.h
> +++ b/include/crypto/sha1.h
> @@ -42,5 +42,6 @@ extern int crypto_sha1_finup(struct shash_desc *desc, const u8 *data,
>  #define SHA1_WORKSPACE_WORDS	16
>  void sha1_init(__u32 *buf);
>  void sha1_transform(__u32 *digest, const char *data, __u32 *W);
> +void sha1(const u8 *data, unsigned int len, u8 *out);
>  
>  #endif /* _CRYPTO_SHA1_H */
> diff --git a/lib/crypto/sha1.c b/lib/crypto/sha1.c
> index 1aebe7be9401..10152125b338 100644
> --- a/lib/crypto/sha1.c
> +++ b/lib/crypto/sha1.c
> @@ -137,4 +137,85 @@ void sha1_init(__u32 *buf)
>  }
>  EXPORT_SYMBOL(sha1_init);
>  
> +static void __sha1_transform(u32 *digest, const char *data)
> +{
> +       u32 ws[SHA1_WORKSPACE_WORDS];
> +
> +       sha1_transform(digest, data, ws);
> +
> +       memzero_explicit(ws, sizeof(ws));

For the sake of future reference I'd carry always some inline comment
with any memzero_explicit() call site.

> +}
> +
> +static void sha1_update(struct sha1_state *sctx, const u8 *data, unsigned int len)
> +{
> +	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
> +
> +	sctx->count += len;
> +
> +	if (likely((partial + len) >= SHA1_BLOCK_SIZE)) {


	if (unlikely((partial + len) < SHA1_BLOCK_SIZE))
		goto out;

?

> +		int blocks;
> +
> +		if (partial) {
> +			int p = SHA1_BLOCK_SIZE - partial;
> +
> +			memcpy(sctx->buffer + partial, data, p);
> +			data += p;
> +			len -= p;
> +
> +			__sha1_transform(sctx->state, sctx->buffer);
> +		}
> +
> +		blocks = len / SHA1_BLOCK_SIZE;
> +		len %= SHA1_BLOCK_SIZE;
> +
> +		if (blocks) {
> +			while (blocks--) {
> +				__sha1_transform(sctx->state, data);
> +				data += SHA1_BLOCK_SIZE;
> +			}
> +		}
> +		partial = 0;
> +	}
> +

out:

> +	if (len)
> +		memcpy(sctx->buffer + partial, data, len);

Why not just memcpy() unconditionally?

> +}
> +
> +static void sha1_final(struct sha1_state *sctx, u8 *out)
> +{
> +	const int bit_offset = SHA1_BLOCK_SIZE - sizeof(__be64);
> +	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
> +	__be64 *bits = (__be64 *)(sctx->buffer + bit_offset);
> +	__be32 *digest = (__be32 *)out;
> +	int i;
> +
> +	sctx->buffer[partial++] = 0x80;
> +	if (partial > bit_offset) {
> +		memset(sctx->buffer + partial, 0x0, SHA1_BLOCK_SIZE - partial);
> +		partial = 0;
> +
> +		__sha1_transform(sctx->state, sctx->buffer);
> +	}
> +
> +	memset(sctx->buffer + partial, 0x0, bit_offset - partial);
> +	*bits = cpu_to_be64(sctx->count << 3);
> +	__sha1_transform(sctx->state, sctx->buffer);
> +
> +	for (i = 0; i < SHA1_DIGEST_SIZE / sizeof(__be32); i++)
> +		put_unaligned_be32(sctx->state[i], digest++);
> +
> +	*sctx = (struct sha1_state){};
> +}
> +
> +void sha1(const u8 *data, unsigned int len, u8 *out)
> +{
> +	struct sha1_state sctx = {0};
> +
> +	sha1_init(sctx.state);
> +	sctx.count = 0;

Hmm... so shouldn't C99 take care of this given the initialization
above? I'm not 100% sure here. I.e. given "= {0}", shouldn't this
already be zero?

> +	sha1_update(&sctx, data, len);
> +	sha1_final(&sctx, out);
> +}
> +EXPORT_SYMBOL(sha1);
> +
>  MODULE_LICENSE("GPL");

BR, Jarkko
Ross Philipson June 4, 2024, 9:02 p.m. UTC | #4 
On 6/4/24 11:52 AM, Jarkko Sakkinen wrote:
> On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> For better or worse, Secure Launch needs SHA-1 and SHA-256. The
>> choice of hashes used lie with the platform firmware, not with
>> software, and is often outside of the users control.
>>
>> Even if we'd prefer to use SHA-256-only, if firmware elected to start us
>> with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
>> the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
>> to safely use SHA-256 for everything else.
>>
>> The SHA-1 code here has its origins in the code from the main kernel:
>>
>> commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")
>>
>> A modified version of this code was introduced to the lib/crypto/sha1.c
>> to bring it in line with the SHA-256 code and allow it to be pulled into the
>> setup kernel in the same manner as SHA-256 is.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>> ---
>>   arch/x86/boot/compressed/Makefile     |  2 +
>>   arch/x86/boot/compressed/early_sha1.c | 12 ++++
>>   include/crypto/sha1.h                 |  1 +
>>   lib/crypto/sha1.c                     | 81 +++++++++++++++++++++++++++
>>   4 files changed, 96 insertions(+)
>>   create mode 100644 arch/x86/boot/compressed/early_sha1.c
>>
>> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
>> index e9522c6893be..3307ebef4e1b 100644
>> --- a/arch/x86/boot/compressed/Makefile
>> +++ b/arch/x86/boot/compressed/Makefile
>> @@ -118,6 +118,8 @@ vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o
>>   vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_mixed.o
>>   vmlinux-objs-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
>>   
>> +vmlinux-objs-$(CONFIG_SECURE_LAUNCH) += $(obj)/early_sha1.o
>> +
>>   $(obj)/vmlinux: $(vmlinux-objs-y) FORCE
>>   	$(call if_changed,ld)
>>   
>> diff --git a/arch/x86/boot/compressed/early_sha1.c b/arch/x86/boot/compressed/early_sha1.c
>> new file mode 100644
>> index 000000000000..8a9b904a73ab
>> --- /dev/null
>> +++ b/arch/x86/boot/compressed/early_sha1.c
>> @@ -0,0 +1,12 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2024 Apertus Solutions, LLC.
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/linkage.h>
>> +#include <linux/string.h>
>> +#include <asm/boot.h>
>> +#include <asm/unaligned.h>
>> +
>> +#include "../../../../lib/crypto/sha1.c"
> }
> 
> Yep, make sense. Thinking only that should this be just sha1.c.
> 
> Comparing this to mainly drivers/firmware/efi/tpm.c, which is not
> early_tpm.c where the early actually probably would make more sense
> than here. Here sha1 primitive is just needed.
> 
> This is definitely a nitpick but why carry a prefix that is not
> that useful, right?

I am not 100% sure what you mean here, sorry. Could you clarify about 
the prefix? Do you mean why did we choose early_*? There was precedent 
for doing that like early_serial_console.c.

> 
>> diff --git a/include/crypto/sha1.h b/include/crypto/sha1.h
>> index 044ecea60ac8..d715dd5332e1 100644
>> --- a/include/crypto/sha1.h
>> +++ b/include/crypto/sha1.h
>> @@ -42,5 +42,6 @@ extern int crypto_sha1_finup(struct shash_desc *desc, const u8 *data,
>>   #define SHA1_WORKSPACE_WORDS	16
>>   void sha1_init(__u32 *buf);
>>   void sha1_transform(__u32 *digest, const char *data, __u32 *W);
>> +void sha1(const u8 *data, unsigned int len, u8 *out);
>>   
>>   #endif /* _CRYPTO_SHA1_H */
>> diff --git a/lib/crypto/sha1.c b/lib/crypto/sha1.c
>> index 1aebe7be9401..10152125b338 100644
>> --- a/lib/crypto/sha1.c
>> +++ b/lib/crypto/sha1.c
>> @@ -137,4 +137,85 @@ void sha1_init(__u32 *buf)
>>   }
>>   EXPORT_SYMBOL(sha1_init);
>>   
>> +static void __sha1_transform(u32 *digest, const char *data)
>> +{
>> +       u32 ws[SHA1_WORKSPACE_WORDS];
>> +
>> +       sha1_transform(digest, data, ws);
>> +
>> +       memzero_explicit(ws, sizeof(ws));
> 
> For the sake of future reference I'd carry always some inline comment
> with any memzero_explicit() call site.

We can do that.

> 
>> +}
>> +
>> +static void sha1_update(struct sha1_state *sctx, const u8 *data, unsigned int len)
>> +{
>> +	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
>> +
>> +	sctx->count += len;
>> +
>> +	if (likely((partial + len) >= SHA1_BLOCK_SIZE)) {
> 
> 
> 	if (unlikely((partial + len) < SHA1_BLOCK_SIZE))
> 		goto out;
> 
> ?

We could do it that way. I guess it would cut down in indenting. I defer 
to Daniel Smith on this...

> 
>> +		int blocks;
>> +
>> +		if (partial) {
>> +			int p = SHA1_BLOCK_SIZE - partial;
>> +
>> +			memcpy(sctx->buffer + partial, data, p);
>> +			data += p;
>> +			len -= p;
>> +
>> +			__sha1_transform(sctx->state, sctx->buffer);
>> +		}
>> +
>> +		blocks = len / SHA1_BLOCK_SIZE;
>> +		len %= SHA1_BLOCK_SIZE;
>> +
>> +		if (blocks) {
>> +			while (blocks--) {
>> +				__sha1_transform(sctx->state, data);
>> +				data += SHA1_BLOCK_SIZE;
>> +			}
>> +		}
>> +		partial = 0;
>> +	}
>> +
> 
> out:
> 
>> +	if (len)
>> +		memcpy(sctx->buffer + partial, data, len);
> 
> Why not just memcpy() unconditionally?
> 

... and this.

>> +}
>> +
>> +static void sha1_final(struct sha1_state *sctx, u8 *out)
>> +{
>> +	const int bit_offset = SHA1_BLOCK_SIZE - sizeof(__be64);
>> +	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
>> +	__be64 *bits = (__be64 *)(sctx->buffer + bit_offset);
>> +	__be32 *digest = (__be32 *)out;
>> +	int i;
>> +
>> +	sctx->buffer[partial++] = 0x80;
>> +	if (partial > bit_offset) {
>> +		memset(sctx->buffer + partial, 0x0, SHA1_BLOCK_SIZE - partial);
>> +		partial = 0;
>> +
>> +		__sha1_transform(sctx->state, sctx->buffer);
>> +	}
>> +
>> +	memset(sctx->buffer + partial, 0x0, bit_offset - partial);
>> +	*bits = cpu_to_be64(sctx->count << 3);
>> +	__sha1_transform(sctx->state, sctx->buffer);
>> +
>> +	for (i = 0; i < SHA1_DIGEST_SIZE / sizeof(__be32); i++)
>> +		put_unaligned_be32(sctx->state[i], digest++);
>> +
>> +	*sctx = (struct sha1_state){};
>> +}
>> +
>> +void sha1(const u8 *data, unsigned int len, u8 *out)
>> +{
>> +	struct sha1_state sctx = {0};
>> +
>> +	sha1_init(sctx.state);
>> +	sctx.count = 0;
> 
> Hmm... so shouldn't C99 take care of this given the initialization
> above? I'm not 100% sure here. I.e. given "= {0}", shouldn't this
> already be zero?

Yes it seems so. We will look at changing that.

> 
>> +	sha1_update(&sctx, data, len);
>> +	sha1_final(&sctx, out);
>> +}
>> +EXPORT_SYMBOL(sha1);
>> +
>>   MODULE_LICENSE("GPL");
> 
> BR, Jarkko

Thanks
Ross
Jarkko Sakkinen June 4, 2024, 10:40 p.m. UTC | #5 
On Wed Jun 5, 2024 at 12:02 AM EEST,  wrote:
> On 6/4/24 11:52 AM, Jarkko Sakkinen wrote:
> > On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> >>
> >> For better or worse, Secure Launch needs SHA-1 and SHA-256. The
> >> choice of hashes used lie with the platform firmware, not with
> >> software, and is often outside of the users control.
> >>
> >> Even if we'd prefer to use SHA-256-only, if firmware elected to start us
> >> with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
> >> the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
> >> to safely use SHA-256 for everything else.
> >>
> >> The SHA-1 code here has its origins in the code from the main kernel:
> >>
> >> commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")
> >>
> >> A modified version of this code was introduced to the lib/crypto/sha1.c
> >> to bring it in line with the SHA-256 code and allow it to be pulled into the
> >> setup kernel in the same manner as SHA-256 is.
> >>
> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> >> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> >> ---
> >>   arch/x86/boot/compressed/Makefile     |  2 +
> >>   arch/x86/boot/compressed/early_sha1.c | 12 ++++
> >>   include/crypto/sha1.h                 |  1 +
> >>   lib/crypto/sha1.c                     | 81 +++++++++++++++++++++++++++
> >>   4 files changed, 96 insertions(+)
> >>   create mode 100644 arch/x86/boot/compressed/early_sha1.c
> >>
> >> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> >> index e9522c6893be..3307ebef4e1b 100644
> >> --- a/arch/x86/boot/compressed/Makefile
> >> +++ b/arch/x86/boot/compressed/Makefile
> >> @@ -118,6 +118,8 @@ vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o
> >>   vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_mixed.o
> >>   vmlinux-objs-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
> >>   
> >> +vmlinux-objs-$(CONFIG_SECURE_LAUNCH) += $(obj)/early_sha1.o
> >> +
> >>   $(obj)/vmlinux: $(vmlinux-objs-y) FORCE
> >>   	$(call if_changed,ld)
> >>   
> >> diff --git a/arch/x86/boot/compressed/early_sha1.c b/arch/x86/boot/compressed/early_sha1.c
> >> new file mode 100644
> >> index 000000000000..8a9b904a73ab
> >> --- /dev/null
> >> +++ b/arch/x86/boot/compressed/early_sha1.c
> >> @@ -0,0 +1,12 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) 2024 Apertus Solutions, LLC.
> >> + */
> >> +
> >> +#include <linux/init.h>
> >> +#include <linux/linkage.h>
> >> +#include <linux/string.h>
> >> +#include <asm/boot.h>
> >> +#include <asm/unaligned.h>
> >> +
> >> +#include "../../../../lib/crypto/sha1.c"
> > }
> > 
> > Yep, make sense. Thinking only that should this be just sha1.c.
> > 
> > Comparing this to mainly drivers/firmware/efi/tpm.c, which is not
> > early_tpm.c where the early actually probably would make more sense
> > than here. Here sha1 primitive is just needed.
> > 
> > This is definitely a nitpick but why carry a prefix that is not
> > that useful, right?
>
> I am not 100% sure what you mean here, sorry. Could you clarify about 
> the prefix? Do you mean why did we choose early_*? There was precedent 
> for doing that like early_serial_console.c.

Yep, that exactly. I'd just name as sha1.c.

>
> > 
> >> diff --git a/include/crypto/sha1.h b/include/crypto/sha1.h
> >> index 044ecea60ac8..d715dd5332e1 100644
> >> --- a/include/crypto/sha1.h
> >> +++ b/include/crypto/sha1.h
> >> @@ -42,5 +42,6 @@ extern int crypto_sha1_finup(struct shash_desc *desc, const u8 *data,
> >>   #define SHA1_WORKSPACE_WORDS	16
> >>   void sha1_init(__u32 *buf);
> >>   void sha1_transform(__u32 *digest, const char *data, __u32 *W);
> >> +void sha1(const u8 *data, unsigned int len, u8 *out);
> >>   
> >>   #endif /* _CRYPTO_SHA1_H */
> >> diff --git a/lib/crypto/sha1.c b/lib/crypto/sha1.c
> >> index 1aebe7be9401..10152125b338 100644
> >> --- a/lib/crypto/sha1.c
> >> +++ b/lib/crypto/sha1.c
> >> @@ -137,4 +137,85 @@ void sha1_init(__u32 *buf)
> >>   }
> >>   EXPORT_SYMBOL(sha1_init);
> >>   
> >> +static void __sha1_transform(u32 *digest, const char *data)
> >> +{
> >> +       u32 ws[SHA1_WORKSPACE_WORDS];
> >> +
> >> +       sha1_transform(digest, data, ws);
> >> +
> >> +       memzero_explicit(ws, sizeof(ws));
> > 
> > For the sake of future reference I'd carry always some inline comment
> > with any memzero_explicit() call site.
>
> We can do that.
>
> > 
> >> +}
> >> +
> >> +static void sha1_update(struct sha1_state *sctx, const u8 *data, unsigned int len)
> >> +{
> >> +	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
> >> +
> >> +	sctx->count += len;
> >> +
> >> +	if (likely((partial + len) >= SHA1_BLOCK_SIZE)) {
> > 
> > 
> > 	if (unlikely((partial + len) < SHA1_BLOCK_SIZE))
> > 		goto out;
> > 
> > ?
>
> We could do it that way. I guess it would cut down in indenting. I defer 
> to Daniel Smith on this...

Yep, that's why I requested this.

>
> > 
> >> +		int blocks;
> >> +
> >> +		if (partial) {
> >> +			int p = SHA1_BLOCK_SIZE - partial;
> >> +
> >> +			memcpy(sctx->buffer + partial, data, p);
> >> +			data += p;
> >> +			len -= p;
> >> +
> >> +			__sha1_transform(sctx->state, sctx->buffer);
> >> +		}
> >> +
> >> +		blocks = len / SHA1_BLOCK_SIZE;
> >> +		len %= SHA1_BLOCK_SIZE;
> >> +
> >> +		if (blocks) {
> >> +			while (blocks--) {
> >> +				__sha1_transform(sctx->state, data);
> >> +				data += SHA1_BLOCK_SIZE;
> >> +			}
> >> +		}
> >> +		partial = 0;
> >> +	}
> >> +
> > 
> > out:
> > 
> >> +	if (len)
> >> +		memcpy(sctx->buffer + partial, data, len);
> > 
> > Why not just memcpy() unconditionally?
> > 
>
> ... and this.

It only adds complexity with no gain.

>
> >> +}
> >> +
> >> +static void sha1_final(struct sha1_state *sctx, u8 *out)
> >> +{
> >> +	const int bit_offset = SHA1_BLOCK_SIZE - sizeof(__be64);
> >> +	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
> >> +	__be64 *bits = (__be64 *)(sctx->buffer + bit_offset);
> >> +	__be32 *digest = (__be32 *)out;
> >> +	int i;
> >> +
> >> +	sctx->buffer[partial++] = 0x80;
> >> +	if (partial > bit_offset) {
> >> +		memset(sctx->buffer + partial, 0x0, SHA1_BLOCK_SIZE - partial);
> >> +		partial = 0;
> >> +
> >> +		__sha1_transform(sctx->state, sctx->buffer);
> >> +	}
> >> +
> >> +	memset(sctx->buffer + partial, 0x0, bit_offset - partial);
> >> +	*bits = cpu_to_be64(sctx->count << 3);
> >> +	__sha1_transform(sctx->state, sctx->buffer);
> >> +
> >> +	for (i = 0; i < SHA1_DIGEST_SIZE / sizeof(__be32); i++)
> >> +		put_unaligned_be32(sctx->state[i], digest++);
> >> +
> >> +	*sctx = (struct sha1_state){};
> >> +}
> >> +
> >> +void sha1(const u8 *data, unsigned int len, u8 *out)
> >> +{
> >> +	struct sha1_state sctx = {0};
> >> +
> >> +	sha1_init(sctx.state);
> >> +	sctx.count = 0;
> > 
> > Hmm... so shouldn't C99 take care of this given the initialization
> > above? I'm not 100% sure here. I.e. given "= {0}", shouldn't this
> > already be zero?
>
> Yes it seems so. We will look at changing that.

Yeah, AFAIK C99 should zero out anything remaining.

>
> > 
> >> +	sha1_update(&sctx, data, len);
> >> +	sha1_final(&sctx, out);
> >> +}
> >> +EXPORT_SYMBOL(sha1);
> >> +
> >>   MODULE_LICENSE("GPL");
> > 
> > BR, Jarkko
>
> Thanks
> Ross

BR, Jarkko
Daniel P. Smith Aug. 15, 2024, 5:38 p.m. UTC | #6 
On 5/31/24 09:54, Eric W. Biederman wrote:
> Eric Biggers <ebiggers@kernel.org> writes:
> 
>> On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote:
>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>>
>>> For better or worse, Secure Launch needs SHA-1 and SHA-256. The
>>> choice of hashes used lie with the platform firmware, not with
>>> software, and is often outside of the users control.
>>>
>>> Even if we'd prefer to use SHA-256-only, if firmware elected to start us
>>> with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
>>> the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
>>> to safely use SHA-256 for everything else.
>>>
>>> The SHA-1 code here has its origins in the code from the main kernel:
>>>
>>> commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")
>>>
>>> A modified version of this code was introduced to the lib/crypto/sha1.c
>>> to bring it in line with the SHA-256 code and allow it to be pulled into the
>>> setup kernel in the same manner as SHA-256 is.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>>
>> Thanks.  This explanation doesn't seem to have made it into the actual code or
>> documentation.  Can you please get it into a more permanent location?
>>
>> Also, can you point to where the "deliberately cap the SHA-1 PCRs" thing happens
>> in the code?
>>
>> 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.

v/r,
dps
Thomas Gleixner Aug. 15, 2024, 7:10 p.m. UTC | #7 
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@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.

Thanks,

        tglx
Jarkko Sakkinen Aug. 16, 2024, 10:42 a.m. UTC | #8 
On Thu Aug 15, 2024 at 10:10 PM EEST, Thomas Gleixner 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@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.

Git also has a bit more wide than secure launch, and the timeline is
also completely different. Git maintains legacy, while has also
introduced SHA-256 support in 2018. This as a new feature in the kernel
stack.

The purpose of SHA1-DC has obviously been to extend the lifespan, not
fix SHA-1.

Linux will be better of not adding anything new related to SHA-1 or
TPM 1.2. They still have a maintenance cost and I think that time
would be better spent of for almost anything else (starting from
taking your trashes out or boiling coffee) ;-)

>
> 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.
>
> Thanks,
>
>         tglx

BR, Jarkko
Andrew Cooper Aug. 16, 2024, 11:01 a.m. UTC | #9 
On 15/08/2024 8:10 pm, Thomas Gleixner 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@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?

No.

The use of SHA-1 is necessary even on modern systems to avoid a
vulnerability.

It is the platform, not Linux, which decides which TPM PCR banks are active.

Linux *must* have an algorithm for every active bank (which is the
platform's choice), even if the single thing it intends to do is cap the
bank and use better ones.

Capping a bank requires updating the TPM Log without corrupting it,
which requires a hash calculation of the correct type for the bank.

~Andrew
Jarkko Sakkinen Aug. 16, 2024, 11:22 a.m. UTC | #10 
On Fri Aug 16, 2024 at 2:01 PM EEST, Andrew Cooper wrote:
> On 15/08/2024 8:10 pm, Thomas Gleixner 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@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?
>
> No.
>
> The use of SHA-1 is necessary even on modern systems to avoid a
> vulnerability.
>
> It is the platform, not Linux, which decides which TPM PCR banks are active.
>
> Linux *must* have an algorithm for every active bank (which is the
> platform's choice), even if the single thing it intends to do is cap the
> bank and use better ones.

For (any) non-legacy features we can choose, which choices we choose to
support, and which we do not. This is not an oppositive view just saying
how it is, and platforms set of choices is not a selling argument.

>
> Capping a bank requires updating the TPM Log without corrupting it,
> which requires a hash calculation of the correct type for the bank.
>
> ~Andrew

BR, Jarkko
Matthew Garrett Aug. 16, 2024, 6:41 p.m. UTC | #11 
On Fri, Aug 16, 2024 at 02:22:04PM +0300, Jarkko Sakkinen wrote:

> For (any) non-legacy features we can choose, which choices we choose to
> support, and which we do not. This is not an oppositive view just saying
> how it is, and platforms set of choices is not a selling argument.

NIST still permits the use of SHA-1 until 2030, and the most significant 
demonstrated weaknesses in it don't seem applicable to the use case 
here. We certainly shouldn't encourage any new uses of it, and anyone 
who's able to use SHA-2 should be doing that instead, but it feels like 
people are arguing about not supporting hardware that exists in the real 
world for vibes reasons rather than it being a realistically attackable 
weakness (and if we really *are* that concerned about SHA-1, why are we 
still supporting TPM 1.2 at all?)
Jarkko Sakkinen Aug. 19, 2024, 6:05 p.m. UTC | #12 
On Fri Aug 16, 2024 at 9:41 PM EEST, Matthew Garrett wrote:
> On Fri, Aug 16, 2024 at 02:22:04PM +0300, Jarkko Sakkinen wrote:
>
> > For (any) non-legacy features we can choose, which choices we choose to
> > support, and which we do not. This is not an oppositive view just saying
> > how it is, and platforms set of choices is not a selling argument.
>
> NIST still permits the use of SHA-1 until 2030, and the most significant 
> demonstrated weaknesses in it don't seem applicable to the use case 
> here. We certainly shouldn't encourage any new uses of it, and anyone 
> who's able to use SHA-2 should be doing that instead, but it feels like 
> people are arguing about not supporting hardware that exists in the real 
> world for vibes reasons rather than it being a realistically attackable 
> weakness (and if we really *are* that concerned about SHA-1, why are we 
> still supporting TPM 1.2 at all?)

We are life-supporting TPM 1.2 as long as necessary but neither the
support is extended nor new features will gain TPM 1.2 support. So
that is at least my policy for that feature.

BR, Jarkko
Matthew Garrett Aug. 19, 2024, 6:24 p.m. UTC | #13 
On Mon, Aug 19, 2024 at 09:05:47PM +0300, Jarkko Sakkinen wrote:
> On Fri Aug 16, 2024 at 9:41 PM EEST, Matthew Garrett wrote:
> > On Fri, Aug 16, 2024 at 02:22:04PM +0300, Jarkko Sakkinen wrote:
> >
> > > For (any) non-legacy features we can choose, which choices we choose to
> > > support, and which we do not. This is not an oppositive view just saying
> > > how it is, and platforms set of choices is not a selling argument.
> >
> > NIST still permits the use of SHA-1 until 2030, and the most significant 
> > demonstrated weaknesses in it don't seem applicable to the use case 
> > here. We certainly shouldn't encourage any new uses of it, and anyone 
> > who's able to use SHA-2 should be doing that instead, but it feels like 
> > people are arguing about not supporting hardware that exists in the real 
> > world for vibes reasons rather than it being a realistically attackable 
> > weakness (and if we really *are* that concerned about SHA-1, why are we 
> > still supporting TPM 1.2 at all?)
> 
> We are life-supporting TPM 1.2 as long as necessary but neither the
> support is extended nor new features will gain TPM 1.2 support. So
> that is at least my policy for that feature.

But the fact that we support it and provide no warning labels is a 
pretty clear indication that we're not actively trying to prevent people 
from using SHA-1 in the general case. Why is this a different case? 
Failing to support it actually opens an entire separate set of footgun 
opportunities in terms of the SHA-1 banks now being out of sync with the 
SHA-2 ones, so either way we're leaving people open to making poor 
choices.
Jarkko Sakkinen Aug. 20, 2024, 3:26 p.m. UTC | #14 
On Mon Aug 19, 2024 at 9:24 PM EEST, Matthew Garrett wrote:
> On Mon, Aug 19, 2024 at 09:05:47PM +0300, Jarkko Sakkinen wrote:
> > On Fri Aug 16, 2024 at 9:41 PM EEST, Matthew Garrett wrote:
> > > On Fri, Aug 16, 2024 at 02:22:04PM +0300, Jarkko Sakkinen wrote:
> > >
> > > > For (any) non-legacy features we can choose, which choices we choose to
> > > > support, and which we do not. This is not an oppositive view just saying
> > > > how it is, and platforms set of choices is not a selling argument.
> > >
> > > NIST still permits the use of SHA-1 until 2030, and the most significant 
> > > demonstrated weaknesses in it don't seem applicable to the use case 
> > > here. We certainly shouldn't encourage any new uses of it, and anyone 
> > > who's able to use SHA-2 should be doing that instead, but it feels like 
> > > people are arguing about not supporting hardware that exists in the real 
> > > world for vibes reasons rather than it being a realistically attackable 
> > > weakness (and if we really *are* that concerned about SHA-1, why are we 
> > > still supporting TPM 1.2 at all?)
> > 
> > We are life-supporting TPM 1.2 as long as necessary but neither the
> > support is extended nor new features will gain TPM 1.2 support. So
> > that is at least my policy for that feature.
>
> But the fact that we support it and provide no warning labels is a 
> pretty clear indication that we're not actively trying to prevent people 
> from using SHA-1 in the general case. Why is this a different case? 
> Failing to support it actually opens an entire separate set of footgun 
> opportunities in terms of the SHA-1 banks now being out of sync with the 
> SHA-2 ones, so either way we're leaving people open to making poor 
> choices.

This is a fair and enclosing argument. I get where you are coming from
now. Please as material for the commit message.

BR, Jarkko
Daniel P. Smith Aug. 22, 2024, 6:29 p.m. UTC | #15 
On 8/15/24 15:10, Thomas Gleixner 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@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.
> 
> Thanks,
> 
>          tglx
> 

There is one simple question, does allowing the Secure Launch code to 
record SHA1 measurements make the system insecure, and the answer is 
absolutely not.

The role of the Secure Launch code base in the context of the larger 
launch process is to function as observer. Within this role, its only 
responsibility is continuing the trust chain(s) that were started by the 
CPU/Hardware. It does so by measuring the components and configuration 
it is responsible for loading and applying, i.e. in TCG parlance, it is 
continuing the construction of the transitive trust for the system. In 
this aspect, the only degradation of security that can affect the 
kernel's role is whether all the necessary entities are safely measured 
and not what algorithms are used.

If the system integrator, whether that be the OEM, your employer, the 
distro maintainer, the system administrator, or the end user, configures 
the DL preamble to only use SHA1 or used older hardware that has a 
TPM1.2, then they are accepting the risk it creates in their solution. 
In fact, a greater threat to the security of the launch is the 
misconfiguration of the IOMMU, which risks the kernel's ability to 
safely make measurements, as compared to the use of SHA1. Yet it was 
insisted in past reviews that we allow the user to specify an incorrect 
IOMMU policy.

In the end, the "security" of an RTM solution is how and what 
measurements are used to assess the health of a system. Thus bringing it 
back to the opening question, if SHA1 measurements are made but not 
used, i.e. the attestation enforcement only uses SHA2, then it has zero 
impact on the security of the system.

Another fact to consider is that the current Intel's TXT MLE 
specification dictates SHA1 as a valid configuration. Secure Launch's 
use of SHA1 is therefore to comply with Intel's specification for TXT. 
And like the IOMMU situation, having the option available allows the 
user to determine how they ultimately want to integrate Secure Launch 
into their integrity management. And because Secure Launch will only 
attempt SHA1 if it was in the TXT configuration, when either Intel 
removes SHA1 from the MLE specification or firmware manufactures begin 
disabling the SHA1 banks, this will obviously mean that Secure Launch 
will not produce SHA1 measurements.

On a side note, with my remote attestation hat on, the SHA1 measurements 
can in fact be extremely useful. If an attestation was made containing 
both SHA1 and SHA2 chains, and the SHA1 of an event was correct but the 
SHA2 was not, either a natural collision happened or someone maliciously 
caused a collision. The former has an extremely low probability, while 
the latter is highly probable.

Thus, with this information alone, it is possible to make the reasonable 
determination the device is compromised. Whereas if both hashes are 
mismatched, without any additional information it is equally probable of 
either misconfiguration or compromise. And to state the obvious, with 
only SHA2, further information is needed to distinguish between 
misconfiguration and compromise.

V/r,
Daniel P. Smith
Eric Biggers Aug. 27, 2024, 6:14 p.m. UTC | #16 
On Thu, May 30, 2024 at 07:16:56PM -0700, Eric Biggers wrote:
> On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote:
> > From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> > 
> > For better or worse, Secure Launch needs SHA-1 and SHA-256. The
> > choice of hashes used lie with the platform firmware, not with
> > software, and is often outside of the users control.
> > 
> > Even if we'd prefer to use SHA-256-only, if firmware elected to start us
> > with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
> > the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
> > to safely use SHA-256 for everything else.
> > 
> > The SHA-1 code here has its origins in the code from the main kernel:
> > 
> > commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")
> > 
> > A modified version of this code was introduced to the lib/crypto/sha1.c
> > to bring it in line with the SHA-256 code and allow it to be pulled into the
> > setup kernel in the same manner as SHA-256 is.
> > 
> > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> > Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> 
> Thanks.  This explanation doesn't seem to have made it into the actual code or
> documentation.  Can you please get it into a more permanent location?

I see that a new version of the patchset was sent out but this suggestion was
not taken.  Are you planning to address it?

- Eric
Ross Philipson Aug. 28, 2024, 8:14 p.m. UTC | #17 
On 8/27/24 11:14 AM, 'Eric Biggers' via trenchboot-devel wrote:
> On Thu, May 30, 2024 at 07:16:56PM -0700, Eric Biggers wrote:
>> On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote:
>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>>
>>> For better or worse, Secure Launch needs SHA-1 and SHA-256. The
>>> choice of hashes used lie with the platform firmware, not with
>>> software, and is often outside of the users control.
>>>
>>> Even if we'd prefer to use SHA-256-only, if firmware elected to start us
>>> with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
>>> the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
>>> to safely use SHA-256 for everything else.
>>>
>>> The SHA-1 code here has its origins in the code from the main kernel:
>>>
>>> commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")
>>>
>>> A modified version of this code was introduced to the lib/crypto/sha1.c
>>> to bring it in line with the SHA-256 code and allow it to be pulled into the
>>> setup kernel in the same manner as SHA-256 is.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>>
>> Thanks.  This explanation doesn't seem to have made it into the actual code or
>> documentation.  Can you please get it into a more permanent location?
> 
> I see that a new version of the patchset was sent out but this suggestion was
> not taken.  Are you planning to address it?

Sorry we sort of overlooked that part of the request. We will take the 
latest commit message, clean it up a little and put it in 
boot/compressed/sha1.c file as a comment. I believe that is what you 
would like us to do.

Thanks
Ross

> 
> - Eric
>
Eric Biggers Aug. 28, 2024, 11:13 p.m. UTC | #18 
On Wed, Aug 28, 2024 at 01:14:45PM -0700, ross.philipson@oracle.com wrote:
> On 8/27/24 11:14 AM, 'Eric Biggers' via trenchboot-devel wrote:
> > On Thu, May 30, 2024 at 07:16:56PM -0700, Eric Biggers wrote:
> > > On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote:
> > > > From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> > > > 
> > > > For better or worse, Secure Launch needs SHA-1 and SHA-256. The
> > > > choice of hashes used lie with the platform firmware, not with
> > > > software, and is often outside of the users control.
> > > > 
> > > > Even if we'd prefer to use SHA-256-only, if firmware elected to start us
> > > > with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
> > > > the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
> > > > to safely use SHA-256 for everything else.
> > > > 
> > > > The SHA-1 code here has its origins in the code from the main kernel:
> > > > 
> > > > commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")
> > > > 
> > > > A modified version of this code was introduced to the lib/crypto/sha1.c
> > > > to bring it in line with the SHA-256 code and allow it to be pulled into the
> > > > setup kernel in the same manner as SHA-256 is.
> > > > 
> > > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> > > > Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> > > 
> > > Thanks.  This explanation doesn't seem to have made it into the actual code or
> > > documentation.  Can you please get it into a more permanent location?
> > 
> > I see that a new version of the patchset was sent out but this suggestion was
> > not taken.  Are you planning to address it?
> 
> Sorry we sort of overlooked that part of the request. We will take the
> latest commit message, clean it up a little and put it in
> boot/compressed/sha1.c file as a comment. I believe that is what you would
> like us to do.
> 

Do users of this feature need to make a decision about SHA-1?  If so there needs
to be guidance in Documentation/.  A comment in a .c file is not user facing.

- Eric
Andy Lutomirski Aug. 29, 2024, 3:17 a.m. UTC | #19 
On Thu, Aug 15, 2024 at 12:10 PM Thomas Gleixner <tglx@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@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.

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?

Thanks,
Andy
Matthew Garrett Aug. 29, 2024, 3:25 a.m. UTC | #20 
On Wed, Aug 28, 2024 at 08:17:05PM -0700, Andy Lutomirski wrote:

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

Presumably this would be dependent upon non-SHA1 banks being enabled?
Andy Lutomirski Aug. 29, 2024, 5:26 p.m. UTC | #21 
On Wed, Aug 28, 2024 at 8:25 PM Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> On Wed, Aug 28, 2024 at 08:17:05PM -0700, Andy Lutomirski wrote:
>
> > 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?
>
> Presumably this would be dependent upon non-SHA1 banks being enabled?

Of course.  It's also not immediately obvious to me what layer of the
stack should be responsible for capping SHA1 PCRs.  Should it be the
kernel?  Userspace?

It seems like a whole lot of people, for better or for worse, want to
minimize the amount of code that even knows how to compute SHA1
hashes.  I'm not personally convinced I agree with this strategy, but
it is what it is.  And maybe people would be happier if the default
behavior of the kernel is to notice that SHA256 is available and then
cap SHA1 before even asking user code's permission.

--Andy
Daniel P. Smith Sept. 13, 2024, 12:34 a.m. UTC | #22 
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@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@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.

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.

v/r,
dps
Andy Lutomirski Sept. 14, 2024, 3:57 a.m. UTC | #23 
On Thu, Sep 12, 2024 at 5:34 PM Daniel P. Smith
<dpsmith@apertussolutions.com> 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@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@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.
>
> 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 Sept. 21, 2024, 10:40 p.m. UTC | #24 
On Sat, Sep 21, 2024 at 11:37 AM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> On 9/13/24 23:57, Andy Lutomirski wrote:
> > On Thu, Sep 12, 2024 at 5:34 PM Daniel P. Smith
> > <dpsmith@apertussolutions.com> wrote:
> >>

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

...

> I did not see the term actually defined in the client profile, but the
> term "cap" refers to the specific action of hashing a value across a set
> of PCRs. This is to reflect that certain events have occurred and will
> result in a different but predictable change to the PCR value. Often
> times this is to ensure that if there are TPM objects sealed to the
> system with either that event having or have not occurred, they cannot
> be unsealed. Thus, one has "capped" the PCRs as a means to close access
> to the “acceptable” system state.

Okay, so I read Ross's earlier email rather differently:

> Even if we'd prefer to use SHA-256-only, if firmware elected to start us
> with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
> the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
> to safely use SHA-256 for everything else.

I assumed that "deliberately cap" meant that there was an actual
feature where you write something to the event log (if applicable) and
extend the PCR in a special way that *turns that PCR off*.  That is,
it does something such that later-loaded software *can't* use that PCR
to attest or unseal anything, etc.

But it sounds like you're saying that no such feature exists.  And a
quick skim of the specs doesn't come up with anything.  And the SHA1
banks may well be susceptible to a collision attack.

So what are the kernel's choices wrt the SHA-1 PCRs?  It can:

a) Perform business as usual: extend them consistently with the
SHA-256 PCRs.  This is sort of *fine*: the kernel code in question is
not relying on the security of SHA-1, but it is making it possible for
future code to (unwisely) rely on them.  (Although, if the kernel is
loading a trustworthy initramfs, then there won't be a collision, and
there is no known second-preimage attack against SHA-1.)

b) Same as (a), but with countermeasures: do something to the effect
of *detecting* the attack a la SHA1-DC and panic if an attack is
detected.  Maybe this is wise; maybe it's not.

c) Do not extend the SHA-1 PCRs and pretend they don't exist.  This
seems likely to cause massive security problems, and having the kernel
try to defend its behavior by saying "we don't support SHA-1 -- this
is a problem downstream" seems unwise to me.

d) Extend them but in an unconventional way that makes using them
extra secure.  For example, calculate SHA-256(next stage), then extend
with (next stage || "Linux thinks this is better" || SHA-256(next
stage).  This makes the SHA-1 banks usable, and it seems like it will
probably defeat anything resembling a current attack.  But maybe this
is silly.  It would probably require doing the same thing to the
SHA-256 banks for the benefit of any software that checks whether the
SHA-1 and SHA-256 banks are consistent with each other.

e) Actually try to make the SHA-1 PCRs unusable.  For example, extend
them with random numbers.

My inclination is that having some kind of Linux "policy" that SHA-1
is forbidden adds no actual security value.  Option (a) honestly seems
fine.  Nothing in the kernel *relies* on the SHA-1 hash being secure.
But option (b) also seems okay if someone is willing to put the effort
into implementing it and creating a proper test case.

But the description of all this could certainly do a better job of
explaining what's going on.

--Andy

> [1] A future expansion of Secure Launch will be to enable usage of
> Intel's Hardware Shield, link below, to provide runtime trustworthy
> determination of SMM. The full extent of this capability can only be
> achieved under a DRTM launch of the system with Intel TXT. When enabled,
> this can be used to verify the SMM protections are in place and inform
> the kernel's memory management which regions of memory are safe from SMM
> tampering.
>
> https://www.intel.com/content/dam/www/central-libraries/us/en/documents/drtm-based-computing-whitepaper.pdf

Wow.  I skimmed this paper.  What an overcomplicated solution to a
problem that doesn't deserve to exist in the first place.
Daniel P. Smith Nov. 2, 2024, 2:53 p.m. UTC | #25 
Hi Luto,

My apologies, I missed this response and the active on v11 cause me to 
get an inquiry why I hadn't responded.

On 9/21/24 18:40, Andy Lutomirski wrote:
> On Sat, Sep 21, 2024 at 11:37 AM Daniel P. Smith
> <dpsmith@apertussolutions.com> wrote:
>>
>> On 9/13/24 23:57, Andy Lutomirski wrote:
>>> On Thu, Sep 12, 2024 at 5:34 PM Daniel P. Smith
>>> <dpsmith@apertussolutions.com> wrote:
>>>>
> 
>>> 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?
>>
>>
> 
> ...
> 
>> I did not see the term actually defined in the client profile, but the
>> term "cap" refers to the specific action of hashing a value across a set
>> of PCRs. This is to reflect that certain events have occurred and will
>> result in a different but predictable change to the PCR value. Often
>> times this is to ensure that if there are TPM objects sealed to the
>> system with either that event having or have not occurred, they cannot
>> be unsealed. Thus, one has "capped" the PCRs as a means to close access
>> to the “acceptable” system state.
> 
> Okay, so I read Ross's earlier email rather differently:
> 
>> Even if we'd prefer to use SHA-256-only, if firmware elected to start us
>> with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
>> the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
>> to safely use SHA-256 for everything else.
> 
> I assumed that "deliberately cap" meant that there was an actual
> feature where you write something to the event log (if applicable) and
> extend the PCR in a special way that *turns that PCR off*.  That is,
> it does something such that later-loaded software *can't* use that PCR
> to attest or unseal anything, etc.
> 
> But it sounds like you're saying that no such feature exists.  And a
> quick skim of the specs doesn't come up with anything.  And the SHA1
> banks may well be susceptible to a collision attack.

Correct, the only entity that can disable PCR banks is the firmware. 
When it initializes the TPM, it can disable banks/algorithms. After 
that, when an extend operation is done, the TPM is expecting an entry 
for all active PCR banks and the TPM itself does the extend hash that is 
stored into the PCRs.

> So what are the kernel's choices wrt the SHA-1 PCRs?  It can:
> 
> a) Perform business as usual: extend them consistently with the
> SHA-256 PCRs.  This is sort of *fine*: the kernel code in question is
> not relying on the security of SHA-1, but it is making it possible for
> future code to (unwisely) rely on them.  (Although, if the kernel is
> loading a trustworthy initramfs, then there won't be a collision, and
> there is no known second-preimage attack against SHA-1.)
> 
> b) Same as (a), but with countermeasures: do something to the effect
> of *detecting* the attack a la SHA1-DC and panic if an attack is
> detected.  Maybe this is wise; maybe it's not.
> 
> c) Do not extend the SHA-1 PCRs and pretend they don't exist.  This
> seems likely to cause massive security problems, and having the kernel
> try to defend its behavior by saying "we don't support SHA-1 -- this
> is a problem downstream" seems unwise to me.

I will chime in here to say that you can't ignore them, but you can send 
a fixed value, either well-known or junk, as the SHA1 value when doing 
the extend operation as you suggest in (e).

> d) Extend them but in an unconventional way that makes using them
> extra secure.  For example, calculate SHA-256(next stage), then extend
> with (next stage || "Linux thinks this is better" || SHA-256(next
> stage).  This makes the SHA-1 banks usable, and it seems like it will
> probably defeat anything resembling a current attack.  But maybe this
> is silly.  It would probably require doing the same thing to the
> SHA-256 banks for the benefit of any software that checks whether the
> SHA-1 and SHA-256 banks are consistent with each other.
> 
> e) Actually try to make the SHA-1 PCRs unusable.  For example, extend
> them with random numbers.
> 
> My inclination is that having some kind of Linux "policy" that SHA-1
> is forbidden adds no actual security value.  Option (a) honestly seems
> fine.  Nothing in the kernel *relies* on the SHA-1 hash being secure.
> But option (b) also seems okay if someone is willing to put the effort
> into implementing it and creating a proper test case.

Obviously, for the most part, we are in agreement. The one caveat is 
that I don't think the effort to shore-up SHA1 provides a good return on 
the costs it would incur. With no intent to disparage any one person, 
there generally will be two groups that would use SHA1. The first would 
be those limited by their platform and understand the risks. The second 
would be those attempting to do a cryptographic-based security solution 
that has either been living under a rock the last few years or has done 
zero research into the capabilities they are using for their solution. 
IMHO it is better to not inhibit the first group trying to save the 
latter group as the latter are always doomed to failure.

> But the description of all this could certainly do a better job of
> explaining what's going on.

I would be glad to do so, and have tried several ways to explain it. 
Even working with multiple people that understand the problem to draft a 
better explanation. It would be greatly appreciated if you could provide 
what points you think should be clarified to better help convey the 
situation.

> --Andy
> 
>> [1] A future expansion of Secure Launch will be to enable usage of
>> Intel's Hardware Shield, link below, to provide runtime trustworthy
>> determination of SMM. The full extent of this capability can only be
>> achieved under a DRTM launch of the system with Intel TXT. When enabled,
>> this can be used to verify the SMM protections are in place and inform
>> the kernel's memory management which regions of memory are safe from SMM
>> tampering.
>>
>> https://www.intel.com/content/dam/www/central-libraries/us/en/documents/drtm-based-computing-whitepaper.pdf
> 
> Wow.  I skimmed this paper.  What an overcomplicated solution to a
> problem that doesn't deserve to exist in the first place.

While we could have a long discussion over the merits of SMM, the fact 
we have to face is that it is here, and it is not going anywhere any 
time soon. I honestly found AMD's SMM Containerization (Appendix D of 
the AMD64 Architecture Programmer’s Manual - Volume 2) the better 
approach, and it saddens me that it is completely disabled.

v/r,
dps
James Bottomley Nov. 2, 2024, 4:04 p.m. UTC | #26 
On Sat, 2024-11-02 at 10:53 -0400, Daniel P. Smith wrote:
> Hi Luto,
> 
> My apologies, I missed this response and the active on v11 cause me
> to 
> get an inquiry why I hadn't responded.
> 
> On 9/21/24 18:40, Andy Lutomirski wrote:
[...]
> > I assumed that "deliberately cap" meant that there was an actual
> > feature where you write something to the event log (if applicable)
> > and extend the PCR in a special way that *turns that PCR off*. 
> > That is, it does something such that later-loaded software *can't*
> > use that PCR to attest or unseal anything, etc.
> > 
> > But it sounds like you're saying that no such feature exists.  And
> > a quick skim of the specs doesn't come up with anything.  And the
> > SHA1 banks may well be susceptible to a collision attack.
> 
> Correct, the only entity that can disable PCR banks is the firmware. 

No, that's not correct.  Any user can use TPM_PCR_Allocate to activate
or deactivate individual banks.  The caveat is the change is not
implemented until the next TPM reset (which should involve a reboot). 
BIOS also gets to the TPM before the kernel does, so it can, in theory,
check what banks a TPM has and call TPM_PCR_Allocate to change them. 
In practice, because this requires a reboot, this is usually only done
from the BIOS menus not on a direct boot ... so you can be reasonably
sure that whatever changes were made will stick.

> When it initializes the TPM, it can disable banks/algorithms. After 
> that, when an extend operation is done, the TPM is expecting an entry
> for all active PCR banks and the TPM itself does the extend hash that
> is stored into the PCRs.

This, also, is not quite correct: an extend is allowed to specify banks
that don't exist (in which case nothing happens and no error is
reported) and miss banks that do (in which case no extend is done to
that bank).  In the early days of TPM2, some BIOS implementations only
extended sha1 for instance, meaning the sha256 banks were all zero when
the kernel started.

Even today, if you activate a bank the BIOS doesn't know about, it
likely won't extend it.  You can see this in VM boots with OVMF and
software TPMs having esoteric banks like SM3.

Regards,

James
Daniel P. Smith Nov. 15, 2024, 1:17 a.m. UTC | #27 
On 11/2/24 12:04, James Bottomley wrote:
> On Sat, 2024-11-02 at 10:53 -0400, Daniel P. Smith wrote:
>> Hi Luto,
>>
>> My apologies, I missed this response and the active on v11 cause me
>> to
>> get an inquiry why I hadn't responded.
>>
>> On 9/21/24 18:40, Andy Lutomirski wrote:
> [...]
>>> I assumed that "deliberately cap" meant that there was an actual
>>> feature where you write something to the event log (if applicable)
>>> and extend the PCR in a special way that *turns that PCR off*.
>>> That is, it does something such that later-loaded software *can't*
>>> use that PCR to attest or unseal anything, etc.
>>>
>>> But it sounds like you're saying that no such feature exists.  And
>>> a quick skim of the specs doesn't come up with anything.  And the
>>> SHA1 banks may well be susceptible to a collision attack.
>>
>> Correct, the only entity that can disable PCR banks is the firmware.
> 
> No, that's not correct.  Any user can use TPM_PCR_Allocate to activate
> or deactivate individual banks.  The caveat is the change is not
> implemented until the next TPM reset (which should involve a reboot).
> BIOS also gets to the TPM before the kernel does, so it can, in theory,
> check what banks a TPM has and call TPM_PCR_Allocate to change them.
> In practice, because this requires a reboot, this is usually only done
> from the BIOS menus not on a direct boot ... so you can be reasonably
> sure that whatever changes were made will stick.

Okay, since there is a desire for exactness. Any system software can 
send the TPM_PCR_Allocate command, specifying which PCRs should be 
activated on next _TPM_init. There are restrictions such that if 
DRTM_PCR is defined, then at least one bank must have a D-RTM PCR 
allocation. In agreement with my statement, this is the mechanism used 
by firmware to select the banks. Depending on the firmware 
implementation, the firmware request will likely override the request 
sent by the system software.

This brings us back to an earlier point, if one disables the SHA1 banks 
in BIOS menu, then TXT will not use them and thus neither will Secure 
Launch. Secure Launch will only use the algorithms used by the CPU and 
the ACM.

>> When it initializes the TPM, it can disable banks/algorithms. After
>> that, when an extend operation is done, the TPM is expecting an entry
>> for all active PCR banks and the TPM itself does the extend hash that
>> is stored into the PCRs.
> 
> This, also, is not quite correct: an extend is allowed to specify banks
> that don't exist (in which case nothing happens and no error is
> reported) and miss banks that do (in which case no extend is done to
> that bank).  In the early days of TPM2, some BIOS implementations only
> extended sha1 for instance, meaning the sha256 banks were all zero when
> the kernel started.
> 
> Even today, if you activate a bank the BIOS doesn't know about, it
> likely won't extend it.  You can see this in VM boots with OVMF and
> software TPMs having esoteric banks like SM3.

Let me correct myself here and again be extremely precise. When an 
extend operation is done, the TPM driver expects to receive an array of 
digests that is the same size as the number of allocated/active banks. 
Specifically, it loops from 0 to chip->nr_allocated_banks, filling 
TPML_DIGEST_VALUES with an entry for all the active banks, to include 
SHA1 if it is active. Coming back to my response to Luto, we can either 
populate it with 0 or a well-known value for each extend we send. 
Regardless of what the value is, the TPM will use its implementation of 
SHA1 to calculate the resulting extend value.

Even with these clarifications, the conclusion does not change. If the 
firmware enables SHA1, there is nothing that can be done to disable or 
block its usage from the user. Linux Secure Launch sending measurements 
to all the banks that the hardware used to start the DRTM chain does not 
create a vulnerability in and of itself. The user is free to leverage 
the SHA1 bank in any of the TPM's Integrity Collection suite of 
operations, regardless of what Secure Launch sends for the SHA1 hash. 
Whereas, neutering the solution of SHA1 breaks the ability for it to 
support any hardware that has a TPM1.2, of which there are still many in 
use.

V/r,
Daniel P. Smith
Andy Lutomirski Nov. 18, 2024, 6:43 p.m. UTC | #28 
On Thu, Nov 14, 2024 at 5:17 PM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> On 11/2/24 12:04, James Bottomley wrote:
> > On Sat, 2024-11-02 at 10:53 -0400, Daniel P. Smith wrote:
> >> Hi Luto,
> >>
> >> My apologies, I missed this response and the active on v11 cause me
> >> to
> >> get an inquiry why I hadn't responded.
> >>
> >> On 9/21/24 18:40, Andy Lutomirski wrote:
> > [...]
> >>> I assumed that "deliberately cap" meant that there was an actual
> >>> feature where you write something to the event log (if applicable)
> >>> and extend the PCR in a special way that *turns that PCR off*.
> >>> That is, it does something such that later-loaded software *can't*
> >>> use that PCR to attest or unseal anything, etc.
> >>>
> >>> But it sounds like you're saying that no such feature exists.  And
> >>> a quick skim of the specs doesn't come up with anything.  And the
> >>> SHA1 banks may well be susceptible to a collision attack.
> >>
> >> Correct, the only entity that can disable PCR banks is the firmware.
> >
> > No, that's not correct.  Any user can use TPM_PCR_Allocate to activate
> > or deactivate individual banks.  The caveat is the change is not
> > implemented until the next TPM reset (which should involve a reboot).
> > BIOS also gets to the TPM before the kernel does, so it can, in theory,
> > check what banks a TPM has and call TPM_PCR_Allocate to change them.
> > In practice, because this requires a reboot, this is usually only done
> > from the BIOS menus not on a direct boot ... so you can be reasonably
> > sure that whatever changes were made will stick.
>
> Okay, since there is a desire for exactness. Any system software can
> send the TPM_PCR_Allocate command, specifying which PCRs should be
> activated on next _TPM_init. There are restrictions such that if
> DRTM_PCR is defined, then at least one bank must have a D-RTM PCR
> allocation. In agreement with my statement, this is the mechanism used
> by firmware to select the banks. Depending on the firmware
> implementation, the firmware request will likely override the request
> sent by the system software.
>
> This brings us back to an earlier point, if one disables the SHA1 banks
> in BIOS menu, then TXT will not use them and thus neither will Secure
> Launch. Secure Launch will only use the algorithms used by the CPU and
> the ACM.
>
> >> When it initializes the TPM, it can disable banks/algorithms. After
> >> that, when an extend operation is done, the TPM is expecting an entry
> >> for all active PCR banks and the TPM itself does the extend hash that
> >> is stored into the PCRs.
> >
> > This, also, is not quite correct: an extend is allowed to specify banks
> > that don't exist (in which case nothing happens and no error is
> > reported) and miss banks that do (in which case no extend is done to
> > that bank).  In the early days of TPM2, some BIOS implementations only
> > extended sha1 for instance, meaning the sha256 banks were all zero when
> > the kernel started.
> >
> > Even today, if you activate a bank the BIOS doesn't know about, it
> > likely won't extend it.  You can see this in VM boots with OVMF and
> > software TPMs having esoteric banks like SM3.

How is this not a security hole you could drive a truck through?
Indeed, looking at the docs, TPM2_PCR_Extend says "If no digest value
is specified for a bank, then the PCR in that bank is not modified."

>
> Let me correct myself here and again be extremely precise. When an
> extend operation is done, the TPM driver expects to receive an array of
> digests that is the same size as the number of allocated/active banks.
> Specifically, it loops from 0 to chip->nr_allocated_banks, filling
> TPML_DIGEST_VALUES with an entry for all the active banks, to include
> SHA1 if it is active. Coming back to my response to Luto, we can either
> populate it with 0 or a well-known value for each extend we send.
> Regardless of what the value is, the TPM will use its implementation of
> SHA1 to calculate the resulting extend value.

At least extending unknown/unsupported banks with 0 modifies the bank,
which gives software that might rely on that bank an indication that
something in the chain doesn't support the bank.  But does actual
TPM-using software in the wild actually look up the event log and
notice that it contains a 0?

This sucks.  How on Earth didn't the TPM2 spec do this instead of
having explicit handling for "a PCR got extended, and the code that
extended it didn't support a given bank, and therefore *the resulting
PCR value cannot be relied on*?  It would have been *one single bit
per PCR, bank* indicating that the PCR's value is incomplete, along
with some basic logic that an incomplete PCR cannot magically become
complete, nor can it be used to authorize anything unless the
authorization policy explicitly allows it?

Anyway, other than the fact that everyone (presumably?) expects
software to be aware of SHA-1 and (mostly) SHA256, and presumably
users of SM3 already expect that a lot of things don't support it,
SHA1 doesn't seem very different from SM3 in the sense that (a) people
might not want to support it and (b) the actual behavior of a boot
chain component that doesn't support a cryptosystem is FUNDAMENTALLY
DANGEROUS.

Is there explicit guidance from TCG as to how this is supposed to work?


In any case, I have a strawman suggestion to resolve this issue much
better from Linux's perspective.  It's a strawman because, while I
attempted to read the relevant part of the specs, the specs and the
ecosystem are a mess, so I could be wrong.

Linux should not use TPM2_PCR_Extend *at all*.  Instead, Linux should
exclusively use TPM2_PCR_Event.  I would expect that passing, say, the
entire kernel image to TPM2_PCR_Event would be a big mistake, so
instead Linux should hash the relevant data with a reasonable
suggestion of hashes (which includes, mandatorily, SHA-384 and *does
not* include SHA-1, and may or may not be configurable at build time
to include things like SM3), concatenate them, and pass that to
TPM2_PCR_Event.  And Linux should make the value that it passed to
TPM2_PCR_Event readily accessible to software using it, and should
also include some straightforward tooling to calculate it from a given
input so that software that wants to figure out what value to expect
in a PCR can easily do so.

And then software that wants to use a SHA-1 bank will work every bit
as well as it would if Linux actually implemented it, but Linux can
happily not implement it, and even users of oddball algorithms that
Linux has never heard of will get secure behavior.

(Why SHA-384?  Because it's mandatory in the TPM Client profile, and
anyone who's happy with SHA-256 should also be willing to accept
SHA-384.)

>
> Even with these clarifications, the conclusion does not change. If the
> firmware enables SHA1, there is nothing that can be done to disable or
> block its usage from the user. Linux Secure Launch sending measurements
> to all the banks that the hardware used to start the DRTM chain does not
> create a vulnerability in and of itself. The user is free to leverage
> the SHA1 bank in any of the TPM's Integrity Collection suite of
> operations, regardless of what Secure Launch sends for the SHA1 hash.
> Whereas, neutering the solution of SHA1 breaks the ability for it to
> support any hardware that has a TPM1.2, of which there are still many in
> use.
>
> V/r,
> Daniel P. Smith
>
>
Andy Lutomirski Nov. 18, 2024, 6:50 p.m. UTC | #29 
On Mon, Nov 18, 2024 at 10:43 AM Andy Lutomirski <luto@amacapital.net> wrote:
>

> Linux should not use TPM2_PCR_Extend *at all*.  Instead, Linux should
> exclusively use TPM2_PCR_Event.  I would expect that passing, say, the
> entire kernel image to TPM2_PCR_Event would be a big mistake, so
> instead Linux should hash the relevant data with a reasonable
> suggestion of hashes (which includes, mandatorily, SHA-384 and *does
> not* include SHA-1, and may or may not be configurable at build time
> to include things like SM3), concatenate them, and pass that to
> TPM2_PCR_Event.  And Linux should make the value that it passed to
> TPM2_PCR_Event readily accessible to software using it, and should
> also include some straightforward tooling to calculate it from a given
> input so that software that wants to figure out what value to expect
> in a PCR can easily do so.

Whoops, putting on my "knows a bit about crypto" hat for a second,
this is not great, as the algorithms aren't distinguished, and one
could hypothetically add a wildly insecure hash to the list that
breaks it.  Instead it should be something like:

"SHA-384 48 bytes: [the SHA-384 data], someotherhash 71 bytes: [other
data], ..."

It might even be polite to include some human readable text that also
indicates what got hashed, e.g. "initramfs", so that anyone reading
the event log can see what got hashed.  On that note, maybe making the
whole thing human readable and using base64 would be nice:

"initramfs\nsha384 [base64 data]\nblake3 [base64 data]\nsm3 [base64 data]"

Whatever format is used should be unambiguously parseable.  And who
knows, maybe there's already some kind of industry standard for how
TPM-using software is expected to behave here.


>
> And then software that wants to use a SHA-1 bank will work every bit
> as well as it would if Linux actually implemented it, but Linux can
> happily not implement it, and even users of oddball algorithms that
> Linux has never heard of will get secure behavior.
>
> (Why SHA-384?  Because it's mandatory in the TPM Client profile, and
> anyone who's happy with SHA-256 should also be willing to accept
> SHA-384.)
>
> >
> > Even with these clarifications, the conclusion does not change. If the
> > firmware enables SHA1, there is nothing that can be done to disable or
> > block its usage from the user. Linux Secure Launch sending measurements
> > to all the banks that the hardware used to start the DRTM chain does not
> > create a vulnerability in and of itself. The user is free to leverage
> > the SHA1 bank in any of the TPM's Integrity Collection suite of
> > operations, regardless of what Secure Launch sends for the SHA1 hash.
> > Whereas, neutering the solution of SHA1 breaks the ability for it to
> > support any hardware that has a TPM1.2, of which there are still many in
> > use.
> >
> > V/r,
> > Daniel P. Smith
> >
> >
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC



--
Andy Lutomirski
AMA Capital Management, LLC
James Bottomley Nov. 18, 2024, 7:12 p.m. UTC | #30 
On Mon, 2024-11-18 at 10:43 -0800, Andy Lutomirski wrote:
> Linux should not use TPM2_PCR_Extend *at all*.  Instead, Linux should
> exclusively use TPM2_PCR_Event.  I would expect that passing, say,
> the entire kernel image to TPM2_PCR_Event would be a big mistake, so
> instead Linux should hash the relevant data with a reasonable
> suggestion of hashes (which includes, mandatorily, SHA-384 and *does
> not* include SHA-1, and may or may not be configurable at build time
> to include things like SM3), concatenate them, and pass that to
> TPM2_PCR_Event.  And Linux should make the value that it passed to
> TPM2_PCR_Event readily accessible to software using it, and should
> also include some straightforward tooling to calculate it from a
> given input so that software that wants to figure out what value to
> expect in a PCR can easily do so.

Just for clarity, this is about how the agile log format works.  Each
event entry in the log contains a list of bank hashes and the extends
occur in log event order, so replaying a log should get you to exactly
the head PCR value of each bank.  If a log doesn't understand a format,
like SM3, then an entry for it doesn't appear in the log and a replay
says nothing about the PCR value.

For some events, the hash is actually the hash of the event entry
itself and for others, the entry is just a hint and the hash is of
something else.

I think part of the confusion stems from the twofold issues of PCRs: at
their simplest they were expected to provide the end policy values
(this turns out to be problematic because there are quite a few ways,
that will produce different end PCR values, that a system could get to
the same state).  If you don't trust a bank (or don't know about it),
you don't code it into a required policy statement and its value
becomes irrelevant.  If, as most remote attestation systems do, you're
analysing log entries, then you can calculate end PCR points for all
banks mentioned in the log and you could ask the TPM to quote all of
them.  In practice, you tend to pick a bank you prefer (sha256 usually)
and quote only that.  Again, if a bank doesn't appear in the log,
you're not going to ask for a quote from it, so what it contains is
irrelevant to the analysis of the log.


The point being that in neither case would the fact that boot software
failed to extend a bank it didn't have a hash for result in some type
of compromise.

Note that one of the things you can do with the log (because the
entries are separable) is strip out all the hashes for a bank. 
However, the remote is likely to refuse to accept the log if you, say,
strip the sha256 ones because you think a collision allows you to fake
a sha1 log because it would know you should have had sha256 entries as
well.

By the way, the only modern hash you can rely on a TPM2 having is
sha256.  Most of the older ones don't have sha384.  They all do have
sha1 for backwards compatibility with TPM1.2

James
Andy Lutomirski Nov. 18, 2024, 8:02 p.m. UTC | #31 
On Mon, Nov 18, 2024 at 11:12 AM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Mon, 2024-11-18 at 10:43 -0800, Andy Lutomirski wrote:
> > Linux should not use TPM2_PCR_Extend *at all*.  Instead, Linux should
> > exclusively use TPM2_PCR_Event.  I would expect that passing, say,
> > the entire kernel image to TPM2_PCR_Event would be a big mistake, so
> > instead Linux should hash the relevant data with a reasonable
> > suggestion of hashes (which includes, mandatorily, SHA-384 and *does
> > not* include SHA-1, and may or may not be configurable at build time
> > to include things like SM3), concatenate them, and pass that to
> > TPM2_PCR_Event.  And Linux should make the value that it passed to
> > TPM2_PCR_Event readily accessible to software using it, and should
> > also include some straightforward tooling to calculate it from a
> > given input so that software that wants to figure out what value to
> > expect in a PCR can easily do so.
>
> Just for clarity, this is about how the agile log format works.  Each
> event entry in the log contains a list of bank hashes and the extends
> occur in log event order, so replaying a log should get you to exactly
> the head PCR value of each bank.  If a log doesn't understand a format,
> like SM3, then an entry for it doesn't appear in the log and a replay
> says nothing about the PCR value.

I have no idea what the "agile log format" is or what all the formats
in existence are.  I found section 4.2.4 here:

https://trustedcomputinggroup.org/wp-content/uploads/TCG_IWG_CEL_v1_r0p41_pub.pdf

It says:

This field contains the list of the digest values Extended. The Extend
method varies with TPM command, so there is
no uniform meaning of TPM Extend in this instance, and separate
descriptions are unavoidable. If using the
TPM2_PCR_Extend command, this field is the data sent to the TPM (i.e.,
not the resulting value of the PCR after the
TPM2_PCR_Extend command completes). If using the TPM2_PCR_Event
command, this field contains the digest
structure returned by the TPM2_PCR_Event command (that contains the
digest(s) submitted to each PCR bank as
the internal Extend operation). This field SHALL contain the
information from the TPML_DIGEST_VALUES used in
the Extend operation.

So we're logging the values with which we extend the PCRs.  Once upon
a time, someone decided it was okay to skip extending a PCR bank:

https://google.github.io/security-research/pocs/bios/tpm-carte-blanche/writeup.html

and it was not a great idea.

There seem to be six (!) currently defined hashes: SHA1, SHA256,
SHA384, SHA512, SM2 and SM3.  I haven't spotted anything promising not
to add more.  It seems to be that Linux really really ought to:

(a) extend all banks.  Not all banks that the maintainers like, and
not all banks that the maintainers knew about when having this
discussion.  *All* banks.  That means TPM2_PCR_Event().  (Or we refuse
to boot if there's a bank we don't like.)

(b) Make a best effort to notice if something is wrong with the TPM
and/or someone is MITMing us and messing with us.  That means
computing the hash algorithms we actually support and checking whether
TPM2_PCR_Event() returns the right thing.  I'm not seeing a specific
attack that seems likely that this prevents, but it does seem like
decent defense in depth, and if someone chooses to provision a machine
by reading its event log and then subsequently getting an attestation
that a future event log matches what was read, then avoiding letting
an attacker who temporarily controls the TPM connection from
corrupting the results seems wise.  And I don't see anything at all
that we gain by removing a check that (TPM's reported SHA1 == what we
calculated) in the name of "not supporting SHA1") other than a few
hundred bytes of object code.  (And yes, SHA1 is much more likely to
be supported than SM3, so it's not absurd to implement SHA1 and not
implement SM3.)

>
> For some events, the hash is actually the hash of the event entry
> itself and for others, the entry is just a hint and the hash is of
> something else.
>
> I think part of the confusion stems from the twofold issues of PCRs: at
> their simplest they were expected to provide the end policy values
> (this turns out to be problematic because there are quite a few ways,
> that will produce different end PCR values, that a system could get to
> the same state).  If you don't trust a bank (or don't know about it),
> you don't code it into a required policy statement and its value
> becomes irrelevant.

I think that "you" refers to multiple entities, and this is a problem.

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.
Andy Lutomirski Nov. 21, 2024, 8:54 p.m. UTC | #32 
On Thu, Nov 21, 2024 at 12:11 PM <ross.philipson@oracle.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
Andy Lutomirski Nov. 21, 2024, 10:42 p.m. UTC | #33 
On Thu, Nov 21, 2024 at 12:54 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
> On Thu, Nov 21, 2024 at 12:11 PM <ross.philipson@oracle.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.
diff mbox series

Patch

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index e9522c6893be..3307ebef4e1b 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -118,6 +118,8 @@  vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o
 vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_mixed.o
 vmlinux-objs-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
 
+vmlinux-objs-$(CONFIG_SECURE_LAUNCH) += $(obj)/early_sha1.o
+
 $(obj)/vmlinux: $(vmlinux-objs-y) FORCE
 	$(call if_changed,ld)
 
diff --git a/arch/x86/boot/compressed/early_sha1.c b/arch/x86/boot/compressed/early_sha1.c
new file mode 100644
index 000000000000..8a9b904a73ab
--- /dev/null
+++ b/arch/x86/boot/compressed/early_sha1.c
@@ -0,0 +1,12 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2024 Apertus Solutions, LLC.
+ */
+
+#include <linux/init.h>
+#include <linux/linkage.h>
+#include <linux/string.h>
+#include <asm/boot.h>
+#include <asm/unaligned.h>
+
+#include "../../../../lib/crypto/sha1.c"
diff --git a/include/crypto/sha1.h b/include/crypto/sha1.h
index 044ecea60ac8..d715dd5332e1 100644
--- a/include/crypto/sha1.h
+++ b/include/crypto/sha1.h
@@ -42,5 +42,6 @@  extern int crypto_sha1_finup(struct shash_desc *desc, const u8 *data,
 #define SHA1_WORKSPACE_WORDS	16
 void sha1_init(__u32 *buf);
 void sha1_transform(__u32 *digest, const char *data, __u32 *W);
+void sha1(const u8 *data, unsigned int len, u8 *out);
 
 #endif /* _CRYPTO_SHA1_H */
diff --git a/lib/crypto/sha1.c b/lib/crypto/sha1.c
index 1aebe7be9401..10152125b338 100644
--- a/lib/crypto/sha1.c
+++ b/lib/crypto/sha1.c
@@ -137,4 +137,85 @@  void sha1_init(__u32 *buf)
 }
 EXPORT_SYMBOL(sha1_init);
 
+static void __sha1_transform(u32 *digest, const char *data)
+{
+       u32 ws[SHA1_WORKSPACE_WORDS];
+
+       sha1_transform(digest, data, ws);
+
+       memzero_explicit(ws, sizeof(ws));
+}
+
+static void sha1_update(struct sha1_state *sctx, const u8 *data, unsigned int len)
+{
+	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
+
+	sctx->count += len;
+
+	if (likely((partial + len) >= SHA1_BLOCK_SIZE)) {
+		int blocks;
+
+		if (partial) {
+			int p = SHA1_BLOCK_SIZE - partial;
+
+			memcpy(sctx->buffer + partial, data, p);
+			data += p;
+			len -= p;
+
+			__sha1_transform(sctx->state, sctx->buffer);
+		}
+
+		blocks = len / SHA1_BLOCK_SIZE;
+		len %= SHA1_BLOCK_SIZE;
+
+		if (blocks) {
+			while (blocks--) {
+				__sha1_transform(sctx->state, data);
+				data += SHA1_BLOCK_SIZE;
+			}
+		}
+		partial = 0;
+	}
+
+	if (len)
+		memcpy(sctx->buffer + partial, data, len);
+}
+
+static void sha1_final(struct sha1_state *sctx, u8 *out)
+{
+	const int bit_offset = SHA1_BLOCK_SIZE - sizeof(__be64);
+	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
+	__be64 *bits = (__be64 *)(sctx->buffer + bit_offset);
+	__be32 *digest = (__be32 *)out;
+	int i;
+
+	sctx->buffer[partial++] = 0x80;
+	if (partial > bit_offset) {
+		memset(sctx->buffer + partial, 0x0, SHA1_BLOCK_SIZE - partial);
+		partial = 0;
+
+		__sha1_transform(sctx->state, sctx->buffer);
+	}
+
+	memset(sctx->buffer + partial, 0x0, bit_offset - partial);
+	*bits = cpu_to_be64(sctx->count << 3);
+	__sha1_transform(sctx->state, sctx->buffer);
+
+	for (i = 0; i < SHA1_DIGEST_SIZE / sizeof(__be32); i++)
+		put_unaligned_be32(sctx->state[i], digest++);
+
+	*sctx = (struct sha1_state){};
+}
+
+void sha1(const u8 *data, unsigned int len, u8 *out)
+{
+	struct sha1_state sctx = {0};
+
+	sha1_init(sctx.state);
+	sctx.count = 0;
+	sha1_update(&sctx, data, len);
+	sha1_final(&sctx, out);
+}
+EXPORT_SYMBOL(sha1);
+
 MODULE_LICENSE("GPL");