From: Eric Biggers ebiggers@google.com
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: stable@vger.kernel.org # v4.20+ Signed-off-by: Eric Biggers ebiggers@google.com --- 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;
On Thu, Jan 13, 2022 at 03:54:38PM -0800, Eric Biggers wrote:
From: Eric Biggers ebiggers@google.com
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: stable@vger.kernel.org # v4.20+ Signed-off-by: Eric Biggers ebiggers@google.com
BR, Jarkko
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
On Tue, Jan 18, 2022 at 04:59:47PM -0800, Eric Biggers wrote:
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.
I would consider dropping asym_tpm as it has no practical use cases existing.
/Jarkko
On Wed, Jan 26, 2022 at 04:21:53PM +0200, Jarkko Sakkinen wrote:
On Tue, Jan 18, 2022 at 04:59:47PM -0800, Eric Biggers wrote:
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.
I would consider dropping asym_tpm as it has no practical use cases existing.
At least I have zero motivation to maintain it as it does not meet any quality standards and is based on insecure crypto algorithms. I neither have participated to its review process.
/Jarkko
On Wed, Jan 26, 2022 at 04:22:53PM +0200, Jarkko Sakkinen wrote:
On Wed, Jan 26, 2022 at 04:21:53PM +0200, Jarkko Sakkinen wrote:
On Tue, Jan 18, 2022 at 04:59:47PM -0800, Eric Biggers wrote:
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.
I would consider dropping asym_tpm as it has no practical use cases existing.
At least I have zero motivation to maintain it as it does not meet any quality standards and is based on insecure crypto algorithms. I neither have participated to its review process.
Fair enough, I'll send a patch to remove it then.
- Eric
On Fri, Jan 28, 2022 at 11:00:12AM -0800, Eric Biggers wrote:
On Wed, Jan 26, 2022 at 04:22:53PM +0200, Jarkko Sakkinen wrote:
On Wed, Jan 26, 2022 at 04:21:53PM +0200, Jarkko Sakkinen wrote:
On Tue, Jan 18, 2022 at 04:59:47PM -0800, Eric Biggers wrote:
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.
I would consider dropping asym_tpm as it has no practical use cases existing.
At least I have zero motivation to maintain it as it does not meet any quality standards and is based on insecure crypto algorithms. I neither have participated to its review process.
Fair enough, I'll send a patch to remove it then.
It is IMHO. I mean having this advertising insecure ways to to do crypto.
Thank you.
PS. My latency is because I've been moving to a new job. It is temporary.
/Jarkko
linux-stable-mirror@lists.linaro.org
-
Eric Biggers -
Jarkko Sakkinen