diff mbox series

[v6,06/14] x86: Add early SHA support for Secure Launch early measurements

Message ID 20230504145023.835096-7-ross.philipson@oracle.com
State New
Headers
Series x86: Trenchboot secure dynamic launch Linux kernel support |

Commit Message

Ross Philipson May 4, 2023, 2:50 p.m. UTC 
  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

Comments

Simon Horman May 5, 2023, 4:34 p.m. UTC | #1 
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?

...
Daniel P. Smith May 9, 2023, 4:09 p.m. UTC | #2 
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
Eric Biggers May 10, 2023, 1:21 a.m. UTC | #3 
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
Jarkko Sakkinen May 10, 2023, 10:28 p.m. UTC | #4 
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
Herbert Xu May 11, 2023, 3:33 a.m. UTC | #5 
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,
Matthew Garrett May 12, 2023, 11:04 a.m. UTC | #6 
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.
Ard Biesheuvel May 12, 2023, 11:18 a.m. UTC | #7 
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?
Matthew Garrett May 12, 2023, 11:28 a.m. UTC | #8 
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.
Ard Biesheuvel May 12, 2023, 11:58 a.m. UTC | #9 
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.
Thomas Gleixner May 12, 2023, 1:24 p.m. UTC | #10 
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
Matthew Garrett May 12, 2023, 4:13 p.m. UTC | #11 
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)
Thomas Gleixner May 12, 2023, 6:17 p.m. UTC | #12 
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.
Matthew Garrett May 12, 2023, 7:12 p.m. UTC | #13 
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.
Andrew Cooper May 12, 2023, 7:42 p.m. UTC | #14 
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
Eric Biggers May 14, 2023, 6:18 p.m. UTC | #15 
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
Matthew Garrett May 14, 2023, 7:11 p.m. UTC | #16 
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.
Daniel P. Smith May 15, 2023, 9:23 p.m. UTC | #17 
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
Daniel P. Smith May 16, 2023, 12:50 a.m. UTC | #18 
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 mbox series

Patch

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)
 {