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

[Yael Tiomkin <yaelt@google.com>]

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?

Yael