On Wed, 21 Sep 2022, Jarkko Sakkinen wrote:
On Tue, Sep 20, 2022 at 09:58:56AM +0200, Nikolaus Voss wrote:
On Tue, 20 Sep 2022, Jarkko Sakkinen wrote:
On Fri, Sep 16, 2022 at 07:45:29AM +0200, Nikolaus Voss wrote:
Commit cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided
decrypted data") added key instantiation with user provided decrypted data.
The user data is hex-ascii-encoded but was just memcpy'ed to the binary buffer.
Fix this to use hex2bin instead.
Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided decrypted data")
Cc: stable <stable@xxxxxxxxxx>
Signed-off-by: Nikolaus Voss <nikolaus.voss@xxxxxxxxxxxxxxx>
---
security/keys/encrypted-keys/encrypted.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index e05cfc2e49ae..1e313982af02 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -627,7 +627,7 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
pr_err("encrypted key: instantiation of keys using provided decrypted data is disabled since CONFIG_USER_DECRYPTED_DATA is set to false\n");
return ERR_PTR(-EINVAL);
}
- if (strlen(decrypted_data) != decrypted_datalen) {
+ if (strlen(decrypted_data) != decrypted_datalen * 2) {
This looks wrong. What does cap decrypted_data, and why strnlen()
is not used?
This is a plausibility check to ensure the user-specified key length
(decrypted_datalen) matches the length of the user specified key. strnlen()
would not add any extra security here, the data has already been copied.
I'd prefer unconditional use of strnlen() because it always
gives you at least some guarantees over deducing why strlen()
is fine in a particular code block.
I agree. Unfortunately, there is no blob size available in
encrypted_key_alloc(), so this would mean changing function signatures
and code to get this downstream.
This would be well worth a patch on its own.
pr_err("encrypted key: decrypted data provided does not match decrypted data length provided\n");
Using pr_err() is probably wrong here and has different prefix
than elsewhere in the file (also most of other uses of pr_err()
are wrong apparently). Nothing bad is really happening.
It actually _is_ an error preventing key instatiation. User space keyctl
cannot be verbose about the reason why instantiation failed so it makes
sense to be verbose in kernel space. To me, this seems consistent with other
occurrences of pr_err() in this file, maybe I misunderstood you?
Then it should be pr_info(), or even pr_debug(), given that it is not a
kernel issue.
Btw, this patch changes neither string length checking nor log levels.
I understand this. It has been my own mistake to ack that pr_err().
However, does not fully apply to strlen() part. Since you are
changing that line anyway, it'd be better to replace strlen()
with strnlen(). This e.g. protects the code block changes in
the context where it is called.
I'd love to do it if it was simple.
Niko
