Re: [PATCH] KEYS: encrypted: fix key instantiation with user-provided data
From: Nikolaus Voss
Date: Wed Sep 28 2022 - 08:08:54 EST
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