| Message ID | 20220113235440.90439-1-ebiggers@kernel.org |
|---|---|
| Headers | show |
| Series | KEYS: fixes for asym_tpm keys | expand |
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 <denkenz@gmail.com> Regards, -Denis
On Thu, Jan 13, 2022 at 03:54:40PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > derive_pub_key() doesn't actually derive a key; it just formats one. > Rename it accordingly. > > Signed-off-by: Eric Biggers <ebiggers@google.com> Acked-by: Jarkko Sakkinen <jarkko@kernel.org> /Jarkko
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 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
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 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