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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c76ab6ed-f276-428d-2471-6d38bf5b51ab@vosn.de>
Date:   Wed, 28 Sep 2022 15:03:46 +0200 (CEST)
From:   Nikolaus Voss <nv@...n.de>
To:     Jarkko Sakkinen <jarkko@...nel.org>
cc:     Mimi Zohar <zohar@...ux.ibm.com>,
        David Howells <dhowells@...hat.com>,
        Yael Tzur <yaelt@...gle.com>, James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        linux-integrity@...r.kernel.org, keyrings@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KEYS: encrypted: fix key instantiation with user-provided
 data

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@...nel.org>
>>>> Signed-off-by: Nikolaus Voss <nikolaus.voss@...g-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