Messages in this thread | | | Date | Wed, 5 Oct 2022 12:04:22 +0200 (CEST) | From | Nikolaus Voss <> | Subject | Re: [PATCH] KEYS: encrypted: fix key instantiation with user-provided data |
| |
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
| |