stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()
       [not found] <20220113235440.90439-1-ebiggers@kernel.org>
@ 2022-01-13 23:54 ` Eric Biggers
  2022-01-15 21:40   ` Jarkko Sakkinen
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Biggers @ 2022-01-13 23:54 UTC (permalink / raw)
  To: keyrings, David Howells, Jarkko Sakkinen
  Cc: Denis Kenzior, Marcel Holtmann, James Morris, linux-crypto, stable

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;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()
  2022-01-13 23:54 ` [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters() Eric Biggers
@ 2022-01-15 21:40   ` Jarkko Sakkinen
  2022-01-19  0:59     ` Eric Biggers
  0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Sakkinen @ 2022-01-15 21:40 UTC (permalink / raw)
  To: Eric Biggers
  Cc: keyrings, David Howells, Denis Kenzior, Marcel Holtmann,
	James Morris, linux-crypto, stable

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()
  2022-01-15 21:40   ` Jarkko Sakkinen
@ 2022-01-19  0:59     ` Eric Biggers
  2022-01-26 14:21       ` Jarkko Sakkinen
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Biggers @ 2022-01-19  0:59 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: keyrings, David Howells, Denis Kenzior, Marcel Holtmann,
	James Morris, linux-crypto, stable

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()
  2022-01-19  0:59     ` Eric Biggers
@ 2022-01-26 14:21       ` Jarkko Sakkinen
  2022-01-26 14:22         ` Jarkko Sakkinen
  0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Sakkinen @ 2022-01-26 14:21 UTC (permalink / raw)
  To: Eric Biggers
  Cc: keyrings, David Howells, Denis Kenzior, Marcel Holtmann,
	James Morris, linux-crypto, stable

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()
  2022-01-26 14:21       ` Jarkko Sakkinen
@ 2022-01-26 14:22         ` Jarkko Sakkinen
  2022-01-28 19:00           ` Eric Biggers
  0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Sakkinen @ 2022-01-26 14:22 UTC (permalink / raw)
  To: Eric Biggers
  Cc: keyrings, David Howells, Denis Kenzior, Marcel Holtmann,
	James Morris, linux-crypto, stable

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()
  2022-01-26 14:22         ` Jarkko Sakkinen
@ 2022-01-28 19:00           ` Eric Biggers
  2022-02-08  9:30             ` Jarkko Sakkinen
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Biggers @ 2022-01-28 19:00 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: keyrings, Denis Kenzior, David Howells, Marcel Holtmann,
	James Morris, linux-crypto, stable

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters()
  2022-01-28 19:00           ` Eric Biggers
@ 2022-02-08  9:30             ` Jarkko Sakkinen
  0 siblings, 0 replies; 7+ messages in thread
From: Jarkko Sakkinen @ 2022-02-08  9:30 UTC (permalink / raw)
  To: Eric Biggers
  Cc: keyrings, Denis Kenzior, David Howells, Marcel Holtmann,
	James Morris, linux-crypto, stable

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-02-08  9:30 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20220113235440.90439-1-ebiggers@kernel.org>
2022-01-13 23:54 ` [PATCH 1/3] KEYS: asym_tpm: fix buffer overreads in extract_key_parameters() Eric Biggers
2022-01-15 21:40   ` Jarkko Sakkinen
2022-01-19  0:59     ` Eric Biggers
2022-01-26 14:21       ` Jarkko Sakkinen
2022-01-26 14:22         ` Jarkko Sakkinen
2022-01-28 19:00           ` Eric Biggers
2022-02-08  9:30             ` Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).