Re: [PATCH] KEYS: encrypted: fix key instantiation with user-provided data

[Nikolaus Voss <nv@vosn.de>]

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@kernel.org>
>>>> Signed-off-by: Nikolaus Voss <nikolaus.voss@haag-streit.com>
>>>> ---
>>>>  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