Re: [PATCH v4] KEYS: encrypted: Instantiate key with user-provided decrypted data

[Nayna <nayna@linux.vnet.ibm.com>]

On 1/13/22 14:01, Yael Tiomkin wrote:
> On Mon, Jan 10, 2022 at 11:04 AM Nayna <nayna@linux.vnet.ibm.com> wrote:
>>
>> On 12/29/21 16:53, Yael Tiomkin wrote:
>>> The encrypted.c class supports instantiation of encrypted keys with
>>> either an already-encrypted key material, or by generating new key
>>> material based on random numbers. This patch defines a new datablob
>>> format: [<format>] <master-key name> <decrypted data length>
>>> <decrypted data> that allows to instantiate encrypted keys using
>>> user-provided decrypted data, and therefore allows to perform key
>>> encryption from userspace. The decrypted key material will be
>>> inaccessible from userspace.
>>>
>>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>>> Signed-off-by: Yael Tiomkin <yaelt@google.com>
>>> ---
>>>
>>> Notes:
>>>       v -> v2: fixed compilation error.
>>>
>>>       v2 -> v3: modified documentation.
>>>
>>>       v3 -> v4: modified commit message.
>>>
>>>    .../security/keys/trusted-encrypted.rst       | 25 ++++++--
>>>    security/keys/encrypted-keys/encrypted.c      | 62 +++++++++++++------
>>>    2 files changed, 63 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/Documentation/security/keys/trusted-encrypted.rst b/Documentation/security/keys/trusted-encrypted.rst
>>> index 80d5a5af62a1..f614dad7de12 100644
>>> --- a/Documentation/security/keys/trusted-encrypted.rst
>>> +++ b/Documentation/security/keys/trusted-encrypted.rst
>>> @@ -107,12 +107,13 @@ Encrypted Keys
>>>    --------------
>>>
>>>    Encrypted keys do not depend on a trust source, and are faster, as they use AES
>>> -for encryption/decryption. New keys are created from kernel-generated random
>>> -numbers, and are encrypted/decrypted using a specified ‘master’ key. The
>>> -‘master’ key can either be a trusted-key or user-key type. The main disadvantage
>>> -of encrypted keys is that if they are not rooted in a trusted key, they are only
>>> -as secure as the user key encrypting them. The master user key should therefore
>>> -be loaded in as secure a way as possible, preferably early in boot.
>>> +for encryption/decryption. New keys are created either from kernel-generated
>>> +random numbers or user-provided decrypted data, and are encrypted/decrypted
>>> +using a specified ‘master’ key. The ‘master’ key can either be a trusted-key or
>>> +user-key type. The main disadvantage of encrypted keys is that if they are not
>>> +rooted in a trusted key, they are only as secure as the user key encrypting
>>> +them. The master user key should therefore be loaded in as secure a way as
>>> +possible, preferably early in boot.
>>>
>>>
>>>    Usage
>>> @@ -199,6 +200,8 @@ Usage::
>>>
>>>        keyctl add encrypted name "new [format] key-type:master-key-name keylen"
>>>            ring
>>> +    keyctl add encrypted name "new [format] key-type:master-key-name keylen
>>> +        decrypted-data" ring
>>>        keyctl add encrypted name "load hex_blob" ring
>>>        keyctl update keyid "update key-type:master-key-name"
>>>
>>> @@ -303,6 +306,16 @@ Load an encrypted key "evm" from saved blob::
>>>        82dbbc55be2a44616e4959430436dc4f2a7a9659aa60bb4652aeb2120f149ed197c564e0
>>>        24717c64 5972dcb82ab2dde83376d82b2e3c09ffc
>>>
>>> +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
>>> +    794890253
>>> +
>>> +    $ keyctl print 794890253
>>> +    default user:kmk 32 2375725ad57798846a9bbd240de8906f006e66c03af53b1b382d
>>> +    bbc55be2a44616e4959430436dc4f2a7a9659aa60bb4652aeb2120f149ed197c564e0247
>>> +    17c64 5972dcb82ab2dde83376d82b2e3c09ffc
>>> +
>>>    Other uses for trusted and encrypted keys, such as for disk and file encryption
>>>    are anticipated.  In particular the new format 'ecryptfs' has been defined
>>>    in order to use encrypted keys to mount an eCryptfs filesystem.  More details
>>> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
>>> index 87432b35d771..baf6fba5e05e 100644
>>> --- a/security/keys/encrypted-keys/encrypted.c
>>> +++ b/security/keys/encrypted-keys/encrypted.c
>>> @@ -159,6 +159,7 @@ static int valid_master_desc(const char *new_desc, const char *orig_desc)
>>>     *
>>>     * datablob format:
>>>     * new [<format>] <master-key name> <decrypted data length>
>>> + * new [<format>] <master-key name> <decrypted data length> <decrypted data>
>>>     * load [<format>] <master-key name> <decrypted data length>
>>>     *     <encrypted iv + data>
>>>     * update <new-master-key name>
>>> @@ -170,7 +171,7 @@ static int valid_master_desc(const char *new_desc, const char *orig_desc)
>>>     */
>>>    static int datablob_parse(char *datablob, const char **format,
>>>                          char **master_desc, char **decrypted_datalen,
>>> -                       char **hex_encoded_iv)
>>> +                       char **hex_encoded_iv, char **decrypted_data)
>>>    {
>>>        substring_t args[MAX_OPT_ARGS];
>>>        int ret = -EINVAL;
>>> @@ -231,6 +232,8 @@ static int datablob_parse(char *datablob, const char **format,
>>>                                "when called from .update method\n", keyword);
>>>                        break;
>>>                }
>>> +             *decrypted_data = strsep(&datablob, " \t");
>>> +
>>>                ret = 0;
>>>                break;
>>>        case Opt_load:
>>> @@ -595,7 +598,8 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
>>>    static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
>>>                                                         const char *format,
>>>                                                         const char *master_desc,
>>> -                                                      const char *datalen)
>>> +                                                      const char *datalen,
>>> +                                                      const char *decrypted_data)
>>>    {
>>>        struct encrypted_key_payload *epayload = NULL;
>>>        unsigned short datablob_len;
>>> @@ -604,6 +608,7 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
>>>        unsigned int encrypted_datalen;
>>>        unsigned int format_len;
>>>        long dlen;
>>> +     int i;
>>>        int ret;
>>>
>>>        ret = kstrtol(datalen, 10, &dlen);
>>> @@ -613,6 +618,20 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
>>>        format_len = (!format) ? strlen(key_format_default) : strlen(format);
>>>        decrypted_datalen = dlen;
>>>        payload_datalen = decrypted_datalen;
>>> +
>>> +     if (decrypted_data) {
>>> +             if (strlen(decrypted_data) != decrypted_datalen) {
>>> +                     pr_err("encrypted key: decrypted data provided does not match decrypted data length provided\n");
>>> +                     return ERR_PTR(-EINVAL);
>>> +             }
>>> +             for (i = 0; i < strlen(decrypted_data); i++) {
>>> +                     if (!isalnum(decrypted_data[i])) {
>> User-provided decrypted data may have special characters, commonly used
>> in passwords or key phrases, apart from alphanumeric.  Replace isalnum
>> with !iscntrl() to validate against control characters but allow special
>> characters.
>>
>> Thanks & Regards,
>>
>>        - Nayna
>>
> Hi Nayna,
> I wonder if we should use isprint() instead?

I have been thinking more about this. Encrypted keys documentation says, 
"All user level blobs, are displayed and loaded in hex ascii for 
convenience."

Should we use a similar hex ascii encoding for user-provided data?  
Verification would then be isxdigit().

Users should convert their input to hex ascii before passing it to keyctl.

Thanks & Regards,

      - Nayna