2022-01-14 14:37:31

by Eric Biggers

[permalink] [raw]
Subject: [PATCH 0/3] KEYS: fixes for asym_tpm keys

This series fixes some bugs in asym_tpm.c.

Eric Biggers (3):
KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()
KEYS: asym_tpm: fix incorrect comment
KEYS: asym_tpm: rename derive_pub_key()

crypto/asymmetric_keys/asym_tpm.c | 44 +++++++++++++++++++------------
1 file changed, 27 insertions(+), 17 deletions(-)


base-commit: feb7a43de5ef625ad74097d8fd3481d5dbc06a59
--
2.34.1


2022-01-14 14:37:31

by Eric Biggers

[permalink] [raw]
Subject: [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()

From: Eric Biggers <[email protected]>

extract_key_parameters() can read past the end of the input buffer due
to buggy and missing bounds checks. Fix it as follows:

- Before reading each key length field, verify that there are at least 4
bytes remaining.

- Avoid integer overflows when validating size fields; 'sz + 12' and
'4 + sz' overflowed if 'sz' is near U32_MAX.

- Before saving the pointer to the public key, check that it doesn't run
past the end of the buffer.

Fixes: f8c54e1ac4b8 ("KEYS: asym_tpm: extract key size & public key [ver #2]")
Cc: <[email protected]> # v4.20+
Signed-off-by: Eric Biggers <[email protected]>
---
crypto/asymmetric_keys/asym_tpm.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
index 0959613560b9..60d20d44c885 100644
--- a/crypto/asymmetric_keys/asym_tpm.c
+++ b/crypto/asymmetric_keys/asym_tpm.c
@@ -814,7 +814,6 @@ static int extract_key_parameters(struct tpm_key *tk)
{
const void *cur = tk->blob;
uint32_t len = tk->blob_len;
- const void *pub_key;
uint32_t sz;
uint32_t key_len;

@@ -845,14 +844,14 @@ static int extract_key_parameters(struct tpm_key *tk)
return -EBADMSG;

sz = get_unaligned_be32(cur + 8);
- if (len < sz + 12)
- return -EBADMSG;

/* Move to TPM_RSA_KEY_PARMS */
- len -= 12;
cur += 12;
+ len -= 12;

/* Grab the RSA key length */
+ if (len < 4)
+ return -EBADMSG;
key_len = get_unaligned_be32(cur);

switch (key_len) {
@@ -866,29 +865,36 @@ static int extract_key_parameters(struct tpm_key *tk)
}

/* Move just past TPM_KEY_PARMS */
+ if (len < sz)
+ return -EBADMSG;
cur += sz;
len -= sz;

if (len < 4)
return -EBADMSG;
-
sz = get_unaligned_be32(cur);
- if (len < 4 + sz)
- return -EBADMSG;
+ cur += 4;
+ len -= 4;

/* Move to TPM_STORE_PUBKEY */
- cur += 4 + sz;
- len -= 4 + sz;
+ if (len < sz)
+ return -EBADMSG;
+ cur += sz;
+ len -= sz;

/* Grab the size of the public key, it should jive with the key size */
+ if (len < 4)
+ return -EBADMSG;
sz = get_unaligned_be32(cur);
+ cur += 4;
+ len -= 4;
if (sz > 256)
return -EINVAL;
-
- pub_key = cur + 4;
+ if (len < sz)
+ return -EBADMSG;

tk->key_len = key_len;
- tk->pub_key = pub_key;
+ tk->pub_key = cur;
tk->pub_key_len = sz;

return 0;
--
2.34.1

2022-01-14 21:34:17

by Denis Kenzior

[permalink] [raw]
Subject: Re: [PATCH 0/3] KEYS: fixes for asym_tpm keys

Hi Eric,

On 1/13/22 17:54, Eric Biggers wrote:
> This series fixes some bugs in asym_tpm.c.
>
> Eric Biggers (3):
> KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()
> KEYS: asym_tpm: fix incorrect comment
> KEYS: asym_tpm: rename derive_pub_key()
>
> crypto/asymmetric_keys/asym_tpm.c | 44 +++++++++++++++++++------------
> 1 file changed, 27 insertions(+), 17 deletions(-)
>
>
> base-commit: feb7a43de5ef625ad74097d8fd3481d5dbc06a59
>

For the series:

Reviewed-By: Denis Kenzior <[email protected]>

Regards,
-Denis

2022-01-16 16:23:18

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()

On Thu, Jan 13, 2022 at 03:54:38PM -0800, Eric Biggers wrote:
> From: Eric Biggers <[email protected]>
>
> extract_key_parameters() can read past the end of the input buffer due
> to buggy and missing bounds checks. Fix it as follows:
>
> - Before reading each key length field, verify that there are at least 4
> bytes remaining.

Maybe start with a "Key length is described as an unsigned 32-bit integer
in the TPM header". Just for clarity.

>
> - Avoid integer overflows when validating size fields; 'sz + 12' and
> '4 + sz' overflowed if 'sz' is near U32_MAX.

So we have a struct tpm_header in include/linux/tpm.h. It would be way
more informative to use sizeof(struct tpm_header) than number 12, even
if the patch does not otherwise use the struct. It tells what it is, 12
does not.

> - Before saving the pointer to the public key, check that it doesn't run
> past the end of the buffer.
>
> Fixes: f8c54e1ac4b8 ("KEYS: asym_tpm: extract key size & public key [ver #2]")
> Cc: <[email protected]> # v4.20+
> Signed-off-by: Eric Biggers <[email protected]>

BR, Jarkko

2022-01-16 16:23:20

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH 0/3] KEYS: fixes for asym_tpm keys

On Thu, Jan 13, 2022 at 03:54:37PM -0800, Eric Biggers wrote:
> This series fixes some bugs in asym_tpm.c.
>
> Eric Biggers (3):
> KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()
> KEYS: asym_tpm: fix incorrect comment
> KEYS: asym_tpm: rename derive_pub_key()
>
> crypto/asymmetric_keys/asym_tpm.c | 44 +++++++++++++++++++------------
> 1 file changed, 27 insertions(+), 17 deletions(-)
>
>
> base-commit: feb7a43de5ef625ad74097d8fd3481d5dbc06a59
> --
> 2.34.1
>

I really appreciate your effort to go and fix this key type. Thank you.

BR, Jarkko

2022-01-21 13:10:31

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()

On Sat, Jan 15, 2022 at 11:40:48PM +0200, Jarkko Sakkinen wrote:
> >
> > - Avoid integer overflows when validating size fields; 'sz + 12' and
> > '4 + sz' overflowed if 'sz' is near U32_MAX.
>
> So we have a struct tpm_header in include/linux/tpm.h. It would be way
> more informative to use sizeof(struct tpm_header) than number 12, even
> if the patch does not otherwise use the struct. It tells what it is, 12
> does not.

I don't think that would be an improvement, given that the code is using
hard-coded offsets. If it's reading 4 bytes from cur + 8, it's much easier to
understand that it needs 12 bytes than 'sizeof(struct tpm_header)' bytes.

I'd certainly encourage whoever is maintaining this code to change it to use
structs instead, but that's not what this patch is meant to do.

- Eric