KEYS: fix length validation in keyctl_pkey_params_get_2()

PATCH In review Eric Biggers 2 participants 6 messages Jan 13, 2022 – Jan 26, 2022

Subsystems (4)

Stable branch Crypto API Security (LSM) Keys/keyrings

lore

Share & tools

lore

Eric Biggers

author

2022-01-13 20:05 UTC #0 lore raw

PATCH 1/1 KEYS: fix length validation in keyctl_pkey_params_get_2()

From: Eric Biggers <[email protected]>

In many cases, keyctl_pkey_params_get_2() is validating the user buffer lengths against the wrong algorithm properties. Fix it to check against the correct properties.

Probably this wasn't noticed before because for all asymmetric keys of the "public_key" subtype, max_data_size == max_sig_size == max_enc_size == max_dec_size. However, this isn't necessarily true for the "asym_tpm" subtype (it should be, but it's not strictly validated). Of course, future key types could have different values as well.

Fixes 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]")

Cc <[email protected]> # v4.20+

Signed-off-by Eric Biggers <[email protected]>

---
 security/keys/keyctl_pkey.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

security/keys/keyctl_pkey.c +11 -3

index 5de0d599a274..97bc27bbf079 100644
@@ -135,15 +135,23 @@ static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_par
 
 	switch (op) {
 	case KEYCTL_PKEY_ENCRYPT:
+		if (uparams.in_len  > info.max_dec_size ||
+		    uparams.out_len > info.max_enc_size)
+			return -EINVAL;
+		break;
 	case KEYCTL_PKEY_DECRYPT:
 		if (uparams.in_len  > info.max_enc_size ||
 		    uparams.out_len > info.max_dec_size)
 			return -EINVAL;
 		break;
 	case KEYCTL_PKEY_SIGN:
+		if (uparams.in_len  > info.max_data_size ||
+		    uparams.out_len > info.max_sig_size)
+			return -EINVAL;
+		break;
 	case KEYCTL_PKEY_VERIFY:
-		if (uparams.in_len  > info.max_sig_size ||
-		    uparams.out_len > info.max_data_size)
+		if (uparams.in_len  > info.max_data_size ||
+		    uparams.in2_len > info.max_sig_size)
 			return -EINVAL;
 		break;
 	default:

@@ -151,7 +159,7 @@ static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_par
 	}
 
 	params->in_len  = uparams.in_len;
-	params->out_len = uparams.out_len;
+	params->out_len = uparams.out_len; /* Note: same as in2_len */
 	return 0;
 }

base-commit: feb7a43de5ef625ad74097d8fd3481d5dbc06a59

Signature · 1 line

2.34.1

Jarkko Sakkinen

maintainer

2022-01-15 18:45 UTC #1 lore raw

Quoted 11 lines · Eric Biggers

From: Eric Biggers <[email protected]>
In many cases, keyctl_pkey_params_get_2() is validating the user buffer lengths against the wrong algorithm properties. Fix it to check against the correct properties.
Probably this wasn't noticed before because for all asymmetric keys of the "public_key" subtype, max_data_size == max_sig_size == max_enc_size == max_dec_size. However, this isn't necessarily true for the "asym_tpm" subtype (it should be, but it's not strictly validated). Of course, future key types could have different values as well.

With a quick look, asym_tpm is TPM 1.x only, which only has 2048-bit RSA keys.

Quoted 47 lines

Fixes 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]")

Cc <[email protected]> # v4.20+

Signed-off-by Eric Biggers <[email protected]>

---
 security/keys/keyctl_pkey.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

security/keys/keyctl_pkey.c +11 -3

index 5de0d599a274..97bc27bbf079 100644
@@ -135,15 +135,23 @@ static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_par
 
 	switch (op) {
 	case KEYCTL_PKEY_ENCRYPT:
+		if (uparams.in_len  > info.max_dec_size ||
+		    uparams.out_len > info.max_enc_size)
+			return -EINVAL;
+		break;
 	case KEYCTL_PKEY_DECRYPT:
 		if (uparams.in_len  > info.max_enc_size ||
 		    uparams.out_len > info.max_dec_size)
 			return -EINVAL;
 		break;
 	case KEYCTL_PKEY_SIGN:
+		if (uparams.in_len  > info.max_data_size ||
+		    uparams.out_len > info.max_sig_size)
+			return -EINVAL;
+		break;
 	case KEYCTL_PKEY_VERIFY:
-		if (uparams.in_len  > info.max_sig_size ||
-		    uparams.out_len > info.max_data_size)
+		if (uparams.in_len  > info.max_data_size ||
+		    uparams.in2_len > info.max_sig_size)
 			return -EINVAL;
 		break;
 	default:

@@ -151,7 +159,7 @@ static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_par
 	}
 
 	params->in_len  = uparams.in_len;
-	params->out_len = uparams.out_len;
+	params->out_len = uparams.out_len; /* Note: same as in2_len */
 	return 0;
 }

Quoted 4 lines

base-commit: feb7a43de5ef625ad74097d8fd3481d5dbc06a59
Signature · 1 line
2.34.1

/Jarkko

Eric Biggers

author

2022-01-15 19:53 UTC #2 lore raw

Quoted 7 lines · Jarkko Sakkinen

