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.
Old keys created from user provided decrypted data saved with "keyctl pipe"
are still valid, however if the key is recreated from decrypted data the
old key must be converted to the correct format. This can be done with a
small shell script, e.g.:
BROKENKEY=abcdefABCDEF1234567890aaaaaaaaaa
NEWKEY=$(echo -ne $BROKENKEY | xxd -p -c32)
keyctl add user masterkey "$(cat masterkey.bin)" @u
keyctl add encrypted testkey "new user:masterkey 32 $NEWKEY" @u
It is encouraged to switch to a new key because the effective key size
of the old keys is only half of the specified size.
The corresponding test for the Linux Test Project ltp has also been
fixed (see link below).
Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided decrypted data")
Cc: stable <[email protected]>
Link: https://lore.kernel.org/ltp/[email protected]/
Signed-off-by: Nikolaus Voss <[email protected]>
---
Changes
=======
v4: - fixed link
v3: - use generated random key in example, reformat commit message
v2: - clarify commit message, add example to recover old/broken keys
- improve example in Documentation/security/keys/trusted-encrypted.rst
- add link to ltp patch
Documentation/security/keys/trusted-encrypted.rst | 3 ++-
security/keys/encrypted-keys/encrypted.c | 6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/Documentation/security/keys/trusted-encrypted.rst b/Documentation/security/keys/trusted-encrypted.rst
index 0bfb4c339748..9bc9db8ec651 100644
--- a/Documentation/security/keys/trusted-encrypted.rst
+++ b/Documentation/security/keys/trusted-encrypted.rst
@@ -350,7 +350,8 @@ Load an encrypted key "evm" from saved blob::
Instantiate an encrypted key "evm" using user-provided decrypted data::
- $ keyctl add encrypted evm "new default user:kmk 32 `cat evm_decrypted_data.blob`" @u
+ $ evmkey=$(dd if=/dev/urandom bs=1 count=32 | xxd -c32 -p)
+ $ keyctl add encrypted evm "new default user:kmk 32 $evmkey" @u
794890253
$ keyctl print 794890253
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
On Thu, 2022-10-13 at 08:39 +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.
>
> Old keys created from user provided decrypted data saved with "keyctl pipe"
> are still valid, however if the key is recreated from decrypted data the
> old key must be converted to the correct format. This can be done with a
> small shell script, e.g.:
>
> BROKENKEY=abcdefABCDEF1234567890aaaaaaaaaa
> NEWKEY=$(echo -ne $BROKENKEY | xxd -p -c32)
> keyctl add user masterkey "$(cat masterkey.bin)" @u
> keyctl add encrypted testkey "new user:masterkey 32 $NEWKEY" @u
>
> It is encouraged to switch to a new key because the effective key size
> of the old keys is only half of the specified size.
Both the old and new decrypted data size is 32 bytes. Is the above
statement necessary, especially since the Documentation example does
the equivalent?
> The corresponding test for the Linux Test Project ltp has also been
> fixed (see link below).
The LTP patch still needs to be revised, but the "Link" is a reference
to the discussion. Is the above statement necessary?
>
> Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided decrypted data")
> Cc: stable <[email protected]>
> Link: https://lore.kernel.org/ltp/[email protected]/
> Signed-off-by: Nikolaus Voss <[email protected]>
Otherwise,
Reviewed-by: Mimi Zohar <[email protected]>
On Thu, 13 Oct 2022, Mimi Zohar wrote:
> On Thu, 2022-10-13 at 08:39 +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.
>>
>> Old keys created from user provided decrypted data saved with "keyctl pipe"
>> are still valid, however if the key is recreated from decrypted data the
>> old key must be converted to the correct format. This can be done with a
>> small shell script, e.g.:
>>
>> BROKENKEY=abcdefABCDEF1234567890aaaaaaaaaa
>> NEWKEY=$(echo -ne $BROKENKEY | xxd -p -c32)
>> keyctl add user masterkey "$(cat masterkey.bin)" @u
>> keyctl add encrypted testkey "new user:masterkey 32 $NEWKEY" @u
>>
>> It is encouraged to switch to a new key because the effective key size
>> of the old keys is only half of the specified size.
>
> Both the old and new decrypted data size is 32 bytes. Is the above
> statement necessary, especially since the Documentation example does
> the equivalent?
The old key has the same byte size but all bytes must be within the
hex-asc?i range of characters, otherwise it is refused by the kernel.
So if you wanted a 32 bytes key you get 16 effective bytes for the key.
In the above example the string size of the $BROKENKEY is 32, while
the string size of the $NEWKEY is 64.
If you do
$ echo $NEWKEY
6162636465664142434445463132333435363738393061616161616161616161
for the example, the range problem is obvious, so $NEWKEY is still broken.
That's why it should only be used to recover data which should be
reencypted with a new key. If you count exactly, the effective key size is
_slightly_ longer than half of the specified size, but it is still a
severe security problem.
>
>> The corresponding test for the Linux Test Project ltp has also been
>> fixed (see link below).
>
> The LTP patch still needs to be revised, but the "Link" is a reference
> to the discussion. Is the above statement necessary?
As long as the patch is not accepted the discussion is helpful. But feel
free to delete it upon integration ;-).
>
>>
>> Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided decrypted data")
>> Cc: stable <[email protected]>
>> Link: https://lore.kernel.org/ltp/[email protected]/
>> Signed-off-by: Nikolaus Voss <[email protected]>
>
> Otherwise,
>
> Reviewed-by: Mimi Zohar <[email protected]>
Thanks Mimi!
On Fri, 2022-10-14 at 08:40 +0200, Nikolaus Voss wrote:
> On Thu, 13 Oct 2022, Mimi Zohar wrote:
> > On Thu, 2022-10-13 at 08:39 +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.
> >>
> >> Old keys created from user provided decrypted data saved with "keyctl pipe"
> >> are still valid, however if the key is recreated from decrypted data the
> >> old key must be converted to the correct format. This can be done with a
> >> small shell script, e.g.:
> >>
> >> BROKENKEY=abcdefABCDEF1234567890aaaaaaaaaa
> >> NEWKEY=$(echo -ne $BROKENKEY | xxd -p -c32)
> >> keyctl add user masterkey "$(cat masterkey.bin)" @u
> >> keyctl add encrypted testkey "new user:masterkey 32 $NEWKEY" @u
> >>
> >> It is encouraged to switch to a new key because the effective key size
> >> of the old keys is only half of the specified size.
> >
> > Both the old and new decrypted data size is 32 bytes. Is the above
> > statement necessary, especially since the Documentation example does
> > the equivalent?
>
> The old key has the same byte size but all bytes must be within the
> hex-asc?i range of characters, otherwise it is refused by the kernel.
> So if you wanted a 32 bytes key you get 16 effective bytes for the key.
> In the above example the string size of the $BROKENKEY is 32, while
> the string size of the $NEWKEY is 64.
>
> If you do
>
> $ echo $NEWKEY
> 6162636465664142434445463132333435363738393061616161616161616161
>
> for the example, the range problem is obvious, so $NEWKEY is still broken.
> That's why it should only be used to recover data which should be
> reencypted with a new key. If you count exactly, the effective key size is
> _slightly_ longer than half of the specified size, but it is still a
> severe security problem.
So the issue with NEWKEY isn't the "effective key size of the old keys
is only half of the specified size", but that the old key, itself, is
limited to the hex-ascii range of characters.
Mimi
On Fri, 14 Oct 2022, Mimi Zohar wrote:
> On Fri, 2022-10-14 at 08:40 +0200, Nikolaus Voss wrote:
>> On Thu, 13 Oct 2022, Mimi Zohar wrote:
>>> On Thu, 2022-10-13 at 08:39 +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.
>>>>
>>>> Old keys created from user provided decrypted data saved with "keyctl pipe"
>>>> are still valid, however if the key is recreated from decrypted data the
>>>> old key must be converted to the correct format. This can be done with a
>>>> small shell script, e.g.:
>>>>
>>>> BROKENKEY=abcdefABCDEF1234567890aaaaaaaaaa
>>>> NEWKEY=$(echo -ne $BROKENKEY | xxd -p -c32)
>>>> keyctl add user masterkey "$(cat masterkey.bin)" @u
>>>> keyctl add encrypted testkey "new user:masterkey 32 $NEWKEY" @u
>>>>
>>>> It is encouraged to switch to a new key because the effective key size
>>>> of the old keys is only half of the specified size.
>>>
>>> Both the old and new decrypted data size is 32 bytes. Is the above
>>> statement necessary, especially since the Documentation example does
>>> the equivalent?
>>
>> The old key has the same byte size but all bytes must be within the
>> hex-asc?i range of characters, otherwise it is refused by the kernel.
>> So if you wanted a 32 bytes key you get 16 effective bytes for the key.
>> In the above example the string size of the $BROKENKEY is 32, while
>> the string size of the $NEWKEY is 64.
>>
>> If you do
>>
>> $ echo $NEWKEY
>> 6162636465664142434445463132333435363738393061616161616161616161
>>
>> for the example, the range problem is obvious, so $NEWKEY is still broken.
>> That's why it should only be used to recover data which should be
>> reencypted with a new key. If you count exactly, the effective key size is
>> _slightly_ longer than half of the specified size, but it is still a
>> severe security problem.
>
> So the issue with NEWKEY isn't the "effective key size of the old keys
> is only half of the specified size", but that the old key, itself, is
> limited to the hex-ascii range of characters.
The latter resulting in the former. If for BROKENKEY 32 bytes were
specified, a brute force attacker knowing the key properties would only
need to try at most 2^(16*8) keys, as if the key was only 16 bytes long.
This is what I mean with "effective size" in contrast to the key's byte
size which is 32 in my example.
The security issue is a result of the combination of limiting the input
range to hex-ascii and using memcpy() instead of hex2bin(). It could have
been fixed either by allowing binary input or using hex2bin() (and
doubling the ascii input key length). I chose the latter.
Niko
On Fri, 2022-10-14 at 13:39 +0200, Nikolaus Voss wrote:
> On Fri, 14 Oct 2022, Mimi Zohar wrote:
> > On Fri, 2022-10-14 at 08:40 +0200, Nikolaus Voss wrote:
> >> On Thu, 13 Oct 2022, Mimi Zohar wrote:
> >>> On Thu, 2022-10-13 at 08:39 +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.
> >>>>
> >>>> Old keys created from user provided decrypted data saved with "keyctl pipe"
> >>>> are still valid, however if the key is recreated from decrypted data the
> >>>> old key must be converted to the correct format. This can be done with a
> >>>> small shell script, e.g.:
> >>>>
> >>>> BROKENKEY=abcdefABCDEF1234567890aaaaaaaaaa
> >>>> NEWKEY=$(echo -ne $BROKENKEY | xxd -p -c32)
> >>>> keyctl add user masterkey "$(cat masterkey.bin)" @u
> >>>> keyctl add encrypted testkey "new user:masterkey 32 $NEWKEY" @u
> >>>>
> >>>> It is encouraged to switch to a new key because the effective key size
> >>>> of the old keys is only half of the specified size.
> >>>
> >>> Both the old and new decrypted data size is 32 bytes. Is the above
> >>> statement necessary, especially since the Documentation example does
> >>> the equivalent?
> >>
> >> The old key has the same byte size but all bytes must be within the
> >> hex-asc?i range of characters, otherwise it is refused by the kernel.
> >> So if you wanted a 32 bytes key you get 16 effective bytes for the key.
> >> In the above example the string size of the $BROKENKEY is 32, while
> >> the string size of the $NEWKEY is 64.
> >>
> >> If you do
> >>
> >> $ echo $NEWKEY
> >> 6162636465664142434445463132333435363738393061616161616161616161
> >>
> >> for the example, the range problem is obvious, so $NEWKEY is still broken.
> >> That's why it should only be used to recover data which should be
> >> reencypted with a new key. If you count exactly, the effective key size is
> >> _slightly_ longer than half of the specified size, but it is still a
> >> severe security problem.
> >
> > So the issue with NEWKEY isn't the "effective key size of the old keys
> > is only half of the specified size", but that the old key, itself, is
> > limited to the hex-ascii range of characters.
>
> The latter resulting in the former. If for BROKENKEY 32 bytes were
> specified, a brute force attacker knowing the key properties would only
> need to try at most 2^(16*8) keys, as if the key was only 16 bytes long.
> This is what I mean with "effective size" in contrast to the key's byte
> size which is 32 in my example.
>
> The security issue is a result of the combination of limiting the input
> range to hex-ascii and using memcpy() instead of hex2bin(). It could have
> been fixed either by allowing binary input or using hex2bin() (and
> doubling the ascii input key length). I chose the latter.
Agreed the latter is better solution. Please update/replace the
sentence "It is encouraged to switch to a new key because ..." based on
this more complete explanation.
thanks,
Mimi
On Thu, Oct 13, 2022 at 08:39:58AM +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.
>
> Old keys created from user provided decrypted data saved with "keyctl pipe"
> are still valid, however if the key is recreated from decrypted data the
> old key must be converted to the correct format. This can be done with a
> small shell script, e.g.:
>
> BROKENKEY=abcdefABCDEF1234567890aaaaaaaaaa
> NEWKEY=$(echo -ne $BROKENKEY | xxd -p -c32)
> keyctl add user masterkey "$(cat masterkey.bin)" @u
> keyctl add encrypted testkey "new user:masterkey 32 $NEWKEY" @u
>
> It is encouraged to switch to a new key because the effective key size
> of the old keys is only half of the specified size.
>
> The corresponding test for the Linux Test Project ltp has also been
> fixed (see link below).
>
> Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided decrypted data")
> Cc: stable <[email protected]>
> Link: https://lore.kernel.org/ltp/[email protected]/
> Signed-off-by: Nikolaus Voss <[email protected]>
> ---
> Changes
> =======
> v4: - fixed link
> v3: - use generated random key in example, reformat commit message
> v2: - clarify commit message, add example to recover old/broken keys
> - improve example in Documentation/security/keys/trusted-encrypted.rst
> - add link to ltp patch
>
> Documentation/security/keys/trusted-encrypted.rst | 3 ++-
> security/keys/encrypted-keys/encrypted.c | 6 +++---
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/security/keys/trusted-encrypted.rst b/Documentation/security/keys/trusted-encrypted.rst
> index 0bfb4c339748..9bc9db8ec651 100644
> --- a/Documentation/security/keys/trusted-encrypted.rst
> +++ b/Documentation/security/keys/trusted-encrypted.rst
> @@ -350,7 +350,8 @@ Load an encrypted key "evm" from saved blob::
>
> Instantiate an encrypted key "evm" using user-provided decrypted data::
>
> - $ keyctl add encrypted evm "new default user:kmk 32 `cat evm_decrypted_data.blob`" @u
> + $ evmkey=$(dd if=/dev/urandom bs=1 count=32 | xxd -c32 -p)
> + $ keyctl add encrypted evm "new default user:kmk 32 $evmkey" @u
> 794890253
>
> $ keyctl print 794890253
> 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
>
Reviewed-by: Jarkko Sakkinen <[email protected]>
Unless there is opposing views, I can pick this.
BR, Jarkko
On Sun, Oct 23, 2022 at 08:18:58AM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 13, 2022 at 08:39:58AM +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.
> >
> > Old keys created from user provided decrypted data saved with "keyctl pipe"
> > are still valid, however if the key is recreated from decrypted data the
> > old key must be converted to the correct format. This can be done with a
> > small shell script, e.g.:
> >
> > BROKENKEY=abcdefABCDEF1234567890aaaaaaaaaa
> > NEWKEY=$(echo -ne $BROKENKEY | xxd -p -c32)
> > keyctl add user masterkey "$(cat masterkey.bin)" @u
> > keyctl add encrypted testkey "new user:masterkey 32 $NEWKEY" @u
> >
> > It is encouraged to switch to a new key because the effective key size
> > of the old keys is only half of the specified size.
> >
> > The corresponding test for the Linux Test Project ltp has also been
> > fixed (see link below).
> >
> > Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided decrypted data")
> > Cc: stable <[email protected]>
> > Link: https://lore.kernel.org/ltp/[email protected]/
> > Signed-off-by: Nikolaus Voss <[email protected]>
> > ---
> > Changes
> > =======
> > v4: - fixed link
> > v3: - use generated random key in example, reformat commit message
> > v2: - clarify commit message, add example to recover old/broken keys
> > - improve example in Documentation/security/keys/trusted-encrypted.rst
> > - add link to ltp patch
> >
> > Documentation/security/keys/trusted-encrypted.rst | 3 ++-
> > security/keys/encrypted-keys/encrypted.c | 6 +++---
> > 2 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/security/keys/trusted-encrypted.rst b/Documentation/security/keys/trusted-encrypted.rst
> > index 0bfb4c339748..9bc9db8ec651 100644
> > --- a/Documentation/security/keys/trusted-encrypted.rst
> > +++ b/Documentation/security/keys/trusted-encrypted.rst
> > @@ -350,7 +350,8 @@ Load an encrypted key "evm" from saved blob::
> >
> > Instantiate an encrypted key "evm" using user-provided decrypted data::
> >
> > - $ keyctl add encrypted evm "new default user:kmk 32 `cat evm_decrypted_data.blob`" @u
> > + $ evmkey=$(dd if=/dev/urandom bs=1 count=32 | xxd -c32 -p)
> > + $ keyctl add encrypted evm "new default user:kmk 32 $evmkey" @u
> > 794890253
> >
> > $ keyctl print 794890253
> > 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
> >
>
>
> Reviewed-by: Jarkko Sakkinen <[email protected]>
>
> Unless there is opposing views, I can pick this.
Actually, taking this back: please fix the checkpatch warnings first.
BR, Jarkko
On Sun, 23 Oct 2022, Jarkko Sakkinen wrote:
> On Sun, Oct 23, 2022 at 08:18:58AM +0300, Jarkko Sakkinen wrote:
>> On Thu, Oct 13, 2022 at 08:39:58AM +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.
>>>
>>> Old keys created from user provided decrypted data saved with "keyctl pipe"
>>> are still valid, however if the key is recreated from decrypted data the
>>> old key must be converted to the correct format. This can be done with a
>>> small shell script, e.g.:
>>>
>>> BROKENKEY=abcdefABCDEF1234567890aaaaaaaaaa
>>> NEWKEY=$(echo -ne $BROKENKEY | xxd -p -c32)
>>> keyctl add user masterkey "$(cat masterkey.bin)" @u
>>> keyctl add encrypted testkey "new user:masterkey 32 $NEWKEY" @u
>>>
>>> It is encouraged to switch to a new key because the effective key size
>>> of the old keys is only half of the specified size.
>>>
>>> The corresponding test for the Linux Test Project ltp has also been
>>> fixed (see link below).
>>>
>>> Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided decrypted data")
>>> Cc: stable <[email protected]>
>>> Link: https://lore.kernel.org/ltp/[email protected]/
>>> Signed-off-by: Nikolaus Voss <[email protected]>
>>> ---
>>> Changes
>>> =======
>>> v4: - fixed link
>>> v3: - use generated random key in example, reformat commit message
>>> v2: - clarify commit message, add example to recover old/broken keys
>>> - improve example in Documentation/security/keys/trusted-encrypted.rst
>>> - add link to ltp patch
>>>
>>> Documentation/security/keys/trusted-encrypted.rst | 3 ++-
>>> security/keys/encrypted-keys/encrypted.c | 6 +++---
>>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/security/keys/trusted-encrypted.rst b/Documentation/security/keys/trusted-encrypted.rst
>>> index 0bfb4c339748..9bc9db8ec651 100644
>>> --- a/Documentation/security/keys/trusted-encrypted.rst
>>> +++ b/Documentation/security/keys/trusted-encrypted.rst
>>> @@ -350,7 +350,8 @@ Load an encrypted key "evm" from saved blob::
>>>
>>> Instantiate an encrypted key "evm" using user-provided decrypted data::
>>>
>>> - $ keyctl add encrypted evm "new default user:kmk 32 `cat evm_decrypted_data.blob`" @u
>>> + $ evmkey=$(dd if=/dev/urandom bs=1 count=32 | xxd -c32 -p)
>>> + $ keyctl add encrypted evm "new default user:kmk 32 $evmkey" @u
>>> 794890253
>>>
>>> $ keyctl print 794890253
>>> 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
>>>
>>
>>
>> Reviewed-by: Jarkko Sakkinen <[email protected]>
>>
>> Unless there is opposing views, I can pick this.
>
> Actually, taking this back: please fix the checkpatch warnings first.
I fixed the warnings in v5, Mimi took this already.
Niko