| Message ID | 20240531010331.134441-7-ross.philipson@oracle.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Trenchboot secure dynamic launch Linux kernel support | expand |
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
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
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
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
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
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
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
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
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
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
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?)
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
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.
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
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
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
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 >
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
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
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?
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
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
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
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");