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

[Nikolaus Voss]

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.

> >  			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?

Btw, this patch changes neither string length checking nor log levels.

> And who does make any sense of that error message anyway?
>
> For one, I don't understand it.
>
> >  			return ERR_PTR(-EINVAL);
> >  		}
> > @@ -791,8 +791,8 @@ static int encrypted_init(struct encrypted_key_payload *epayload,
> >  		ret = encrypted_key_decrypt(epayload, format, hex_encoded_iv);
> >  	} else if (decrypted_data) {
> >  		get_random_bytes(epayload->iv, ivsize);
> > -		memcpy(epayload->decrypted_data, decrypted_data,
> > -				   epayload->decrypted_datalen);
> > +		ret = hex2bin(epayload->decrypted_data, decrypted_data,
> > +			      epayload->decrypted_datalen);
> >  	} else {
> >  		get_random_bytes(epayload->iv, ivsize);
> >  		get_random_bytes(epayload->decrypted_data, epayload->decrypted_datalen);
> > --
> > 2.34.1
>
>
> BR, Jarkko

Jarkko, thanks for the review!

Niko