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

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

[Nikolaus Voss]

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) {
 			pr_err("encrypted key: decrypted data provided does not match decrypted data length provided\n");
 			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

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

[Jarkko Sakkinen]

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?

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

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

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

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

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

[Mimi Zohar]

On Fri, 2022-09-16 at 07:45 +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.

Thanks, Nikolaus.  We iterated a number of times over what would be the
safest userspace input.  One of the last changes was that the key data
should be hex-ascii-encoded.  Unfortunately, the LTP
testcases/kernel/syscalls/keyctl09.c example isn't hex-ascii-encoded
and the example in Documentation/security/keys/trusted-encrypted.rst
just cat's a file.  Both expect the length to be the length of the
userspace provided data.   With this patch, when hex2bin() fails, there
is no explanation.

-- 
thanks,

Mimi

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

[Nikolaus Voss]

On Tue, 20 Sep 2022, Mimi Zohar wrote:
> On Fri, 2022-09-16 at 07:45 +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.
>
> Thanks, Nikolaus.  We iterated a number of times over what would be the
> safest userspace input.  One of the last changes was that the key data
> should be hex-ascii-encoded.  Unfortunately, the LTP
> testcases/kernel/syscalls/keyctl09.c example isn't hex-ascii-encoded
> and the example in Documentation/security/keys/trusted-encrypted.rst
> just cat's a file.  Both expect the length to be the length of the
> userspace provided data.   With this patch, when hex2bin() fails, there
> is no explanation.

That's true. But it's true for all occurrences of hex2bin() in this file.
I could pr_err() an explanation, improve the trusted-encrypted.rst example 
and respin the patch. Should I, or do you have another suggestion?

I wasn't aware of keyctl09.c, but quickly looking into it, the user data 
_is_ hex-ascii-encoded, only the length is "wrong": Imho, the specified 
length should be the binary length as this is consistent with key-length 
specs in other cases (e.g. when loading the key from a blob).
keyctl09.c could be easy to fix, if only the length is modified. Should 
I propose a patch? What is the correct/appropriate workflow there?

Thanks,
Niko

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

[Mimi Zohar]

On Tue, 2022-09-20 at 18:23 +0200, Nikolaus Voss wrote:
> On Tue, 20 Sep 2022, Mimi Zohar wrote:
> > On Fri, 2022-09-16 at 07:45 +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.
> >
> > Thanks, Nikolaus.  We iterated a number of times over what would be the
> > safest userspace input.  One of the last changes was that the key data
> > should be hex-ascii-encoded.  Unfortunately, the LTP
> > testcases/kernel/syscalls/keyctl09.c example isn't hex-ascii-encoded
> > and the example in Documentation/security/keys/trusted-encrypted.rst
> > just cat's a file.  Both expect the length to be the length of the
> > userspace provided data.   With this patch, when hex2bin() fails, there
> > is no explanation.
> 
> That's true. But it's true for all occurrences of hex2bin() in this file.
> I could pr_err() an explanation, improve the trusted-encrypted.rst example 
> and respin the patch. Should I, or do you have another suggestion?

> I wasn't aware of keyctl09.c, but quickly looking into it, the user data 
> _is_ hex-ascii-encoded, only the length is "wrong": Imho, the specified 
> length should be the binary length as this is consistent with key-length 
> specs in other cases (e.g. when loading the key from a blob).
> keyctl09.c could be easy to fix, if only the length is modified. Should 
> I propose a patch? What is the correct/appropriate workflow there?

I'm concerned that this change breaks existing encrypted keys created
with user-provided data.  Otherwise I'm fine with your suggestion.

The LTP example decrypted data length is 32, but the minimum decrypted
data size is  20.  So it's a bit more than just changing the LTP
decrypted data size.   The modified LTP test should work on kernels
with and without this patch.

-- 
thanks,

Mimi

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

[Nikolaus Voss]

On Tue, 20 Sep 2022, Mimi Zohar wrote:
> On Tue, 2022-09-20 at 18:23 +0200, Nikolaus Voss wrote:
>> On Tue, 20 Sep 2022, Mimi Zohar wrote:
>>> On Fri, 2022-09-16 at 07:45 +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.
>>>
>>> Thanks, Nikolaus.  We iterated a number of times over what would be the
>>> safest userspace input.  One of the last changes was that the key data
>>> should be hex-ascii-encoded.  Unfortunately, the LTP
>>> testcases/kernel/syscalls/keyctl09.c example isn't hex-ascii-encoded
>>> and the example in Documentation/security/keys/trusted-encrypted.rst
>>> just cat's a file.  Both expect the length to be the length of the
>>> userspace provided data.   With this patch, when hex2bin() fails, there
>>> is no explanation.
>>
>> That's true. But it's true for all occurrences of hex2bin() in this file.
>> I could pr_err() an explanation, improve the trusted-encrypted.rst example
>> and respin the patch. Should I, or do you have another suggestion?
>
>> I wasn't aware of keyctl09.c, but quickly looking into it, the user data
>> _is_ hex-ascii-encoded, only the length is "wrong": Imho, the specified
>> length should be the binary length as this is consistent with key-length
>> specs in other cases (e.g. when loading the key from a blob).
>> keyctl09.c could be easy to fix, if only the length is modified. Should
>> I propose a patch? What is the correct/appropriate workflow there?
>
> I'm concerned that this change breaks existing encrypted keys created
> with user-provided data.  Otherwise I'm fine with your suggestion.

Ok, but this change does not touch the hex-ascii format of encrypted key 
blobs?

>
> The LTP example decrypted data length is 32, but the minimum decrypted
> data size is  20.  So it's a bit more than just changing the LTP
> decrypted data size.   The modified LTP test should work on kernels
> with and without this patch.

So this would mean OR-ing old and new variant for the test?

The current implementation of the test will fail anyway as the key size is 
below the minimum of 20 and thus should have failed before.

Niko

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

[Mimi Zohar]

On Wed, 2022-09-21 at 09:24 +0200, Nikolaus Voss wrote:
> On Tue, 20 Sep 2022, Mimi Zohar wrote:
> > On Tue, 2022-09-20 at 18:23 +0200, Nikolaus Voss wrote:
> >> On Tue, 20 Sep 2022, Mimi Zohar wrote:
> >>> On Fri, 2022-09-16 at 07:45 +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.
> >>>
> >>> Thanks, Nikolaus.  We iterated a number of times over what would be the
> >>> safest userspace input.  One of the last changes was that the key data
> >>> should be hex-ascii-encoded.  Unfortunately, the LTP
> >>> testcases/kernel/syscalls/keyctl09.c example isn't hex-ascii-encoded
> >>> and the example in Documentation/security/keys/trusted-encrypted.rst
> >>> just cat's a file.  Both expect the length to be the length of the
> >>> userspace provided data.   With this patch, when hex2bin() fails, there
> >>> is no explanation.
> >>
> >> That's true. But it's true for all occurrences of hex2bin() in this file.
> >> I could pr_err() an explanation, improve the trusted-encrypted.rst example
> >> and respin the patch. Should I, or do you have another suggestion?
> >
> >> I wasn't aware of keyctl09.c, but quickly looking into it, the user data
> >> _is_ hex-ascii-encoded, only the length is "wrong": Imho, the specified
> >> length should be the binary length as this is consistent with key-length
> >> specs in other cases (e.g. when loading the key from a blob).
> >> keyctl09.c could be easy to fix, if only the length is modified. Should
> >> I propose a patch? What is the correct/appropriate workflow there?
> >
> > I'm concerned that this change breaks existing encrypted keys created
> > with user-provided data.  Otherwise I'm fine with your suggestion.
> 
> Ok, but this change does not touch the hex-ascii format of encrypted key 
> blobs?

True, but any persistent data based on this key would be affected.

> 
> >
> > The LTP example decrypted data length is 32, but the minimum decrypted
> > data size is  20.  So it's a bit more than just changing the LTP
> > decrypted data size.   The modified LTP test should work on kernels
> > with and without this patch.
> 
> So this would mean OR-ing old and new variant for the test?
> 
> The current implementation of the test will fail anyway as the key size is 
> below the minimum of 20 and thus should have failed before.

The existing keyctl09 test is a plain text string.  Converting it to
hex-ascii (e.g. hexdump, xdd) solves the length issue.  For those
already using encrypted keys with user provided data, this might also
resolve the persistent data usage case mentioned above.

Perhaps keep the existing test.  On success issue a warning.  On
failure, retry with the converted plain text string.

-- 
thanks,

Mimi

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


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

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

[Nikolaus Voss]

On Wed, 21 Sep 2022, Mimi Zohar wrote:
> On Wed, 2022-09-21 at 09:24 +0200, Nikolaus Voss wrote:
>> On Tue, 20 Sep 2022, Mimi Zohar wrote:
>>> On Tue, 2022-09-20 at 18:23 +0200, Nikolaus Voss wrote:
>>>> On Tue, 20 Sep 2022, Mimi Zohar wrote:
>>>>> On Fri, 2022-09-16 at 07:45 +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.
>>>>>
>>>>> Thanks, Nikolaus.  We iterated a number of times over what would be the
>>>>> safest userspace input.  One of the last changes was that the key data
>>>>> should be hex-ascii-encoded.  Unfortunately, the LTP
>>>>> testcases/kernel/syscalls/keyctl09.c example isn't hex-ascii-encoded
>>>>> and the example in Documentation/security/keys/trusted-encrypted.rst
>>>>> just cat's a file.  Both expect the length to be the length of the
>>>>> userspace provided data.   With this patch, when hex2bin() fails, there
>>>>> is no explanation.
>>>>
>>>> That's true. But it's true for all occurrences of hex2bin() in this file.
>>>> I could pr_err() an explanation, improve the trusted-encrypted.rst example
>>>> and respin the patch. Should I, or do you have another suggestion?
>>>
>>>> I wasn't aware of keyctl09.c, but quickly looking into it, the user data
>>>> _is_ hex-ascii-encoded, only the length is "wrong": Imho, the specified
>>>> length should be the binary length as this is consistent with key-length
>>>> specs in other cases (e.g. when loading the key from a blob).
>>>> keyctl09.c could be easy to fix, if only the length is modified. Should
>>>> I propose a patch? What is the correct/appropriate workflow there?
>>>
>>> I'm concerned that this change breaks existing encrypted keys created
>>> with user-provided data.  Otherwise I'm fine with your suggestion.
>>
>> Ok, but this change does not touch the hex-ascii format of encrypted key
>> blobs?
>
> True, but any persistent data based on this key would be affected.

Persistent data is stored encypted with e.g. the master key in hex-ascii 
already and should not be affected. Only persistent data stored 
unencrypted is affected, but the encrypted-keys stuff is just about 
avoiding that. Or do I still misunderstand something?

>
>>
>>>
>>> The LTP example decrypted data length is 32, but the minimum decrypted
>>> data size is  20.  So it's a bit more than just changing the LTP
>>> decrypted data size.   The modified LTP test should work on kernels
>>> with and without this patch.
>>
>> So this would mean OR-ing old and new variant for the test?
>>
>> The current implementation of the test will fail anyway as the key size is
>> below the minimum of 20 and thus should have failed before.
>
> The existing keyctl09 test is a plain text string.  Converting it to
> hex-ascii (e.g. hexdump, xdd) solves the length issue.  For those
> already using encrypted keys with user provided data, this might also
> resolve the persistent data usage case mentioned above.

The unencrypted data from testcases/kernel/syscalls/keyctl/keyctl09.c 
looks like hex-ascii to me:
#define ENCRYPTED_KEY_VALID_PAYLOAD    "new enc32 user:masterkey 32 abcdefABCDEF1234567890aaaaaaaaaa"

And beeing it hex-ascii is checked in encrypted.c driver:
 		for (i = 0; i < strlen(decrypted_data); i++) {
 			if (!isxdigit(decrypted_data[i])) {
 				pr_err("encrypted key: decrypted data provided must contain only hexadecimal characters\n");
 				return ERR_PTR(-EINVAL);
 			}
 		}

It was never possible to provide unencrypted data other than hex-ascii, it 
just wasn't decoded to binary, so the resulting key was simply wrong and 
rendered CONFIG_USER_DECRYPTED_DATA useless. Because the resulting binary 
key was limited to the hex-ascii range of values.

> Perhaps keep the existing test.  On success issue a warning. 
> On failure, retry with the converted plain text string.

With that being said, I would expect the existing test fail and the 
corrected test to pass.

Niko

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

[Nikolaus Voss]

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

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

[Mimi Zohar]

On Wed, 2022-09-28 at 14:08 +0200, Nikolaus Voss wrote:
> On Wed, 21 Sep 2022, Mimi Zohar wrote:
> > On Wed, 2022-09-21 at 09:24 +0200, Nikolaus Voss wrote:
> >> On Tue, 20 Sep 2022, Mimi Zohar wrote:
> >>> On Tue, 2022-09-20 at 18:23 +0200, Nikolaus Voss wrote:
> >>>> On Tue, 20 Sep 2022, Mimi Zohar wrote:
> >>>>> On Fri, 2022-09-16 at 07:45 +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.
> >>>>>
> >>>>> Thanks, Nikolaus.  We iterated a number of times over what would be the
> >>>>> safest userspace input.  One of the last changes was that the key data
> >>>>> should be hex-ascii-encoded.  Unfortunately, the LTP
> >>>>> testcases/kernel/syscalls/keyctl09.c example isn't hex-ascii-encoded
> >>>>> and the example in Documentation/security/keys/trusted-encrypted.rst
> >>>>> just cat's a file.  Both expect the length to be the length of the
> >>>>> userspace provided data.   With this patch, when hex2bin() fails, there
> >>>>> is no explanation.
> >>>>
> >>>> That's true. But it's true for all occurrences of hex2bin() in this file.
> >>>> I could pr_err() an explanation, improve the trusted-encrypted.rst example
> >>>> and respin the patch. Should I, or do you have another suggestion?
> >>>
> >>>> I wasn't aware of keyctl09.c, but quickly looking into it, the user data
> >>>> _is_ hex-ascii-encoded, only the length is "wrong": Imho, the specified
> >>>> length should be the binary length as this is consistent with key-length
> >>>> specs in other cases (e.g. when loading the key from a blob).
> >>>> keyctl09.c could be easy to fix, if only the length is modified. Should
> >>>> I propose a patch? What is the correct/appropriate workflow there?
> >>>
> >>> I'm concerned that this change breaks existing encrypted keys created
> >>> with user-provided data.  Otherwise I'm fine with your suggestion.
> >>
> >> Ok, but this change does not touch the hex-ascii format of encrypted key
> >> blobs?
> >
> > True, but any persistent data based on this key would be affected.
> 
> Persistent data is stored encypted with e.g. the master key in hex-ascii 
> already and should not be affected. Only persistent data stored 
> unencrypted is affected, but the encrypted-keys stuff is just about 
> avoiding that. Or do I still misunderstand something?

Perhaps an existing encrypted key usage example would help clarify what
is meant by persistent data.  The two original encrypted key usages are
the EVM HMAC key and ecryptfs.  The EVM key is an encrypted key used to
calculate the EVM HMAC, which is stored in security.evm.  In that
scenario, the persistent data would be the data stored in security.evm.

Would this patch break existing kernel/application persistent data
based on encrypted keys created with user-provided data?

-- 
thanks,

Mimi

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

[Nikolaus Voss]

On Wed, 28 Sep 2022, Mimi Zohar wrote:
> On Wed, 2022-09-28 at 14:08 +0200, Nikolaus Voss wrote:
>> On Wed, 21 Sep 2022, Mimi Zohar wrote:
>>> On Wed, 2022-09-21 at 09:24 +0200, Nikolaus Voss wrote:
>>>> On Tue, 20 Sep 2022, Mimi Zohar wrote:
>>>>> On Tue, 2022-09-20 at 18:23 +0200, Nikolaus Voss wrote:
>>>>>> On Tue, 20 Sep 2022, Mimi Zohar wrote:
>>>>>>> On Fri, 2022-09-16 at 07:45 +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.
>>>>>>>
>>>>>>> Thanks, Nikolaus.  We iterated a number of times over what would be the
>>>>>>> safest userspace input.  One of the last changes was that the key data
>>>>>>> should be hex-ascii-encoded.  Unfortunately, the LTP
>>>>>>> testcases/kernel/syscalls/keyctl09.c example isn't hex-ascii-encoded
>>>>>>> and the example in Documentation/security/keys/trusted-encrypted.rst
>>>>>>> just cat's a file.  Both expect the length to be the length of the
>>>>>>> userspace provided data.   With this patch, when hex2bin() fails, there
>>>>>>> is no explanation.
>>>>>>
>>>>>> That's true. But it's true for all occurrences of hex2bin() in this file.
>>>>>> I could pr_err() an explanation, improve the trusted-encrypted.rst example
>>>>>> and respin the patch. Should I, or do you have another suggestion?
>>>>>
>>>>>> I wasn't aware of keyctl09.c, but quickly looking into it, the user data
>>>>>> _is_ hex-ascii-encoded, only the length is "wrong": Imho, the specified
>>>>>> length should be the binary length as this is consistent with key-length
>>>>>> specs in other cases (e.g. when loading the key from a blob).
>>>>>> keyctl09.c could be easy to fix, if only the length is modified. Should
>>>>>> I propose a patch? What is the correct/appropriate workflow there?
>>>>>
>>>>> I'm concerned that this change breaks existing encrypted keys created
>>>>> with user-provided data.  Otherwise I'm fine with your suggestion.
>>>>
>>>> Ok, but this change does not touch the hex-ascii format of encrypted key
>>>> blobs?
>>>
>>> True, but any persistent data based on this key would be affected.
>>
>> Persistent data is stored encypted with e.g. the master key in hex-ascii
>> already and should not be affected. Only persistent data stored
>> unencrypted is affected, but the encrypted-keys stuff is just about
>> avoiding that. Or do I still misunderstand something?
>
> Perhaps an existing encrypted key usage example would help clarify what
> is meant by persistent data.  The two original encrypted key usages are
> the EVM HMAC key and ecryptfs.  The EVM key is an encrypted key used to
> calculate the EVM HMAC, which is stored in security.evm.  In that
> scenario, the persistent data would be the data stored in security.evm.
>
> Would this patch break existing kernel/application persistent data
> based on encrypted keys created with user-provided data?

As far as I can tell, it does not.

Niko