Messages in this thread | | | Date | Wed, 28 Sep 2022 15:03:46 +0200 (CEST) | From | Nikolaus Voss <> | 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@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
| |