| 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
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.
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
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
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
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 > >
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
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
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.
On 11/18/24 12:02 PM, Andy Lutomirski wrote: > 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://urldefense.com/v3/__https://trustedcomputinggroup.org/wp-content/uploads/TCG_IWG_CEL_v1_r0p41_pub.pdf__;!!ACWV5N9M2RV99hQ!Iw9aAHcJMT6j3t_DSb7cOk8iWy8VJYkJOlGQ_gtLUz0XwPcIZclY4I8GZJ5VP4OScLjBaz3RX1QlGGBWWZw$ > > 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://urldefense.com/v3/__https://google.github.io/security-research/pocs/bios/tpm-carte-blanche/writeup.html__;!!ACWV5N9M2RV99hQ!Iw9aAHcJMT6j3t_DSb7cOk8iWy8VJYkJOlGQ_gtLUz0XwPcIZclY4I8GZJ5VP4OScLjBaz3RX1QlKxD4S1w$ > > 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. 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. As far as our patch series goes, we have done the things that were asked of us like documenting SHA-1 usage, fixing comments and commit message and breaking up the original patch into two (one for SHA-1 and one for SHA-256). It seems we should be able to submit our next version at this point. Thanks Ross
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
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.
On 11/21/24 2:42 PM, Andy Lutomirski wrote: > 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): > Concerning my previous response, I realized that I did not fully understand what you were suggesting/proposing so I am sorry about that. You are correct that addressing this is within the scope of what we are doing. I have reread all the emails again and I/we now understand what you are saying. We are now exploring how we might use TPM2_PCR_Event, whether it introduces other issues with respect to how TXT/ACM might behave and what would be needed to adopt this approach. More to come on that front. I will mention that the TPM code in the Linux kernel does not currently support the TPM2_PCR_Event command. The functionality needs to be added and that needs buy in from the TPM maintainers (probably guidance too). A bit more below to clarify a few things... > > ... 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. Noted, there does look like there is an issue there. Thank you for the analysis. > > 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. We gather the list of algorithms used from the DRTM event log which the TXT/ACM phase initializes and begins populating. One thing to note here is that in the early setup kernel stub code, we would fail to boot if we saw an algorithm we did not support so we would not reach a state where we were simply ignoring an active bank as you mentioned in an earlier reply. But I also understand your point about limiting the functionality to just a subset of algorithms. Thank you for your feedback, Ross
Hey Luto! Let me try to address your concerns below. On 11/18/24 15:02, Andy Lutomirski wrote: > 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. Let me start with providing background on the two measurement policies that is implemented by Intel TXT (from Intel TXT Developers Guide): - Maximum Agility PCR Extend Policy: ACM can support algorithm agile commands TPM2_PCR_Event; TPM2_HashSequenceStart; TPM2_HashUpdate; TPM2_EventSequenceComplete. When this policy is selected, ACM will use the commands above if not all PCR algorithms are covered by embedded set of algorithms and will extend all existing PCR banks. Side effect of this policy is possible performance loss. ‒ Maximum Performance PCR Extend Policy: ACM can support several hash algorithms via embedded SW. When this policy is selected, ACM will use embedded SW to compute hashes and then will use TPM2_PCR_Extend commands to extend them into PCRs. If PCRs utilizing hash algorithms not supported by SW are discovered, they will be capped with “1” value. This policy, when selected, will ensure maximum possible performance but has side effect of possible capping of some of the PCRs. Allow me to clarify/expand on the last statement in Maximum Agility. There is almost certainly a performance loss as anything larger than 1024 bytes, for example the Linux kernel, the ACM will bit-banging the bytes to the TPM using the TPM2_Hash* functions. Before addressing the next point, I would also clarify how the D-CRTM measurement taken by the CPU is done. It uses the _TPM_HASH_* functions, Section 22.9 of TPM2 Commands specification, to store SHA256(SINIT ACM) | EDX into all active PCR banks. For clarity, when this done, EDX holds the 4-byte value of the SENTER parameters for which 0 is the only valid value currently. > 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. Let's begin by why/how that attack occurs. The TPM Carte Blanche attack took advantage of the fact that without BootGuard in place, the SRTM measurements are done by the software/firmware, to include the self-referential S-CRTM measurement. In particular, for the target platform, it just so happens that it was possible to construct a configuration where not a single hash would be sent to the SHA256 bank. This allowed the attacker the ability to replay any set of measurements, i.e. carte blanche control, into a completely empty PCR bank for which the attestation service would accept quotes. The key to this attack requires both, access to an empty PCR bank, and an attestation service that will accept a quote with only the exploited bank present. Let us return to my statements above, which will demonstrate why TXT/DRTM completely invalidates the attack. First, as noted above, when the CPU is processing the GETSEC[SENTER] instruction, it (the CPU) will compute the D-CRTM as SHA256(SINIT ACM) | EDX, sending it to the TPM using _TPM_HASH_* functions. The _TPM_HASH_* functions result in all PCR banks to be extended with the D-CRTM value. If Maximum Performance PCR Extend policy is in use, which is the default policy used by TrenchBoot, any algorithm not supported by the ACM is capped by sending the value "1" as the digest value for the extend. Therefore, after the TXT sequence has completed and before control is given to the Linux kernel by the ACM, all PCR banks will consist of either, the D-CRTM + all ACM measurements, or the D-CRTM + TPM2_PCR_Extend(0x1). There will be no PCR banks with empty DRTM PCRs, thus none of the banks would be usable for a TPM Carte Blanche-style attack. > 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.) Or, (c) Upon initialization, cap the PCR banks with unsupported algorithms using a well-known value. A problem with (a) is that the result will be an unorthodox event, PCR_EXTEND(H(H'(data))). An attestation verifier will have to be aware of that this is being done, and have a way to determine which method was used for each event. This creates a potentially expensive cost for any existing attestation solutions to incorporate support for the unorthodox event. At least for DRTM solutions, it seeks to solve a problem that TXT does not experience. For Linux Secure Launch, I would like to propose an alternative to what the current logic does in the setup kernel. Specifically, Secure Launch will trigger a TXT reset when an unsupported algorithm is encountered. Instead, I would like to propose the adoption of (c), and have it extends a well-known, fixed value for unsupported algorithms. Secure Launch can leverage the fact that the TPM driver's extend function already expects to be given digests for all active algorithms. Therefore, it will record the well-known value, 0x01 to follow the ACM, into the digest buffers of any algorithms that Secure Launch does not support. This will result in the well-known value being extended each time a measurement is recorded. This will not be a problem as no one should be using those banks for attestation and can ignore those digests in the event log. I would like to note that we made a conscious design decision early on to use the PCR performance policy approach. We weighed a variety of security concerns, hardware availability, and the practicality of integrating the capability into our respective efforts. I do not want you to feel as though we are not taking your comments seriously. Ross reached out to some their contacts, and I reached out to a colleague with domain experience as well. From a cursory review, no one saw an issue from a crypto standpoint, beyond some algorithm recommendations. As we highlighted, they did caution about the resulting unorthodox measurement that would impose a burden on attestation solutions. Hopefully With the background and context presented, you would agree the above is a reasonable approach. If you do have concerns, please let us know. V/r, Daniel P. Smith
On Thu, Dec 12, 2024 at 11:56 AM Daniel P. Smith <dpsmith@apertussolutions.com> wrote: > > Hey Luto! > > Let me try to address your concerns below. > > On 11/18/24 15:02, Andy Lutomirski wrote: > > 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. > > > Let me start with providing background on the two measurement policies > that is implemented by Intel TXT (from Intel TXT Developers Guide): > > - Maximum Agility PCR Extend Policy: ACM can support algorithm agile > commands TPM2_PCR_Event; TPM2_HashSequenceStart; TPM2_HashUpdate; > TPM2_EventSequenceComplete. When this policy is selected, ACM will use > the commands above if not all PCR algorithms are covered by embedded set > of algorithms and will extend all existing PCR banks. Side effect of > this policy is possible performance loss. > > ‒ Maximum Performance PCR Extend Policy: ACM can support several hash > algorithms via embedded SW. When this policy is selected, ACM will use > embedded SW to compute hashes and then will use TPM2_PCR_Extend commands > to extend them into PCRs. If PCRs utilizing hash algorithms not > supported by SW are discovered, they will be capped with “1” value. This > policy, when selected, will ensure maximum possible performance but has > side effect of possible capping of some of the PCRs. > What is responsible for choosing which of these policies to use? > Allow me to clarify/expand on the last statement in Maximum Agility. > There is almost certainly a performance loss as anything larger than > 1024 bytes, for example the Linux kernel, the ACM will bit-banging the > bytes to the TPM using the TPM2_Hash* functions. Surely, if Linux's stub started using TPM2_PCR_Event, it would first hash any large inputs and then send the _hash_ to TPM2_PCR_Event instead of, say, bit-banging the entire initramfs to the TPM. But you're talking about the ACM and I'm talking about the Linux stub code in this patchset. But the capping-with-"1" does suggest that maybe one can actually cap with "1" without preventing downstream software from consuming the event log. > > Before addressing the next point, I would also clarify how the D-CRTM > measurement taken by the CPU is done. It uses the _TPM_HASH_* functions, > Section 22.9 of TPM2 Commands specification, to store SHA256(SINIT ACM) > | EDX into all active PCR banks. For clarity, when this done, EDX holds > the 4-byte value of the SENTER parameters for which 0 is the only valid > value currently. > > > > 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. > > > Let's begin by why/how that attack occurs. The TPM Carte Blanche attack > took advantage of the fact that without BootGuard in place, the SRTM > measurements are done by the software/firmware, to include the > self-referential S-CRTM measurement. In particular, for the target > platform, it just so happens that it was possible to construct a > configuration where not a single hash would be sent to the SHA256 bank. > This allowed the attacker the ability to replay any set of measurements, > i.e. carte blanche control, into a completely empty PCR bank for which > the attestation service would accept quotes. The key to this attack > requires both, access to an empty PCR bank, and an attestation service > that will accept a quote with only the exploited bank present. > > Let us return to my statements above, which will demonstrate why > TXT/DRTM completely invalidates the attack. First, as noted above, when > the CPU is processing the GETSEC[SENTER] instruction, it (the CPU) will > compute the D-CRTM as SHA256(SINIT ACM) | EDX, sending it to the TPM > using _TPM_HASH_* functions. The _TPM_HASH_* functions result in all PCR > banks to be extended with the D-CRTM value. If Maximum Performance PCR > Extend policy is in use, which is the default policy used by TrenchBoot, > any algorithm not supported by the ACM is capped by sending the value > "1" as the digest value for the extend. Therefore, after the TXT > sequence has completed and before control is given to the Linux kernel > by the ACM, all PCR banks will consist of either, the D-CRTM + all ACM > measurements, or the D-CRTM + TPM2_PCR_Extend(0x1). There will be no PCR > banks with empty DRTM PCRs, thus none of the banks would be usable for a > TPM Carte Blanche-style attack. Sure, a "carte blanche" attack in the sense that the PCRs are entirely blank won't happen, but if any component in the chain does not extend a bank, then an attacker can replace the code _after_ that component without being noticed. > (c) Upon initialization, cap the PCR banks with unsupported algorithms > using a well-known value. > > A problem with (a) is that the result will be an unorthodox event, > PCR_EXTEND(H(H'(data))). An attestation verifier will have to be aware > of that this is being done, and have a way to determine which method was > used for each event. This creates a potentially expensive cost for any > existing attestation solutions to incorporate support for the unorthodox > event. At least for DRTM solutions, it seeks to solve a problem that TXT > does not experience. > > For Linux Secure Launch, I would like to propose an alternative to what > the current logic does in the setup kernel. Specifically, Secure Launch > will trigger a TXT reset when an unsupported algorithm is encountered. > Instead, I would like to propose the adoption of (c), and have it > extends a well-known, fixed value for unsupported algorithms. Secure > Launch can leverage the fact that the TPM driver's extend function > already expects to be given digests for all active algorithms. > Therefore, it will record the well-known value, 0x01 to follow the ACM, > into the digest buffers of any algorithms that Secure Launch does not > support. This will result in the well-known value being extended each > time a measurement is recorded. This will not be a problem as no one > should be using those banks for attestation and can ignore those digests > in the event log. This seems to be at least not terrible. Or I suppose one could use TPM_HASH_ functions? The spec for them is somewhat impenetrable to me. > > I would like to note that we made a conscious design decision early on > to use the PCR performance policy approach. We weighed a variety of > security concerns, hardware availability, and the practicality of > integrating the capability into our respective efforts. I do not want > you to feel as though we are not taking your comments seriously. Ross > reached out to some their contacts, and I reached out to a colleague > with domain experience as well. From a cursory review, no one saw an > issue from a crypto standpoint, beyond some algorithm recommendations. > As we highlighted, they did caution about the resulting unorthodox > measurement that would impose a burden on attestation solutions. > > Hopefully With the background and context presented, you would agree the > above is a reasonable approach. If you do have concerns, please let us know. > > V/r, > Daniel P. Smith
On 12/12/24 17:30, Andy Lutomirski wrote: > On Thu, Dec 12, 2024 at 11:56 AM Daniel P. Smith > <dpsmith@apertussolutions.com> wrote: >> >> Hey Luto! >> >> Let me try to address your concerns below. >> >> On 11/18/24 15:02, Andy Lutomirski wrote: >>> 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. >> >> >> Let me start with providing background on the two measurement policies >> that is implemented by Intel TXT (from Intel TXT Developers Guide): >> >> - Maximum Agility PCR Extend Policy: ACM can support algorithm agile >> commands TPM2_PCR_Event; TPM2_HashSequenceStart; TPM2_HashUpdate; >> TPM2_EventSequenceComplete. When this policy is selected, ACM will use >> the commands above if not all PCR algorithms are covered by embedded set >> of algorithms and will extend all existing PCR banks. Side effect of >> this policy is possible performance loss. >> >> ‒ Maximum Performance PCR Extend Policy: ACM can support several hash >> algorithms via embedded SW. When this policy is selected, ACM will use >> embedded SW to compute hashes and then will use TPM2_PCR_Extend commands >> to extend them into PCRs. If PCRs utilizing hash algorithms not >> supported by SW are discovered, they will be capped with “1” value. This >> policy, when selected, will ensure maximum possible performance but has >> side effect of possible capping of some of the PCRs. >> > > What is responsible for choosing which of these policies to use? It is setup by the Dynamic Launch Preamble (GRUB) that will invoke the Dynamic Launch Event (GETSEC[SENTER]). >> Allow me to clarify/expand on the last statement in Maximum Agility. >> There is almost certainly a performance loss as anything larger than >> 1024 bytes, for example the Linux kernel, the ACM will bit-banging the >> bytes to the TPM using the TPM2_Hash* functions. > > Surely, if Linux's stub started using TPM2_PCR_Event, it would first > hash any large inputs and then send the _hash_ to TPM2_PCR_Event > instead of, say, bit-banging the entire initramfs to the TPM. But > you're talking about the ACM and I'm talking about the Linux stub code > in this patchset. I think there might be some confusion on how this starts, let me provide a quick, enumerated flow: 1. GRUB prepares launch environment (Linux kernel, SINIT ACM, OsSinitData) and calls GETSEC[SENTER] instruction 2. The CPU signals PCH that DRTM has started, unlocking Locality 4 3. The CPU loads the SINIT ACM into CRAM, measures it with SHA256 and uses _TPM_HASH_START /_TPM_HASH_DATA / _TPM_HASH / _TPM_HASH_END to extend SHA256(ACM) | EDX into PCR17 for all PCR banks 4. The CPU then jumps into the ACM 5. The ACM does a series of actions and measurements of the system into PCRs 17 and 18 for SHA1, SHA256, SHA384, and SM3_256 (PCR Performance Policy) 6. The ACM then measures the "MLE", the Linux kernel in this case, using the same algorithms 7. The ACM then caps PCRs 17 and 18 by extending 0x01 for any unsupported algorithm the TPM supports. 8. The ACM then jumps to the Secure Launch entry point in the setup kernel If the initramfs is external, then yes, the setup kernel would get to do the measurement. If it is a unified kernel image, well, that whole thing will be bit banged to the TPM by the ACM if the PCR Agile Policy is used. > But the capping-with-"1" does suggest that maybe one can actually cap > with "1" without preventing downstream software from consuming the > event log. Correct, capping the PCR should never block downstream software from consuming the event log. >> >> Before addressing the next point, I would also clarify how the D-CRTM >> measurement taken by the CPU is done. It uses the _TPM_HASH_* functions, >> Section 22.9 of TPM2 Commands specification, to store SHA256(SINIT ACM) >> | EDX into all active PCR banks. For clarity, when this done, EDX holds >> the 4-byte value of the SENTER parameters for which 0 is the only valid >> value currently. >> >> >>> 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. >> >> >> Let's begin by why/how that attack occurs. The TPM Carte Blanche attack >> took advantage of the fact that without BootGuard in place, the SRTM >> measurements are done by the software/firmware, to include the >> self-referential S-CRTM measurement. In particular, for the target >> platform, it just so happens that it was possible to construct a >> configuration where not a single hash would be sent to the SHA256 bank. >> This allowed the attacker the ability to replay any set of measurements, >> i.e. carte blanche control, into a completely empty PCR bank for which >> the attestation service would accept quotes. The key to this attack >> requires both, access to an empty PCR bank, and an attestation service >> that will accept a quote with only the exploited bank present. >> >> Let us return to my statements above, which will demonstrate why >> TXT/DRTM completely invalidates the attack. First, as noted above, when >> the CPU is processing the GETSEC[SENTER] instruction, it (the CPU) will >> compute the D-CRTM as SHA256(SINIT ACM) | EDX, sending it to the TPM >> using _TPM_HASH_* functions. The _TPM_HASH_* functions result in all PCR >> banks to be extended with the D-CRTM value. If Maximum Performance PCR >> Extend policy is in use, which is the default policy used by TrenchBoot, >> any algorithm not supported by the ACM is capped by sending the value >> "1" as the digest value for the extend. Therefore, after the TXT >> sequence has completed and before control is given to the Linux kernel >> by the ACM, all PCR banks will consist of either, the D-CRTM + all ACM >> measurements, or the D-CRTM + TPM2_PCR_Extend(0x1). There will be no PCR >> banks with empty DRTM PCRs, thus none of the banks would be usable for a >> TPM Carte Blanche-style attack. > > Sure, a "carte blanche" attack in the sense that the PCRs are entirely > blank won't happen, but if any component in the chain does not extend > a bank, then an attacker can replace the code _after_ that component > without being noticed. There are two scenarios that could be considered here. The first is if an algorithm is added to the TPM that neither the ACM nor the TPM driver supports. The second is if an algorithm supported by the ACM and the TPM driver is added, but not supported by Secure Launch. In the first case, the PCR and corresponding log entries for the new algorithm will have the D-CRTM event and Cap (0x01) event. Rendering the bank unusable for the attacker. For the second case, this is a "it all depends on the usage". First and foremost, I would say it is ultimately a failure of the Attestation Service if it evaluated algorithms that Secure Launch is not advertised to support. With that, let's consider the options for bad components under the second case: - A bad kernel image will be detected since the ACM is what measures it. - A bad initramfs, assuming that (c) below was not implemented, then the initramfs could add a fake entry for itself into the bank in question. - Any component after this depends on the use case. The primary use case currently is to combine Secure Launch with TrenchBoot additions to u-root making it an intermediate bootloader that kexec's into the target kernel. I bring this up because when the kexec is done, Secure Launch does GETSEC[SEXIT]. The result being that it closes all localities except Locality 0. They cannot be re-opened without doing a new Dynamic Launch, which will reset them all back to 0. I would also like to note that Secure Launch would not be the only in-kernel capability that would have to scramble to add support for the additional algorithm if it suddenly became available to block a Carte Blanche attack. >> (c) Upon initialization, cap the PCR banks with unsupported algorithms >> using a well-known value. >> >> A problem with (a) is that the result will be an unorthodox event, >> PCR_EXTEND(H(H'(data))). An attestation verifier will have to be aware >> of that this is being done, and have a way to determine which method was >> used for each event. This creates a potentially expensive cost for any >> existing attestation solutions to incorporate support for the unorthodox >> event. At least for DRTM solutions, it seeks to solve a problem that TXT >> does not experience. >> >> For Linux Secure Launch, I would like to propose an alternative to what >> the current logic does in the setup kernel. Specifically, Secure Launch >> will trigger a TXT reset when an unsupported algorithm is encountered. >> Instead, I would like to propose the adoption of (c), and have it >> extends a well-known, fixed value for unsupported algorithms. Secure >> Launch can leverage the fact that the TPM driver's extend function >> already expects to be given digests for all active algorithms. >> Therefore, it will record the well-known value, 0x01 to follow the ACM, >> into the digest buffers of any algorithms that Secure Launch does not >> support. This will result in the well-known value being extended each >> time a measurement is recorded. This will not be a problem as no one >> should be using those banks for attestation and can ignore those digests >> in the event log. > > This seems to be at least not terrible. With (c) in place, it would completely close off the second case for the case when the initramfs is not part of the kernel image. > Or I suppose one could use TPM_HASH_ functions? The spec for them is > somewhat impenetrable to me. Do you mean TPM2_HashSequenceStart / TPM2_SequenceUpdate / TPM2_EventSequenceComplete? If you really meant _TPM_Hash_Start / _TPM_Hash_Data / _TPM_Hash_End, then no, you cannot call these. They are not sent via the Command Buffer but directly through the TPM interface. Have a look at Section 34 of the TPM Architecture[1]. While the spec authors wrote it generically, as if there is more than one way for an H-CRTM to be established, the only way available for x86 is DRTM. [1] https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-1.83-Part-1-Architecture.pdf Ross has prepared a new version of the series using (c), which actually reduces the size and complexity of the SL setup code. Once it has been tested, Ross will post it so you can have some concrete code to look at and consider the proposal. V/r, Daniel P. Smith
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");