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

[Jarkko Sakkinen]

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.


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

BR, Jarkko