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