| Message ID | 20230504145023.835096-7-ross.philipson@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | x86: Trenchboot secure dynamic launch Linux kernel support | expand |
On Thu, May 04, 2023 at 02:50:15PM +0000, Ross Philipson wrote: > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > The SHA algorithms are necessary to measure configuration information into > the TPM as early as possible before using the values. This implementation > uses the established approach of #including the SHA libraries directly in > the code since the compressed kernel is not uncompressed at this point. > > The SHA code here has its origins in the code from the main kernel: > > commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1") > > That code could not be pulled directly into the setup portion of the > compressed kernel because of other dependencies it pulls in. The result > is this is a modified copy of that code that still leverages the core > SHA algorithms. > > 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 | 97 +++++++++++++++++++++++++++++++++ > arch/x86/boot/compressed/early_sha1.h | 17 ++++++ > arch/x86/boot/compressed/early_sha256.c | 7 +++ > lib/crypto/sha1.c | 4 ++ > lib/crypto/sha256.c | 8 +++ > 6 files changed, 135 insertions(+) > create mode 100644 arch/x86/boot/compressed/early_sha1.c > create mode 100644 arch/x86/boot/compressed/early_sha1.h > create mode 100644 arch/x86/boot/compressed/early_sha256.c > > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile > index 6b6cfe6..1d327d4 100644 > --- a/arch/x86/boot/compressed/Makefile > +++ b/arch/x86/boot/compressed/Makefile > @@ -112,6 +112,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)/early_sha256.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 0000000..524ec23 > --- /dev/null > +++ b/arch/x86/boot/compressed/early_sha1.c > @@ -0,0 +1,97 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2022 Apertus Solutions, LLC. > + */ > + > +#include <linux/init.h> > +#include <linux/linkage.h> > +#include <linux/string.h> > +#include <asm/boot.h> > +#include <asm/unaligned.h> > + > +#include "early_sha1.h" > + > +#define SHA1_DISABLE_EXPORT Hi Ross, I could be mistaken, but it seems to me that *_DISABLE_EXPORT is a mechanism of this patch to disable exporting symbols (use of EXPORT_SYMBOL), at compile time. If so, this doesn't strike me as something that should be part of upstream kernel code. Could you consider dropping this part of the patch? ...
On 5/5/23 12:34, Simon Horman wrote: > On Thu, May 04, 2023 at 02:50:15PM +0000, Ross Philipson wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> The SHA algorithms are necessary to measure configuration information into >> the TPM as early as possible before using the values. This implementation >> uses the established approach of #including the SHA libraries directly in >> the code since the compressed kernel is not uncompressed at this point. >> >> The SHA code here has its origins in the code from the main kernel: >> >> commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1") >> >> That code could not be pulled directly into the setup portion of the >> compressed kernel because of other dependencies it pulls in. The result >> is this is a modified copy of that code that still leverages the core >> SHA algorithms. >> >> 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 | 97 +++++++++++++++++++++++++++++++++ >> arch/x86/boot/compressed/early_sha1.h | 17 ++++++ >> arch/x86/boot/compressed/early_sha256.c | 7 +++ >> lib/crypto/sha1.c | 4 ++ >> lib/crypto/sha256.c | 8 +++ >> 6 files changed, 135 insertions(+) >> create mode 100644 arch/x86/boot/compressed/early_sha1.c >> create mode 100644 arch/x86/boot/compressed/early_sha1.h >> create mode 100644 arch/x86/boot/compressed/early_sha256.c >> >> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile >> index 6b6cfe6..1d327d4 100644 >> --- a/arch/x86/boot/compressed/Makefile >> +++ b/arch/x86/boot/compressed/Makefile >> @@ -112,6 +112,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)/early_sha256.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 0000000..524ec23 >> --- /dev/null >> +++ b/arch/x86/boot/compressed/early_sha1.c >> @@ -0,0 +1,97 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2022 Apertus Solutions, LLC. >> + */ >> + >> +#include <linux/init.h> >> +#include <linux/linkage.h> >> +#include <linux/string.h> >> +#include <asm/boot.h> >> +#include <asm/unaligned.h> >> + >> +#include "early_sha1.h" >> + >> +#define SHA1_DISABLE_EXPORT > > Hi Ross, > > I could be mistaken, but it seems to me that *_DISABLE_EXPORT > is a mechanism of this patch to disable exporting symbols > (use of EXPORT_SYMBOL), at compile time. > > If so, this doesn't strike me as something that should be part of upstream > kernel code. Could you consider dropping this part of the patch? Greetings Simon, This was patterned after the move of sha256 to /lib. Upon re-inspection, it appears this has since been updated to use the __DISABLE_EXPORTS CFLAG mechanism of EXPORT_SYMBOL as a conditionally included rule in the Makefile where the desire to disable exporting is wanted. We will update this patch to follow the same pattern. V/r, Daniel P. Smith
On Thu, May 04, 2023 at 02:50:15PM +0000, Ross Philipson wrote: > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > The SHA algorithms are necessary to measure configuration information into > the TPM as early as possible before using the values. This implementation > uses the established approach of #including the SHA libraries directly in > the code since the compressed kernel is not uncompressed at this point. > > The SHA code here has its origins in the code from the main kernel: > > commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1") > > That code could not be pulled directly into the setup portion of the > compressed kernel because of other dependencies it pulls in. The result > is this is a modified copy of that code that still leverages the core > SHA algorithms. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > Signed-off-by: Ross Philipson <ross.philipson@oracle.com> SHA-1 is insecure. Why are you still using SHA-1? Don't TPMs support SHA-2 now? And if you absolutely MUST use SHA-1 despite it being insecure, please at least don't obfuscate it by calling it simply "SHA". - Eric
On Wed May 10, 2023 at 4:21 AM EEST, Eric Biggers wrote: > On Thu, May 04, 2023 at 02:50:15PM +0000, Ross Philipson wrote: > > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > > > The SHA algorithms are necessary to measure configuration information into > > the TPM as early as possible before using the values. This implementation > > uses the established approach of #including the SHA libraries directly in > > the code since the compressed kernel is not uncompressed at this point. > > > > The SHA code here has its origins in the code from the main kernel: > > > > commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1") > > > > That code could not be pulled directly into the setup portion of the > > compressed kernel because of other dependencies it pulls in. The result > > is this is a modified copy of that code that still leverages the core > > SHA algorithms. > > > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > > Signed-off-by: Ross Philipson <ross.philipson@oracle.com> > > SHA-1 is insecure. Why are you still using SHA-1? Don't TPMs support SHA-2 > now? > > And if you absolutely MUST use SHA-1 despite it being insecure, please at least > don't obfuscate it by calling it simply "SHA". AFAIK the TCG specs require for any TPM2 implementation to support both SHA-1 and SHA-256, so this as a new feature should lock in to the latter. BR, Jarkko
Ross Philipson <ross.philipson@oracle.com> wrote: > > +static void __sha_transform(u32 *digest, const char *data) > +{ > + u32 ws[SHA1_WORKSPACE_WORDS]; > + > + sha1_transform(digest, data, ws); > + > + memzero_explicit(ws, sizeof(ws)); > +} > + > +void early_sha1_init(struct sha1_state *sctx) > +{ > + sha1_init(sctx->state); > + sctx->count = 0; > +} > + > +void early_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; > + > + __sha_transform(sctx->state, sctx->buffer); > + } > + > + blocks = len / SHA1_BLOCK_SIZE; > + len %= SHA1_BLOCK_SIZE; > + > + if (blocks) { > + while (blocks--) { > + __sha_transform(sctx->state, data); > + data += SHA1_BLOCK_SIZE; > + } > + } > + partial = 0; > + } > + > + if (len) > + memcpy(sctx->buffer + partial, data, len); > +} > + > +void early_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; > + > + __sha_transform(sctx->state, sctx->buffer); > + } > + > + memset(sctx->buffer + partial, 0x0, bit_offset - partial); > + *bits = cpu_to_be64(sctx->count << 3); > + __sha_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){}; > +} If we're going to add SHA1 then this should go into lib/crypto just like SHA2. Thanks,
On Tue, May 09, 2023 at 06:21:44PM -0700, Eric Biggers wrote: > SHA-1 is insecure. Why are you still using SHA-1? Don't TPMs support SHA-2 > now? TXT is supported on some TPM 1.2 systems as well. TPM 2 systems are also at the whim of the firmware in terms of whether the SHA-2 banks are enabled. But even if the SHA-2 banks are enabled, if you suddenly stop extending the SHA-1 banks, a malicious actor can later turn up and extend whatever they want into them and present a SHA-1-only attestation. Ideally whatever is handling that attestation should know whether or not to expect an attestation with SHA-2, but the easiest way to maintain security is to always extend all banks.
On Fri, 12 May 2023 at 13:04, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > On Tue, May 09, 2023 at 06:21:44PM -0700, Eric Biggers wrote: > > > SHA-1 is insecure. Why are you still using SHA-1? Don't TPMs support SHA-2 > > now? > > TXT is supported on some TPM 1.2 systems as well. TPM 2 systems are also > at the whim of the firmware in terms of whether the SHA-2 banks are > enabled. But even if the SHA-2 banks are enabled, if you suddenly stop > extending the SHA-1 banks, a malicious actor can later turn up and > extend whatever they want into them and present a SHA-1-only > attestation. Ideally whatever is handling that attestation should know > whether or not to expect an attestation with SHA-2, but the easiest way > to maintain security is to always extend all banks. > Wouldn't it make more sense to measure some terminating event into the SHA-1 banks instead?
On Fri, May 12, 2023 at 01:18:45PM +0200, Ard Biesheuvel wrote: > On Fri, 12 May 2023 at 13:04, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > > > On Tue, May 09, 2023 at 06:21:44PM -0700, Eric Biggers wrote: > > > > > SHA-1 is insecure. Why are you still using SHA-1? Don't TPMs support SHA-2 > > > now? > > > > TXT is supported on some TPM 1.2 systems as well. TPM 2 systems are also > > at the whim of the firmware in terms of whether the SHA-2 banks are > > enabled. But even if the SHA-2 banks are enabled, if you suddenly stop > > extending the SHA-1 banks, a malicious actor can later turn up and > > extend whatever they want into them and present a SHA-1-only > > attestation. Ideally whatever is handling that attestation should know > > whether or not to expect an attestation with SHA-2, but the easiest way > > to maintain security is to always extend all banks. > > > > Wouldn't it make more sense to measure some terminating event into the > SHA-1 banks instead? Unless we assert that SHA-1 events are unsupported, it seems a bit odd to force a policy on people who have both banks enabled. People with mixed fleets are potentially going to be dealing with SHA-1 measurements for a while yet, and while there's obviously a security benefit in using SHA-2 instead it'd be irritating to have to maintain two attestation policies.
On Fri, 12 May 2023 at 13:28, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > On Fri, May 12, 2023 at 01:18:45PM +0200, Ard Biesheuvel wrote: > > On Fri, 12 May 2023 at 13:04, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > > > > > On Tue, May 09, 2023 at 06:21:44PM -0700, Eric Biggers wrote: > > > > > > > SHA-1 is insecure. Why are you still using SHA-1? Don't TPMs support SHA-2 > > > > now? > > > > > > TXT is supported on some TPM 1.2 systems as well. TPM 2 systems are also > > > at the whim of the firmware in terms of whether the SHA-2 banks are > > > enabled. But even if the SHA-2 banks are enabled, if you suddenly stop > > > extending the SHA-1 banks, a malicious actor can later turn up and > > > extend whatever they want into them and present a SHA-1-only > > > attestation. Ideally whatever is handling that attestation should know > > > whether or not to expect an attestation with SHA-2, but the easiest way > > > to maintain security is to always extend all banks. > > > > > > > Wouldn't it make more sense to measure some terminating event into the > > SHA-1 banks instead? > > Unless we assert that SHA-1 events are unsupported, it seems a bit odd > to force a policy on people who have both banks enabled. People with > mixed fleets are potentially going to be dealing with SHA-1 measurements > for a while yet, and while there's obviously a security benefit in using > SHA-2 instead it'd be irritating to have to maintain two attestation > policies. I understand why that matters from an operational perspective. However, we are dealing with brand new code being proposed for Linux mainline, and so this is our only chance to push back on this, as otherwise, we will have to maintain it for a very long time. IOW, D-RTM does not exist today in Linux, and it is up to us to define what it will look like. From that perspective, it is downright preposterous to even consider supporting SHA-1, given that SHA-1 by itself gives none of the guarantees that D-RTM aims to provide. If reducing your TCB is important enough to warrant switching to this implementation of D-RTM, surely you can upgrade your attestation policies as well.
On 12/05/2023 12:58 pm, Ard Biesheuvel wrote: > On Fri, 12 May 2023 at 13:28, Matthew Garrett <mjg59@srcf.ucam.org> wrote: >> On Fri, May 12, 2023 at 01:18:45PM +0200, Ard Biesheuvel wrote: >>> On Fri, 12 May 2023 at 13:04, Matthew Garrett <mjg59@srcf.ucam.org> wrote: >>>> On Tue, May 09, 2023 at 06:21:44PM -0700, Eric Biggers wrote: >>>> >>>>> SHA-1 is insecure. Why are you still using SHA-1? Don't TPMs support SHA-2 >>>>> now? >>>> TXT is supported on some TPM 1.2 systems as well. TPM 2 systems are also >>>> at the whim of the firmware in terms of whether the SHA-2 banks are >>>> enabled. But even if the SHA-2 banks are enabled, if you suddenly stop >>>> extending the SHA-1 banks, a malicious actor can later turn up and >>>> extend whatever they want into them and present a SHA-1-only >>>> attestation. Ideally whatever is handling that attestation should know >>>> whether or not to expect an attestation with SHA-2, but the easiest way >>>> to maintain security is to always extend all banks. >>>> >>> Wouldn't it make more sense to measure some terminating event into the >>> SHA-1 banks instead? >> Unless we assert that SHA-1 events are unsupported, it seems a bit odd >> to force a policy on people who have both banks enabled. People with >> mixed fleets are potentially going to be dealing with SHA-1 measurements >> for a while yet, and while there's obviously a security benefit in using >> SHA-2 instead it'd be irritating to have to maintain two attestation >> policies. > I understand why that matters from an operational perspective. > > However, we are dealing with brand new code being proposed for Linux > mainline, and so this is our only chance to push back on this, as > otherwise, we will have to maintain it for a very long time. > > IOW, D-RTM does not exist today in Linux, and it is up to us to define > what it will look like. From that perspective, it is downright > preposterous to even consider supporting SHA-1, given that SHA-1 by > itself gives none of the guarantees that D-RTM aims to provide. If > reducing your TCB is important enough to warrant switching to this > implementation of D-RTM, surely you can upgrade your attestation > policies as well. You're suggesting that because Linux has been slow to take D-RTM over the past decade, you're going to intentionally break people with older hardware just because you don't feel like using an older algorithm? That's about the worst possible reason to not take support. There really are people in the world with older TPM 1.2 systems where this D-RTM using SHA1 only is an improvement over using the incumbent tboot. ~Andrew
On Fri, May 12 2023 at 12:28, Matthew Garrett wrote: > On Fri, May 12, 2023 at 01:18:45PM +0200, Ard Biesheuvel wrote: >> On Fri, 12 May 2023 at 13:04, Matthew Garrett <mjg59@srcf.ucam.org> wrote: >> > >> > On Tue, May 09, 2023 at 06:21:44PM -0700, Eric Biggers wrote: >> > >> > > SHA-1 is insecure. Why are you still using SHA-1? Don't TPMs support SHA-2 >> > > now? >> > >> > TXT is supported on some TPM 1.2 systems as well. TPM 2 systems are also >> > at the whim of the firmware in terms of whether the SHA-2 banks are >> > enabled. But even if the SHA-2 banks are enabled, if you suddenly stop >> > extending the SHA-1 banks, a malicious actor can later turn up and >> > extend whatever they want into them and present a SHA-1-only >> > attestation. Ideally whatever is handling that attestation should know >> > whether or not to expect an attestation with SHA-2, but the easiest way >> > to maintain security is to always extend all banks. >> > >> >> Wouldn't it make more sense to measure some terminating event into the >> SHA-1 banks instead? > > Unless we assert that SHA-1 events are unsupported, it seems a bit odd > to force a policy on people who have both banks enabled. People with > mixed fleets are potentially going to be dealing with SHA-1 measurements > for a while yet, and while there's obviously a security benefit in using > SHA-2 instead it'd be irritating to have to maintain two attestation > policies. Why? If you have a mixed fleet then it's not too much asked to provide two data sets. On a TPM2 system you can enforce SHA-2 and only fallback to SHA-1 on TPM 1.2 hardware. No? Thanks, tglx
On Fri, May 12, 2023 at 03:24:04PM +0200, Thomas Gleixner wrote: > On Fri, May 12 2023 at 12:28, Matthew Garrett wrote: > > Unless we assert that SHA-1 events are unsupported, it seems a bit odd > > to force a policy on people who have both banks enabled. People with > > mixed fleets are potentially going to be dealing with SHA-1 measurements > > for a while yet, and while there's obviously a security benefit in using > > SHA-2 instead it'd be irritating to have to maintain two attestation > > policies. > > Why? > > If you have a mixed fleet then it's not too much asked to provide two > data sets. On a TPM2 system you can enforce SHA-2 and only fallback to > SHA-1 on TPM 1.2 hardware. No? No, beause having TPM2 hardware doesn't guarantee that your firmware enables SHA-2 (which also means this is something that could change with firmware updates, which means that refusing to support SHA-1 if the SHA-2 banks are enabled could result in an entirely different policy being required (and plausibly one that isn't implemented in their existing tooling)
On Fri, May 12 2023 at 17:13, Matthew Garrett wrote: > On Fri, May 12, 2023 at 03:24:04PM +0200, Thomas Gleixner wrote: >> On Fri, May 12 2023 at 12:28, Matthew Garrett wrote: >> > Unless we assert that SHA-1 events are unsupported, it seems a bit odd >> > to force a policy on people who have both banks enabled. People with >> > mixed fleets are potentially going to be dealing with SHA-1 measurements >> > for a while yet, and while there's obviously a security benefit in using >> > SHA-2 instead it'd be irritating to have to maintain two attestation >> > policies. >> >> Why? >> >> If you have a mixed fleet then it's not too much asked to provide two >> data sets. On a TPM2 system you can enforce SHA-2 and only fallback to >> SHA-1 on TPM 1.2 hardware. No? > > No, beause having TPM2 hardware doesn't guarantee that your firmware > enables SHA-2 (which also means this is something that could change with > firmware updates, which means that refusing to support SHA-1 if the > SHA-2 banks are enabled could result in an entirely different policy > being required (and plausibly one that isn't implemented in their > existing tooling) It's not rocket science to have both variants supported in tooling, really. What a mess.
On Fri, May 12, 2023 at 08:17:21PM +0200, Thomas Gleixner wrote: > On Fri, May 12 2023 at 17:13, Matthew Garrett wrote: > > On Fri, May 12, 2023 at 03:24:04PM +0200, Thomas Gleixner wrote: > >> On Fri, May 12 2023 at 12:28, Matthew Garrett wrote: > >> > Unless we assert that SHA-1 events are unsupported, it seems a bit odd > >> > to force a policy on people who have both banks enabled. People with > >> > mixed fleets are potentially going to be dealing with SHA-1 measurements > >> > for a while yet, and while there's obviously a security benefit in using > >> > SHA-2 instead it'd be irritating to have to maintain two attestation > >> > policies. > >> > >> Why? > >> > >> If you have a mixed fleet then it's not too much asked to provide two > >> data sets. On a TPM2 system you can enforce SHA-2 and only fallback to > >> SHA-1 on TPM 1.2 hardware. No? > > > > No, beause having TPM2 hardware doesn't guarantee that your firmware > > enables SHA-2 (which also means this is something that could change with > > firmware updates, which means that refusing to support SHA-1 if the > > SHA-2 banks are enabled could result in an entirely different policy > > being required (and plausibly one that isn't implemented in their > > existing tooling) > > It's not rocket science to have both variants supported in tooling, > really. People who are currently using tboot are only getting SHA-1, so there's no obvious reason for them to have added support yet. *My* tooling all supports SHA-2 so I'm completely fine here, but either we refuse to support a bunch of hardware or we have to support SHA-1 anyway, and if we have to support it the only reason not to implement it even in the "SHA-2 is supported" case is because we have opinions about how other people implement their security.
On 12/05/2023 8:12 pm, Matthew Garrett wrote: > On Fri, May 12, 2023 at 08:17:21PM +0200, Thomas Gleixner wrote: >> On Fri, May 12 2023 at 17:13, Matthew Garrett wrote: >>> On Fri, May 12, 2023 at 03:24:04PM +0200, Thomas Gleixner wrote: >>>> On Fri, May 12 2023 at 12:28, Matthew Garrett wrote: >>>>> Unless we assert that SHA-1 events are unsupported, it seems a bit odd >>>>> to force a policy on people who have both banks enabled. People with >>>>> mixed fleets are potentially going to be dealing with SHA-1 measurements >>>>> for a while yet, and while there's obviously a security benefit in using >>>>> SHA-2 instead it'd be irritating to have to maintain two attestation >>>>> policies. >>>> Why? >>>> >>>> If you have a mixed fleet then it's not too much asked to provide two >>>> data sets. On a TPM2 system you can enforce SHA-2 and only fallback to >>>> SHA-1 on TPM 1.2 hardware. No? >>> No, beause having TPM2 hardware doesn't guarantee that your firmware >>> enables SHA-2 (which also means this is something that could change with >>> firmware updates, which means that refusing to support SHA-1 if the >>> SHA-2 banks are enabled could result in an entirely different policy >>> being required (and plausibly one that isn't implemented in their >>> existing tooling) >> It's not rocket science to have both variants supported in tooling, >> really. > People who are currently using tboot are only getting SHA-1, so there's > no obvious reason for them to have added support yet. *My* tooling all > supports SHA-2 so I'm completely fine here, but either we refuse to > support a bunch of hardware or we have to support SHA-1 anyway, and if > we have to support it the only reason not to implement it even in the > "SHA-2 is supported" case is because we have opinions about how other > people implement their security. The way to deal with this is to merge DRTM support (when it's ready of course) so people have an option which isn't tboot. Then warn on finding a TPM2 without SHA-2, and make it a failure for https://fwupd.github.io/libfwupdplugin/hsi.html#tpm-20-present etc, and eventually the vendors will decide that the easiest way to avoid getting a cross in their customers UIs is to implement SHA-2 support properly. ~Andrew
On Fri, May 12, 2023 at 01:24:22PM +0100, Andrew Cooper wrote: > On 12/05/2023 12:58 pm, Ard Biesheuvel wrote: > > On Fri, 12 May 2023 at 13:28, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > >> On Fri, May 12, 2023 at 01:18:45PM +0200, Ard Biesheuvel wrote: > >>> On Fri, 12 May 2023 at 13:04, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > >>>> On Tue, May 09, 2023 at 06:21:44PM -0700, Eric Biggers wrote: > >>>> > >>>>> SHA-1 is insecure. Why are you still using SHA-1? Don't TPMs support SHA-2 > >>>>> now? > >>>> TXT is supported on some TPM 1.2 systems as well. TPM 2 systems are also > >>>> at the whim of the firmware in terms of whether the SHA-2 banks are > >>>> enabled. But even if the SHA-2 banks are enabled, if you suddenly stop > >>>> extending the SHA-1 banks, a malicious actor can later turn up and > >>>> extend whatever they want into them and present a SHA-1-only > >>>> attestation. Ideally whatever is handling that attestation should know > >>>> whether or not to expect an attestation with SHA-2, but the easiest way > >>>> to maintain security is to always extend all banks. > >>>> > >>> Wouldn't it make more sense to measure some terminating event into the > >>> SHA-1 banks instead? > >> Unless we assert that SHA-1 events are unsupported, it seems a bit odd > >> to force a policy on people who have both banks enabled. People with > >> mixed fleets are potentially going to be dealing with SHA-1 measurements > >> for a while yet, and while there's obviously a security benefit in using > >> SHA-2 instead it'd be irritating to have to maintain two attestation > >> policies. > > I understand why that matters from an operational perspective. > > > > However, we are dealing with brand new code being proposed for Linux > > mainline, and so this is our only chance to push back on this, as > > otherwise, we will have to maintain it for a very long time. > > > > IOW, D-RTM does not exist today in Linux, and it is up to us to define > > what it will look like. From that perspective, it is downright > > preposterous to even consider supporting SHA-1, given that SHA-1 by > > itself gives none of the guarantees that D-RTM aims to provide. If > > reducing your TCB is important enough to warrant switching to this > > implementation of D-RTM, surely you can upgrade your attestation > > policies as well. > > You're suggesting that because Linux has been slow to take D-RTM over > the past decade, you're going to intentionally break people with older > hardware just because you don't feel like using an older algorithm? > > That's about the worst possible reason to not take support. > > There really are people in the world with older TPM 1.2 systems where > this D-RTM using SHA1 only is an improvement over using the incumbent tboot. > > ~Andrew This patchset is proposing a new kernel feature. So by definition, there are no existing users of it that can be broken. The fact is, SHA-1 is cryptographically broken. It isn't actually about how "old" the algorithm is, or what anyone's "feelings" are. Maybe a renaming from Secure Launch to simply Launch is in order? - Eric
On Sun, May 14, 2023 at 11:18:17AM -0700, Eric Biggers wrote: > On Fri, May 12, 2023 at 01:24:22PM +0100, Andrew Cooper wrote: > > You're suggesting that because Linux has been slow to take D-RTM over > > the past decade, you're going to intentionally break people with older > > hardware just because you don't feel like using an older algorithm? > > > > That's about the worst possible reason to not take support. > > > > There really are people in the world with older TPM 1.2 systems where > > this D-RTM using SHA1 only is an improvement over using the incumbent tboot. > > > > ~Andrew > > This patchset is proposing a new kernel feature. So by definition, there are no > existing users of it that can be broken. The patchset reimplements a more extensible version of an existing feature which people already consume, and presumably people will be encouraged to transition to it. There is plenty of hardware that supports this feature that only implements SHA-1. If you want to propose that the kernel not implement any functionality that uses deprecated hash algorithms then that seems like a larger conversation rather than one that should focus on a single pachset.
On 5/9/23 21:21, Eric Biggers wrote: > On Thu, May 04, 2023 at 02:50:15PM +0000, Ross Philipson wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> The SHA algorithms are necessary to measure configuration information into >> the TPM as early as possible before using the values. This implementation >> uses the established approach of #including the SHA libraries directly in >> the code since the compressed kernel is not uncompressed at this point. >> >> The SHA code here has its origins in the code from the main kernel: >> >> commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1") >> >> That code could not be pulled directly into the setup portion of the >> compressed kernel because of other dependencies it pulls in. The result >> is this is a modified copy of that code that still leverages the core >> SHA algorithms. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> Signed-off-by: Ross Philipson <ross.philipson@oracle.com> > > SHA-1 is insecure. Why are you still using SHA-1? Don't TPMs support SHA-2 > now? I think others have commented as to why SHA-1 is provided. > And if you absolutely MUST use SHA-1 despite it being insecure, please at least > don't obfuscate it by calling it simply "SHA". Apologies that it appears that way to you. Typically when referring to the family or a subset of the SHA algorithms, SHA-0, SHA-1, SHA-2, and SHA-3, it is generally accepted to refer to them as the "SHA algorithms". And this is contrasted to the SM algorithms which the TCG spec provides for which we have no intentions to support ourselves, though others are welcome to contribute. Again, apologies for misunderstanding and thank you for taking the time to review the series. v/r, dps
On 5/10/23 23:33, Herbert Xu wrote: > Ross Philipson <ross.philipson@oracle.com> wrote: >> >> +static void __sha_transform(u32 *digest, const char *data) >> +{ >> + u32 ws[SHA1_WORKSPACE_WORDS]; >> + >> + sha1_transform(digest, data, ws); >> + >> + memzero_explicit(ws, sizeof(ws)); >> +} >> + >> +void early_sha1_init(struct sha1_state *sctx) >> +{ >> + sha1_init(sctx->state); >> + sctx->count = 0; >> +} >> + >> +void early_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; >> + >> + __sha_transform(sctx->state, sctx->buffer); >> + } >> + >> + blocks = len / SHA1_BLOCK_SIZE; >> + len %= SHA1_BLOCK_SIZE; >> + >> + if (blocks) { >> + while (blocks--) { >> + __sha_transform(sctx->state, data); >> + data += SHA1_BLOCK_SIZE; >> + } >> + } >> + partial = 0; >> + } >> + >> + if (len) >> + memcpy(sctx->buffer + partial, data, len); >> +} >> + >> +void early_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; >> + >> + __sha_transform(sctx->state, sctx->buffer); >> + } >> + >> + memset(sctx->buffer + partial, 0x0, bit_offset - partial); >> + *bits = cpu_to_be64(sctx->count << 3); >> + __sha_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){}; >> +} > > If we're going to add SHA1 then this should go into lib/crypto > just like SHA2. As mentioned before, this patch mimicked an early version for SHA2. We were remiss in not keeping it aligned with how the SHA2 evolved. I will take a closer look, but these wrappers may be able to go away and be reduced to just an include as SHA2 does these days. v/r, dps
diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile index 6b6cfe6..1d327d4 100644 --- a/arch/x86/boot/compressed/Makefile +++ b/arch/x86/boot/compressed/Makefile @@ -112,6 +112,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)/early_sha256.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 0000000..524ec23 --- /dev/null +++ b/arch/x86/boot/compressed/early_sha1.c @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2022 Apertus Solutions, LLC. + */ + +#include <linux/init.h> +#include <linux/linkage.h> +#include <linux/string.h> +#include <asm/boot.h> +#include <asm/unaligned.h> + +#include "early_sha1.h" + +#define SHA1_DISABLE_EXPORT +#include "../../../../lib/crypto/sha1.c" + +/* The SHA1 implementation in lib/sha1.c was written to get the workspace + * buffer as a parameter. This wrapper function provides a container + * around a temporary workspace that is cleared after the transform completes. + */ +static void __sha_transform(u32 *digest, const char *data) +{ + u32 ws[SHA1_WORKSPACE_WORDS]; + + sha1_transform(digest, data, ws); + + memzero_explicit(ws, sizeof(ws)); +} + +void early_sha1_init(struct sha1_state *sctx) +{ + sha1_init(sctx->state); + sctx->count = 0; +} + +void early_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; + + __sha_transform(sctx->state, sctx->buffer); + } + + blocks = len / SHA1_BLOCK_SIZE; + len %= SHA1_BLOCK_SIZE; + + if (blocks) { + while (blocks--) { + __sha_transform(sctx->state, data); + data += SHA1_BLOCK_SIZE; + } + } + partial = 0; + } + + if (len) + memcpy(sctx->buffer + partial, data, len); +} + +void early_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; + + __sha_transform(sctx->state, sctx->buffer); + } + + memset(sctx->buffer + partial, 0x0, bit_offset - partial); + *bits = cpu_to_be64(sctx->count << 3); + __sha_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){}; +} diff --git a/arch/x86/boot/compressed/early_sha1.h b/arch/x86/boot/compressed/early_sha1.h new file mode 100644 index 0000000..adcc4a9 --- /dev/null +++ b/arch/x86/boot/compressed/early_sha1.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (c) 2022 Apertus Solutions, LLC + */ + +#ifndef BOOT_COMPRESSED_EARLY_SHA1_H +#define BOOT_COMPRESSED_EARLY_SHA1_H + +#include <crypto/sha1.h> + +void early_sha1_init(struct sha1_state *sctx); +void early_sha1_update(struct sha1_state *sctx, + const u8 *data, + unsigned int len); +void early_sha1_final(struct sha1_state *sctx, u8 *out); + +#endif /* BOOT_COMPRESSED_EARLY_SHA1_H */ diff --git a/arch/x86/boot/compressed/early_sha256.c b/arch/x86/boot/compressed/early_sha256.c new file mode 100644 index 0000000..28a8e32 --- /dev/null +++ b/arch/x86/boot/compressed/early_sha256.c @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2022 Apertus Solutions, LLC + */ + +#define SHA256_DISABLE_EXPORT +#include "../../../../lib/crypto/sha256.c" diff --git a/lib/crypto/sha1.c b/lib/crypto/sha1.c index 1aebe7b..771ff90 100644 --- a/lib/crypto/sha1.c +++ b/lib/crypto/sha1.c @@ -121,7 +121,9 @@ void sha1_transform(__u32 *digest, const char *data, __u32 *array) digest[3] += D; digest[4] += E; } +#ifndef SHA1_DISABLE_EXPORT EXPORT_SYMBOL(sha1_transform); +#endif /** * sha1_init - initialize the vectors for a SHA1 digest @@ -135,6 +137,8 @@ void sha1_init(__u32 *buf) buf[3] = 0x10325476; buf[4] = 0xc3d2e1f0; } +#ifndef SHA1_DISABLE_EXPORT EXPORT_SYMBOL(sha1_init); +#endif MODULE_LICENSE("GPL"); diff --git a/lib/crypto/sha256.c b/lib/crypto/sha256.c index 72a4b0b..e532220 100644 --- a/lib/crypto/sha256.c +++ b/lib/crypto/sha256.c @@ -149,13 +149,17 @@ void sha256_update(struct sha256_state *sctx, const u8 *data, unsigned int len) } memcpy(sctx->buf + partial, src, len - done); } +#ifndef SHA256_DISABLE_EXPORT EXPORT_SYMBOL(sha256_update); +#endif void sha224_update(struct sha256_state *sctx, const u8 *data, unsigned int len) { sha256_update(sctx, data, len); } +#ifndef SHA256_DISABLE_EXPORT EXPORT_SYMBOL(sha224_update); +#endif static void __sha256_final(struct sha256_state *sctx, u8 *out, int digest_words) { @@ -188,13 +192,17 @@ void sha256_final(struct sha256_state *sctx, u8 *out) { __sha256_final(sctx, out, 8); } +#ifndef SHA256_DISABLE_EXPORT EXPORT_SYMBOL(sha256_final); +#endif void sha224_final(struct sha256_state *sctx, u8 *out) { __sha256_final(sctx, out, 7); } +#ifndef SHA256_DISABLE_EXPORT EXPORT_SYMBOL(sha224_final); +#endif void sha256(const u8 *data, unsigned int len, u8 *out) {