Quoted 6 lines · Eric Biggers
From: Eric Biggers <[email protected]>
In many cases, keyctl_pkey_params_get_2() is validating the user buffer lengths against the wrong algorithm properties. Fix it to check against the correct properties.

Quoted 8 lines

Quoted 5 lines
Probably this wasn't noticed before because for all asymmetric keys of the "public_key" subtype, max_data_size == max_sig_size == max_enc_size == max_dec_size. However, this isn't necessarily true for the "asym_tpm" subtype (it should be, but it's not strictly validated). Of course, future key types could have different values as well.
With a quick look, asym_tpm is TPM 1.x only, which only has 2048-bit RSA keys.

The code allows other lengths, as well as the case where the "RSA key size" doesn't match the "public key size". Probably both are bugs and they should both be 256 bytes (2048 bits) only. Anyway, that would be a separate fix.

- Eric

Jarkko Sakkinen

maintainer Acked-by

2022-01-15 21:26 UTC #3 lore raw

Quoted 14 lines · Eric Biggers

Quoted 13 lines · Jarkko Sakkinen
Quoted 11 lines · Eric Biggers
From: Eric Biggers <[email protected]>
In many cases, keyctl_pkey_params_get_2() is validating the user buffer lengths against the wrong algorithm properties. Fix it to check against the correct properties.
Probably this wasn't noticed before because for all asymmetric keys of the "public_key" subtype, max_data_size == max_sig_size == max_enc_size == max_dec_size. However, this isn't necessarily true for the "asym_tpm" subtype (it should be, but it's not strictly validated). Of course, future key types could have different values as well.

Quoted 8 lines

Quoted 2 lines
With a quick look, asym_tpm is TPM 1.x only, which only has 2048-bit RSA keys.
The code allows other lengths, as well as the case where the "RSA key size" doesn't match the "public key size". Probably both are bugs and they should both be 256 bytes (2048 bits) only. Anyway, that would be a separate fix.
- Eric

I'm fine with the current commit message. E.g. I have no idea at this point whether there should be in future separate asym_tpm2 or all bundled to asym_tpm.

Acked-by Jarkko Sakkinen <[email protected]>

BR, Jarkko

Eric Biggers

author

2022-01-19 00:22 UTC #4 lore raw

Quoted 22 lines · Jarkko Sakkinen

Quoted 21 lines · Eric Biggers
Quoted 7 lines · Jarkko Sakkinen
Quoted 6 lines · Eric Biggers
From: Eric Biggers <[email protected]>
In many cases, keyctl_pkey_params_get_2() is validating the user buffer lengths against the wrong algorithm properties. Fix it to check against the correct properties.
Quoted 8 lines
Quoted 5 lines
Probably this wasn't noticed before because for all asymmetric keys of the "public_key" subtype, max_data_size == max_sig_size == max_enc_size == max_dec_size. However, this isn't necessarily true for the "asym_tpm" subtype (it should be, but it's not strictly validated). Of course, future key types could have different values as well.
With a quick look, asym_tpm is TPM 1.x only, which only has 2048-bit RSA keys.
The code allows other lengths, as well as the case where the "RSA key size" doesn't match the "public key size". Probably both are bugs and they should both be 256 bytes (2048 bits) only. Anyway, that would be a separate fix.

Quoted 8 lines

Quoted 1 line
- Eric
I'm fine with the current commit message. E.g. I have no idea at this point whether there should be in future separate asym_tpm2 or all bundled to asym_tpm.
Acked-by Jarkko Sakkinen <[email protected]>

Okay, great. Just to be clear, I'm expecting either you or David (maintainer:KEYS/KEYRINGS) to apply this patch. Acked-by is usually given by a maintainer when someone else applies a patch.

- Eric

Jarkko Sakkinen

maintainer

2022-01-26 14:06 UTC #5 lore raw

Quoted 29 lines · Eric Biggers

Quoted 28 lines · Jarkko Sakkinen
Quoted 14 lines · Eric Biggers
Quoted 13 lines · Jarkko Sakkinen
Quoted 11 lines · Eric Biggers
From: Eric Biggers <[email protected]>
In many cases, keyctl_pkey_params_get_2() is validating the user buffer lengths against the wrong algorithm properties. Fix it to check against the correct properties.
Probably this wasn't noticed before because for all asymmetric keys of the "public_key" subtype, max_data_size == max_sig_size == max_enc_size == max_dec_size. However, this isn't necessarily true for the "asym_tpm" subtype (it should be, but it's not strictly validated). Of course, future key types could have different values as well.
Quoted 8 lines
Quoted 2 lines
With a quick look, asym_tpm is TPM 1.x only, which only has 2048-bit RSA keys.
The code allows other lengths, as well as the case where the "RSA key size" doesn't match the "public key size". Probably both are bugs and they should both be 256 bytes (2048 bits) only. Anyway, that would be a separate fix.
- Eric
I'm fine with the current commit message. E.g. I have no idea at this point whether there should be in future separate asym_tpm2 or all bundled to asym_tpm.

Quoted 6 lines

Quoted 2 lines
Acked-by Jarkko Sakkinen <[email protected]>
Okay, great. Just to be clear, I'm expecting either you or David (maintainer:KEYS/KEYRINGS) to apply this patch. Acked-by is usually given by a maintainer when someone else applies a patch.

I changed it to reviewed-by and applied.

BR, Jarkko