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

[Mimi Zohar <zohar@linux.ibm.com>]

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