From: "Daniel P. Smith" <[email protected]>
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 <[email protected]>
Signed-off-by: Ross Philipson <[email protected]>
---
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
+#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)
{
--
1.8.3.1
On Thu, May 04, 2023 at 02:50:15PM +0000, Ross Philipson wrote:
> From: "Daniel P. Smith" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Ross Philipson <[email protected]>
> ---
> 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" <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Ross Philipson <[email protected]>
>> ---
>> 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" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Ross Philipson <[email protected]>
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" <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Ross Philipson <[email protected]>
>
> 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 <[email protected]> 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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>
> On Fri, May 12, 2023 at 01:18:45PM +0200, Ard Biesheuvel wrote:
> > On Fri, 12 May 2023 at 13:04, Matthew Garrett <[email protected]> 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 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 <[email protected]> wrote:
> >> On Fri, May 12, 2023 at 01:18:45PM +0200, Ard Biesheuvel wrote:
> >>> On Fri, 12 May 2023 at 13:04, Matthew Garrett <[email protected]> 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" <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Ross Philipson <[email protected]>
>
> 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 <[email protected]> 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