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

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

[Yael Tiomkin]

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])) {
+				pr_err("encrypted key: decrypted data provided must be alphanumeric\n");
+				return ERR_PTR(-EINVAL);
+			}
+		}
+	}
+
 	if (format) {
 		if (!strcmp(format, key_format_ecryptfs)) {
 			if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
@@ -740,13 +759,14 @@ static void __ekey_init(struct encrypted_key_payload *epayload,
 /*
  * encrypted_init - initialize an encrypted key
  *
- * For a new key, use a random number for both the iv and data
- * itself.  For an old key, decrypt the hex encoded data.
+ * For a new key, use either a random number or user-provided decrypted data in
+ * case it is provided. A random number is used for the iv in both cases. For
+ * an old key, decrypt the hex encoded data.
  */
 static int encrypted_init(struct encrypted_key_payload *epayload,
 			  const char *key_desc, const char *format,
 			  const char *master_desc, const char *datalen,
-			  const char *hex_encoded_iv)
+			  const char *hex_encoded_iv, const char *decrypted_data)
 {
 	int ret = 0;
 
@@ -760,21 +780,26 @@ static int encrypted_init(struct encrypted_key_payload *epayload,
 	}
 
 	__ekey_init(epayload, format, master_desc, datalen);
-	if (!hex_encoded_iv) {
-		get_random_bytes(epayload->iv, ivsize);
-
-		get_random_bytes(epayload->decrypted_data,
-				 epayload->decrypted_datalen);
-	} else
+	if (hex_encoded_iv) {
 		ret = encrypted_key_decrypt(epayload, format, hex_encoded_iv);
+	} else if (decrypted_data) {
+		get_random_bytes(epayload->iv, ivsize);
+		memcpy(epayload->decrypted_data, decrypted_data,
+				epayload->decrypted_datalen);
+	} else {
+		get_random_bytes(epayload->iv, ivsize);
+		get_random_bytes(epayload->decrypted_data, epayload->decrypted_datalen);
+	}
 	return ret;
 }
 
 /*
  * encrypted_instantiate - instantiate an encrypted key
  *
- * Decrypt an existing encrypted datablob or create a new encrypted key
- * based on a kernel random number.
+ * Instantiates the key:
+ * - by decrypting an existing encrypted datablob, or
+ * - by creating a new encrypted key based on a kernel random number, or
+ * - using provided decrypted data.
  *
  * On success, return 0. Otherwise return errno.
  */
@@ -787,6 +812,7 @@ static int encrypted_instantiate(struct key *key,
 	char *master_desc = NULL;
 	char *decrypted_datalen = NULL;
 	char *hex_encoded_iv = NULL;
+	char *decrypted_data = NULL;
 	size_t datalen = prep->datalen;
 	int ret;
 
@@ -799,18 +825,18 @@ static int encrypted_instantiate(struct key *key,
 	datablob[datalen] = 0;
 	memcpy(datablob, prep->data, datalen);
 	ret = datablob_parse(datablob, &format, &master_desc,
-			     &decrypted_datalen, &hex_encoded_iv);
+			     &decrypted_datalen, &hex_encoded_iv, &decrypted_data);
 	if (ret < 0)
 		goto out;
 
 	epayload = encrypted_key_alloc(key, format, master_desc,
-				       decrypted_datalen);
+				       decrypted_datalen, decrypted_data);
 	if (IS_ERR(epayload)) {
 		ret = PTR_ERR(epayload);
 		goto out;
 	}
 	ret = encrypted_init(epayload, key->description, format, master_desc,
-			     decrypted_datalen, hex_encoded_iv);
+			     decrypted_datalen, hex_encoded_iv, decrypted_data);
 	if (ret < 0) {
 		kfree_sensitive(epayload);
 		goto out;
@@ -860,7 +886,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
 
 	buf[datalen] = 0;
 	memcpy(buf, prep->data, datalen);
-	ret = datablob_parse(buf, &format, &new_master_desc, NULL, NULL);
+	ret = datablob_parse(buf, &format, &new_master_desc, NULL, NULL, NULL);
 	if (ret < 0)
 		goto out;
 
@@ -869,7 +895,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
 		goto out;
 
 	new_epayload = encrypted_key_alloc(key, epayload->format,
-					   new_master_desc, epayload->datalen);
+					   new_master_desc, epayload->datalen, NULL);
 	if (IS_ERR(new_epayload)) {
 		ret = PTR_ERR(new_epayload);
 		goto out;
-- 
2.34.1.448.ga2b2bfdf31-goog

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

[Sumit Garg]

+ Jan, Ahmad

On Thu, 30 Dec 2021 at 03:24, Yael Tiomkin <yaelt@google.com> 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.

This type of user-space key import feature has already been discussed
at large in the context of trusted keys here [1]. So what makes it
special in case of encrypted keys such that it isn't a "UNSAFE_IMPORT"
or "DEBUGGING_IMPORT" or "DEVELOPMENT_IMPORT", ...?

[1] https://lore.kernel.org/linux-integrity/74830d4f-5a76-8ba8-aad0-0d79f7c01af9@pengutronix.de/

-Sumit

>
> 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])) {
> +                               pr_err("encrypted key: decrypted data provided must be alphanumeric\n");
> +                               return ERR_PTR(-EINVAL);
> +                       }
> +               }
> +       }
> +
>         if (format) {
>                 if (!strcmp(format, key_format_ecryptfs)) {
>                         if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> @@ -740,13 +759,14 @@ static void __ekey_init(struct encrypted_key_payload *epayload,
>  /*
>   * encrypted_init - initialize an encrypted key
>   *
> - * For a new key, use a random number for both the iv and data
> - * itself.  For an old key, decrypt the hex encoded data.
> + * For a new key, use either a random number or user-provided decrypted data in
> + * case it is provided. A random number is used for the iv in both cases. For
> + * an old key, decrypt the hex encoded data.
>   */
>  static int encrypted_init(struct encrypted_key_payload *epayload,
>                           const char *key_desc, const char *format,
>                           const char *master_desc, const char *datalen,
> -                         const char *hex_encoded_iv)
> +                         const char *hex_encoded_iv, const char *decrypted_data)
>  {
>         int ret = 0;
>
> @@ -760,21 +780,26 @@ static int encrypted_init(struct encrypted_key_payload *epayload,
>         }
>
>         __ekey_init(epayload, format, master_desc, datalen);
> -       if (!hex_encoded_iv) {
> -               get_random_bytes(epayload->iv, ivsize);
> -
> -               get_random_bytes(epayload->decrypted_data,
> -                                epayload->decrypted_datalen);
> -       } else
> +       if (hex_encoded_iv) {
>                 ret = encrypted_key_decrypt(epayload, format, hex_encoded_iv);
> +       } else if (decrypted_data) {
> +               get_random_bytes(epayload->iv, ivsize);
> +               memcpy(epayload->decrypted_data, decrypted_data,
> +                               epayload->decrypted_datalen);
> +       } else {
> +               get_random_bytes(epayload->iv, ivsize);
> +               get_random_bytes(epayload->decrypted_data, epayload->decrypted_datalen);
> +       }
>         return ret;
>  }
>
>  /*
>   * encrypted_instantiate - instantiate an encrypted key
>   *
> - * Decrypt an existing encrypted datablob or create a new encrypted key
> - * based on a kernel random number.
> + * Instantiates the key:
> + * - by decrypting an existing encrypted datablob, or
> + * - by creating a new encrypted key based on a kernel random number, or
> + * - using provided decrypted data.
>   *
>   * On success, return 0. Otherwise return errno.
>   */
> @@ -787,6 +812,7 @@ static int encrypted_instantiate(struct key *key,
>         char *master_desc = NULL;
>         char *decrypted_datalen = NULL;
>         char *hex_encoded_iv = NULL;
> +       char *decrypted_data = NULL;
>         size_t datalen = prep->datalen;
>         int ret;
>
> @@ -799,18 +825,18 @@ static int encrypted_instantiate(struct key *key,
>         datablob[datalen] = 0;
>         memcpy(datablob, prep->data, datalen);
>         ret = datablob_parse(datablob, &format, &master_desc,
> -                            &decrypted_datalen, &hex_encoded_iv);
> +                            &decrypted_datalen, &hex_encoded_iv, &decrypted_data);
>         if (ret < 0)
>                 goto out;
>
>         epayload = encrypted_key_alloc(key, format, master_desc,
> -                                      decrypted_datalen);
> +                                      decrypted_datalen, decrypted_data);
>         if (IS_ERR(epayload)) {
>                 ret = PTR_ERR(epayload);
>                 goto out;
>         }
>         ret = encrypted_init(epayload, key->description, format, master_desc,
> -                            decrypted_datalen, hex_encoded_iv);
> +                            decrypted_datalen, hex_encoded_iv, decrypted_data);
>         if (ret < 0) {
>                 kfree_sensitive(epayload);
>                 goto out;
> @@ -860,7 +886,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
>
>         buf[datalen] = 0;
>         memcpy(buf, prep->data, datalen);
> -       ret = datablob_parse(buf, &format, &new_master_desc, NULL, NULL);
> +       ret = datablob_parse(buf, &format, &new_master_desc, NULL, NULL, NULL);
>         if (ret < 0)
>                 goto out;
>
> @@ -869,7 +895,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
>                 goto out;
>
>         new_epayload = encrypted_key_alloc(key, epayload->format,
> -                                          new_master_desc, epayload->datalen);
> +                                          new_master_desc, epayload->datalen, NULL);
>         if (IS_ERR(new_epayload)) {
>                 ret = PTR_ERR(new_epayload);
>                 goto out;
> --
> 2.34.1.448.ga2b2bfdf31-goog
>

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

[Mimi Zohar]

Hi Sumit,

On Thu, 2021-12-30 at 15:37 +0530, Sumit Garg wrote:
> + Jan, Ahmad
> 
> On Thu, 30 Dec 2021 at 03:24, Yael Tiomkin <yaelt@google.com> 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.
> 
> This type of user-space key import feature has already been discussed
> at large in the context of trusted keys here [1]. So what makes it
> special in case of encrypted keys such that it isn't a "UNSAFE_IMPORT"
> or "DEBUGGING_IMPORT" or "DEVELOPMENT_IMPORT", ...?
> 
> [1] https://lore.kernel.org/linux-integrity/74830d4f-5a76-8ba8-aad0-0d79f7c01af9@pengutronix.de/
> 
> -Sumit
> 
> >
> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > Signed-off-by: Yael Tiomkin <yaelt@google.com>

There is a difference between trusted and encrypted keys.  So in
addition to pointing to the rather long discussion thread, please
summarize the conclusion and, assuming you agree, include why in once
case it was acceptable and in the other it wasn't to provide userspace
key data.

thanks,

Mimi

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

[Sumit Garg]

Hi Mimi,

Apologies for the delayed reply as I was on leave for a long new year weekend.

On Thu, 30 Dec 2021 at 18:59, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> Hi Sumit,
>
> On Thu, 2021-12-30 at 15:37 +0530, Sumit Garg wrote:
> > + Jan, Ahmad
> >
> > On Thu, 30 Dec 2021 at 03:24, Yael Tiomkin <yaelt@google.com> 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.
> >
> > This type of user-space key import feature has already been discussed
> > at large in the context of trusted keys here [1]. So what makes it
> > special in case of encrypted keys such that it isn't a "UNSAFE_IMPORT"
> > or "DEBUGGING_IMPORT" or "DEVELOPMENT_IMPORT", ...?
> >
> > [1] https://lore.kernel.org/linux-integrity/74830d4f-5a76-8ba8-aad0-0d79f7c01af9@pengutronix.de/
> >
> > -Sumit
> >
> > >
> > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > Signed-off-by: Yael Tiomkin <yaelt@google.com>
>
> There is a difference between trusted and encrypted keys.

Yeah I understand the implementation differences.

>  So in
> addition to pointing to the rather long discussion thread, please
> summarize the conclusion and, assuming you agree, include why in once
> case it was acceptable and in the other it wasn't to provide userspace
> key data.

My major concern with importing user-space key data in *plain* format
is that if import is *not* done in a safe (manufacturing or
production) environment then the plain key data is susceptible to
user-space compromises when the device is in the field.

And it sounds like we are diverting from basic definition [1] of encrypted keys:

"Trusted and Encrypted Keys are two new key types added to the
existing kernel key ring service. Both of these new types are variable
length symmetric keys, and in both cases all keys are created in the
kernel, and **user space sees, stores, and loads** only encrypted
blobs."

Also, as Jarrko mentioned earlier the use-case is still not clear to
me as well. Isn't user logon keys an alternative option for
non-readable user-space keys?

[1] https://www.kernel.org/doc/html/v4.13/security/keys/trusted-encrypted.html

-Sumit

>
> thanks,
>
> Mimi
>

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

[Jarkko Sakkinen]

On Wed, 2021-12-29 at 16:53 -0500, 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.

The 2nd to last sentence is essentially a tautology but fails to
be even that, as you can already "perform key encryption" from user
space, just not with arbitrary key material.

It does not elighten any applications of this feature.

/Jarkko

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

[Yael Tiomkin]

Hi Sumit,

On Mon, Jan 3, 2022 at 1:51 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Mimi,
>
> Apologies for the delayed reply as I was on leave for a long new year weekend.
>
> On Thu, 30 Dec 2021 at 18:59, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > Hi Sumit,
> >
> > On Thu, 2021-12-30 at 15:37 +0530, Sumit Garg wrote:
> > > + Jan, Ahmad
> > >
> > > On Thu, 30 Dec 2021 at 03:24, Yael Tiomkin <yaelt@google.com> 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.
> > >
> > > This type of user-space key import feature has already been discussed
> > > at large in the context of trusted keys here [1]. So what makes it
> > > special in case of encrypted keys such that it isn't a "UNSAFE_IMPORT"
> > > or "DEBUGGING_IMPORT" or "DEVELOPMENT_IMPORT", ...?
> > >
> > > [1] https://lore.kernel.org/linux-integrity/74830d4f-5a76-8ba8-aad0-0d79f7c01af9@pengutronix.de/
> > >
> > > -Sumit
> > >
> > > >
> > > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > Signed-off-by: Yael Tiomkin <yaelt@google.com>
> >
> > There is a difference between trusted and encrypted keys.
>
> Yeah I understand the implementation differences.
>
> >  So in
> > addition to pointing to the rather long discussion thread, please
> > summarize the conclusion and, assuming you agree, include why in once
> > case it was acceptable and in the other it wasn't to provide userspace
> > key data.
>
> My major concern with importing user-space key data in *plain* format
> is that if import is *not* done in a safe (manufacturing or
> production) environment then the plain key data is susceptible to
> user-space compromises when the device is in the field.

I agree this can happen. Key distribution in any scenario needs to be
secure and this could also potentially be an issue if the key is first
encrypted and then imported. We can make sure the documentation
highlights the safety requirement.

>
> And it sounds like we are diverting from basic definition [1] of encrypted keys:
>
> "Trusted and Encrypted Keys are two new key types added to the
> existing kernel key ring service. Both of these new types are variable
> length symmetric keys, and in both cases all keys are created in the
> kernel, and **user space sees, stores, and loads** only encrypted
> blobs."
>
> Also, as Jarrko mentioned earlier the use-case is still not clear to
> me as well. Isn't user logon keys an alternative option for
> non-readable user-space keys?

The goal in this change is to allow key encryption from userspace,
using user-provided decrypted data. This cannot be achieved in logon
keys, which as you mentioned, are simply non-readable user type keys.


>
> [1] https://www.kernel.org/doc/html/v4.13/security/keys/trusted-encrypted.html
>
> -Sumit
>
> >
> > thanks,
> >
> > Mimi
> >

Yael

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

[Yael Tiomkin]

On Wed, Jan 5, 2022 at 3:12 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Wed, 2021-12-29 at 16:53 -0500, 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.
>
> The 2nd to last sentence is essentially a tautology but fails to
> be even that, as you can already "perform key encryption" from user
> space, just not with arbitrary key material.
>
> It does not elighten any applications of this feature.
>
> /Jarkko

Sure. Please look at the modification below.

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 inject (and encrypt) user-provided
decrypted data. The decrypted key material will be inaccessible from
userspace. This feature also acts as a building block for a userspace
envelope encryption capability.

Yael

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

[Sumit Garg]

Hi Yael,

On Thu, 6 Jan 2022 at 01:48, Yael Tiomkin <yaelt@google.com> wrote:
>
> Hi Sumit,
>
> On Mon, Jan 3, 2022 at 1:51 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Mimi,
> >
> > Apologies for the delayed reply as I was on leave for a long new year weekend.
> >
> > On Thu, 30 Dec 2021 at 18:59, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >
> > > Hi Sumit,
> > >
> > > On Thu, 2021-12-30 at 15:37 +0530, Sumit Garg wrote:
> > > > + Jan, Ahmad
> > > >
> > > > On Thu, 30 Dec 2021 at 03:24, Yael Tiomkin <yaelt@google.com> 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.
> > > >
> > > > This type of user-space key import feature has already been discussed
> > > > at large in the context of trusted keys here [1]. So what makes it
> > > > special in case of encrypted keys such that it isn't a "UNSAFE_IMPORT"
> > > > or "DEBUGGING_IMPORT" or "DEVELOPMENT_IMPORT", ...?
> > > >
> > > > [1] https://lore.kernel.org/linux-integrity/74830d4f-5a76-8ba8-aad0-0d79f7c01af9@pengutronix.de/
> > > >
> > > > -Sumit
> > > >
> > > > >
> > > > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > > Signed-off-by: Yael Tiomkin <yaelt@google.com>
> > >
> > > There is a difference between trusted and encrypted keys.
> >
> > Yeah I understand the implementation differences.
> >
> > >  So in
> > > addition to pointing to the rather long discussion thread, please
> > > summarize the conclusion and, assuming you agree, include why in once
> > > case it was acceptable and in the other it wasn't to provide userspace
> > > key data.
> >
> > My major concern with importing user-space key data in *plain* format
> > is that if import is *not* done in a safe (manufacturing or
> > production) environment then the plain key data is susceptible to
> > user-space compromises when the device is in the field.
>
> I agree this can happen. Key distribution in any scenario needs to be
> secure and this could also potentially be an issue if the key is first
> encrypted and then imported.

Currently its not the case with encrypted keys. These are random keys
generated within the kernel and encrypted with master key within the
kernel and then exposed to user-space as encrypted blob only.

> We can make sure the documentation
> highlights the safety requirement.
>

IMO, you should enable this feature as a compile time option. The help
text for that config option should highlight the use-case along with a
safety warning.

-Sumit

> >
> > And it sounds like we are diverting from basic definition [1] of encrypted keys:
> >
> > "Trusted and Encrypted Keys are two new key types added to the
> > existing kernel key ring service. Both of these new types are variable
> > length symmetric keys, and in both cases all keys are created in the
> > kernel, and **user space sees, stores, and loads** only encrypted
> > blobs."
> >
> > Also, as Jarrko mentioned earlier the use-case is still not clear to
> > me as well. Isn't user logon keys an alternative option for
> > non-readable user-space keys?
>
> The goal in this change is to allow key encryption from userspace,
> using user-provided decrypted data. This cannot be achieved in logon
> keys, which as you mentioned, are simply non-readable user type keys.
>
>
> >
> > [1] https://www.kernel.org/doc/html/v4.13/security/keys/trusted-encrypted.html
> >
> > -Sumit
> >
> > >
> > > thanks,
> > >
> > > Mimi
> > >
>
> Yael

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

[Yael Tiomkin]

Hi Sumit,

On Fri, Jan 7, 2022 at 12:15 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Yael,
>
> On Thu, 6 Jan 2022 at 01:48, Yael Tiomkin <yaelt@google.com> wrote:
> >
> > Hi Sumit,
> >
> > On Mon, Jan 3, 2022 at 1:51 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > Hi Mimi,
> > >
> > > Apologies for the delayed reply as I was on leave for a long new year weekend.
> > >
> > > On Thu, 30 Dec 2021 at 18:59, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > >
> > > > Hi Sumit,
> > > >
> > > > On Thu, 2021-12-30 at 15:37 +0530, Sumit Garg wrote:
> > > > > + Jan, Ahmad
> > > > >
> > > > > On Thu, 30 Dec 2021 at 03:24, Yael Tiomkin <yaelt@google.com> 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.
> > > > >
> > > > > This type of user-space key import feature has already been discussed
> > > > > at large in the context of trusted keys here [1]. So what makes it
> > > > > special in case of encrypted keys such that it isn't a "UNSAFE_IMPORT"
> > > > > or "DEBUGGING_IMPORT" or "DEVELOPMENT_IMPORT", ...?
> > > > >
> > > > > [1] https://lore.kernel.org/linux-integrity/74830d4f-5a76-8ba8-aad0-0d79f7c01af9@pengutronix.de/
> > > > >
> > > > > -Sumit
> > > > >
> > > > > >
> > > > > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > > > Signed-off-by: Yael Tiomkin <yaelt@google.com>
> > > >
> > > > There is a difference between trusted and encrypted keys.
> > >
> > > Yeah I understand the implementation differences.
> > >
> > > >  So in
> > > > addition to pointing to the rather long discussion thread, please
> > > > summarize the conclusion and, assuming you agree, include why in once
> > > > case it was acceptable and in the other it wasn't to provide userspace
> > > > key data.
> > >
> > > My major concern with importing user-space key data in *plain* format
> > > is that if import is *not* done in a safe (manufacturing or
> > > production) environment then the plain key data is susceptible to
> > > user-space compromises when the device is in the field.
> >
> > I agree this can happen. Key distribution in any scenario needs to be
> > secure and this could also potentially be an issue if the key is first
> > encrypted and then imported.
>
> Currently its not the case with encrypted keys. These are random keys
> generated within the kernel and encrypted with master key within the
> kernel and then exposed to user-space as encrypted blob only.


There are two different ways to create encrypted keys. One is to have
them generated within the kernel using random numbers, and the other
is by importing them in their encrypted form from user-space.
I was referring to the latter in my previous statement.

>
> > We can make sure the documentation
> > highlights the safety requirement.
> >
>
> IMO, you should enable this feature as a compile time option. The help
> text for that config option should highlight the use-case along with a
> safety warning.
>
> -Sumit
>
> > >
> > > And it sounds like we are diverting from basic definition [1] of encrypted keys:
> > >
> > > "Trusted and Encrypted Keys are two new key types added to the
> > > existing kernel key ring service. Both of these new types are variable
> > > length symmetric keys, and in both cases all keys are created in the
> > > kernel, and **user space sees, stores, and loads** only encrypted
> > > blobs."
> > >
> > > Also, as Jarrko mentioned earlier the use-case is still not clear to
> > > me as well. Isn't user logon keys an alternative option for
> > > non-readable user-space keys?
> >
> > The goal in this change is to allow key encryption from userspace,
> > using user-provided decrypted data. This cannot be achieved in logon
> > keys, which as you mentioned, are simply non-readable user type keys.
> >
> >
> > >
> > > [1] https://www.kernel.org/doc/html/v4.13/security/keys/trusted-encrypted.html
> > >
> > > -Sumit
> > >
> > > >
> > > > thanks,
> > > >
> > > > Mimi
> > > >
> >
> > Yael

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

[Sumit Garg]

On Fri, 7 Jan 2022 at 18:23, Yael Tiomkin <yaelt@google.com> wrote:
>
> Hi Sumit,
>
> On Fri, Jan 7, 2022 at 12:15 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Yael,
> >
> > On Thu, 6 Jan 2022 at 01:48, Yael Tiomkin <yaelt@google.com> wrote:
> > >
> > > Hi Sumit,
> > >
> > > On Mon, Jan 3, 2022 at 1:51 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > Hi Mimi,
> > > >
> > > > Apologies for the delayed reply as I was on leave for a long new year weekend.
> > > >
> > > > On Thu, 30 Dec 2021 at 18:59, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > >
> > > > > Hi Sumit,
> > > > >
> > > > > On Thu, 2021-12-30 at 15:37 +0530, Sumit Garg wrote:
> > > > > > + Jan, Ahmad
> > > > > >
> > > > > > On Thu, 30 Dec 2021 at 03:24, Yael Tiomkin <yaelt@google.com> 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.
> > > > > >
> > > > > > This type of user-space key import feature has already been discussed
> > > > > > at large in the context of trusted keys here [1]. So what makes it
> > > > > > special in case of encrypted keys such that it isn't a "UNSAFE_IMPORT"
> > > > > > or "DEBUGGING_IMPORT" or "DEVELOPMENT_IMPORT", ...?
> > > > > >
> > > > > > [1] https://lore.kernel.org/linux-integrity/74830d4f-5a76-8ba8-aad0-0d79f7c01af9@pengutronix.de/
> > > > > >
> > > > > > -Sumit
> > > > > >
> > > > > > >
> > > > > > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > > > > Signed-off-by: Yael Tiomkin <yaelt@google.com>
> > > > >
> > > > > There is a difference between trusted and encrypted keys.
> > > >
> > > > Yeah I understand the implementation differences.
> > > >
> > > > >  So in
> > > > > addition to pointing to the rather long discussion thread, please
> > > > > summarize the conclusion and, assuming you agree, include why in once
> > > > > case it was acceptable and in the other it wasn't to provide userspace
> > > > > key data.
> > > >
> > > > My major concern with importing user-space key data in *plain* format
> > > > is that if import is *not* done in a safe (manufacturing or
> > > > production) environment then the plain key data is susceptible to
> > > > user-space compromises when the device is in the field.
> > >
> > > I agree this can happen. Key distribution in any scenario needs to be
> > > secure and this could also potentially be an issue if the key is first
> > > encrypted and then imported.
> >
> > Currently its not the case with encrypted keys. These are random keys
> > generated within the kernel and encrypted with master key within the
> > kernel and then exposed to user-space as encrypted blob only.
>
>
> There are two different ways to create encrypted keys. One is to have
> them generated within the kernel using random numbers, and the other
> is by importing them in their encrypted form from user-space.
> I was referring to the latter in my previous statement.
>

So, from a key distribution security point of view, encrypted key
user-space import is **not equal to** plain key user-space import.
That's why we need to have a separate compile time option as every
device may come with its own threat model and may choose to enable or
disable this user-space plain key import feature.

-Sumit

> >
> > > We can make sure the documentation
> > > highlights the safety requirement.
> > >
> >
> > IMO, you should enable this feature as a compile time option. The help
> > text for that config option should highlight the use-case along with a
> > safety warning.
> >
> > -Sumit
> >
> > > >
> > > > And it sounds like we are diverting from basic definition [1] of encrypted keys:
> > > >
> > > > "Trusted and Encrypted Keys are two new key types added to the
> > > > existing kernel key ring service. Both of these new types are variable
> > > > length symmetric keys, and in both cases all keys are created in the
> > > > kernel, and **user space sees, stores, and loads** only encrypted
> > > > blobs."
> > > >
> > > > Also, as Jarrko mentioned earlier the use-case is still not clear to
> > > > me as well. Isn't user logon keys an alternative option for
> > > > non-readable user-space keys?
> > >
> > > The goal in this change is to allow key encryption from userspace,
> > > using user-provided decrypted data. This cannot be achieved in logon
> > > keys, which as you mentioned, are simply non-readable user type keys.
> > >
> > >
> > > >
> > > > [1] https://www.kernel.org/doc/html/v4.13/security/keys/trusted-encrypted.html
> > > >
> > > > -Sumit
> > > >
> > > > >
> > > > > thanks,
> > > > >
> > > > > Mimi
> > > > >
> > >
> > > Yael

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

[Jarkko Sakkinen]

On Wed, Dec 29, 2021 at 04:53:30PM -0500, 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>

What is the use case for this?

BR, Jarkko

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

[Nayna]

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

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

[Yael Tiomkin]

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

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

[Yael Tiomkin]

On Fri, Jan 7, 2022 at 8:32 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Fri, 7 Jan 2022 at 18:23, Yael Tiomkin <yaelt@google.com> wrote:
> >
> > Hi Sumit,
> >
> > On Fri, Jan 7, 2022 at 12:15 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > Hi Yael,
> > >
> > > On Thu, 6 Jan 2022 at 01:48, Yael Tiomkin <yaelt@google.com> wrote:
> > > >
> > > > Hi Sumit,
> > > >
> > > > On Mon, Jan 3, 2022 at 1:51 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > > >
> > > > > Hi Mimi,
> > > > >
> > > > > Apologies for the delayed reply as I was on leave for a long new year weekend.
> > > > >
> > > > > On Thu, 30 Dec 2021 at 18:59, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > > >
> > > > > > Hi Sumit,
> > > > > >
> > > > > > On Thu, 2021-12-30 at 15:37 +0530, Sumit Garg wrote:
> > > > > > > + Jan, Ahmad
> > > > > > >
> > > > > > > On Thu, 30 Dec 2021 at 03:24, Yael Tiomkin <yaelt@google.com> 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.
> > > > > > >
> > > > > > > This type of user-space key import feature has already been discussed
> > > > > > > at large in the context of trusted keys here [1]. So what makes it
> > > > > > > special in case of encrypted keys such that it isn't a "UNSAFE_IMPORT"
> > > > > > > or "DEBUGGING_IMPORT" or "DEVELOPMENT_IMPORT", ...?
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/linux-integrity/74830d4f-5a76-8ba8-aad0-0d79f7c01af9@pengutronix.de/
> > > > > > >
> > > > > > > -Sumit
> > > > > > >
> > > > > > > >
> > > > > > > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > > > > > Signed-off-by: Yael Tiomkin <yaelt@google.com>
> > > > > >
> > > > > > There is a difference between trusted and encrypted keys.
> > > > >
> > > > > Yeah I understand the implementation differences.
> > > > >
> > > > > >  So in
> > > > > > addition to pointing to the rather long discussion thread, please
> > > > > > summarize the conclusion and, assuming you agree, include why in once
> > > > > > case it was acceptable and in the other it wasn't to provide userspace
> > > > > > key data.
> > > > >
> > > > > My major concern with importing user-space key data in *plain* format
> > > > > is that if import is *not* done in a safe (manufacturing or
> > > > > production) environment then the plain key data is susceptible to
> > > > > user-space compromises when the device is in the field.
> > > >
> > > > I agree this can happen. Key distribution in any scenario needs to be
> > > > secure and this could also potentially be an issue if the key is first
> > > > encrypted and then imported.
> > >
> > > Currently its not the case with encrypted keys. These are random keys
> > > generated within the kernel and encrypted with master key within the
> > > kernel and then exposed to user-space as encrypted blob only.
> >
> >
> > There are two different ways to create encrypted keys. One is to have
> > them generated within the kernel using random numbers, and the other
> > is by importing them in their encrypted form from user-space.
> > I was referring to the latter in my previous statement.
> >
>
> So, from a key distribution security point of view, encrypted key
> user-space import is **not equal to** plain key user-space import.
> That's why we need to have a separate compile time option as every
> device may come with its own threat model and may choose to enable or
> disable this user-space plain key import feature.
>
> -Sumit
>
> > >
> > > > We can make sure the documentation
> > > > highlights the safety requirement.
> > > >
> > >
> > > IMO, you should enable this feature as a compile time option. The help
> > > text for that config option should highlight the use-case along with a
> > > safety warning.
> > >
> > > -Sumit
> > >
> > > > >
> > > > > And it sounds like we are diverting from basic definition [1] of encrypted keys:
> > > > >
> > > > > "Trusted and Encrypted Keys are two new key types added to the
> > > > > existing kernel key ring service. Both of these new types are variable
> > > > > length symmetric keys, and in both cases all keys are created in the
> > > > > kernel, and **user space sees, stores, and loads** only encrypted
> > > > > blobs."
> > > > >
> > > > > Also, as Jarrko mentioned earlier the use-case is still not clear to
> > > > > me as well. Isn't user logon keys an alternative option for
> > > > > non-readable user-space keys?
> > > >
> > > > The goal in this change is to allow key encryption from userspace,
> > > > using user-provided decrypted data. This cannot be achieved in logon
> > > > keys, which as you mentioned, are simply non-readable user type keys.
> > > >
> > > >
> > > > >
> > > > > [1] https://www.kernel.org/doc/html/v4.13/security/keys/trusted-encrypted.html
> > > > >
> > > > > -Sumit
> > > > >
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > Mimi
> > > > > >
> > > >
> > > > Yael

Hi Sumit,
The encrypted-keys module is already controlled by the
CONFIG_ENCRYPTED_KEYS option, which I think might give sufficient
granularity to control the behavior. Do you still think a feature
dedicated option is needed?

Yael

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

[Sumit Garg]

On Fri, 14 Jan 2022 at 00:45, Yael Tiomkin <yaelt@google.com> wrote:
>
> On Fri, Jan 7, 2022 at 8:32 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Fri, 7 Jan 2022 at 18:23, Yael Tiomkin <yaelt@google.com> wrote:
> > >
> > > Hi Sumit,
> > >
> > > On Fri, Jan 7, 2022 at 12:15 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > Hi Yael,
> > > >
> > > > On Thu, 6 Jan 2022 at 01:48, Yael Tiomkin <yaelt@google.com> wrote:
> > > > >
> > > > > Hi Sumit,
> > > > >
> > > > > On Mon, Jan 3, 2022 at 1:51 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > > > >
> > > > > > Hi Mimi,
> > > > > >
> > > > > > Apologies for the delayed reply as I was on leave for a long new year weekend.
> > > > > >
> > > > > > On Thu, 30 Dec 2021 at 18:59, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > > > >
> > > > > > > Hi Sumit,
> > > > > > >
> > > > > > > On Thu, 2021-12-30 at 15:37 +0530, Sumit Garg wrote:
> > > > > > > > + Jan, Ahmad
> > > > > > > >
> > > > > > > > On Thu, 30 Dec 2021 at 03:24, Yael Tiomkin <yaelt@google.com> 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.
> > > > > > > >
> > > > > > > > This type of user-space key import feature has already been discussed
> > > > > > > > at large in the context of trusted keys here [1]. So what makes it
> > > > > > > > special in case of encrypted keys such that it isn't a "UNSAFE_IMPORT"
> > > > > > > > or "DEBUGGING_IMPORT" or "DEVELOPMENT_IMPORT", ...?
> > > > > > > >
> > > > > > > > [1] https://lore.kernel.org/linux-integrity/74830d4f-5a76-8ba8-aad0-0d79f7c01af9@pengutronix.de/
> > > > > > > >
> > > > > > > > -Sumit
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > > > > > > Signed-off-by: Yael Tiomkin <yaelt@google.com>
> > > > > > >
> > > > > > > There is a difference between trusted and encrypted keys.
> > > > > >
> > > > > > Yeah I understand the implementation differences.
> > > > > >
> > > > > > >  So in
> > > > > > > addition to pointing to the rather long discussion thread, please
> > > > > > > summarize the conclusion and, assuming you agree, include why in once
> > > > > > > case it was acceptable and in the other it wasn't to provide userspace
> > > > > > > key data.
> > > > > >
> > > > > > My major concern with importing user-space key data in *plain* format
> > > > > > is that if import is *not* done in a safe (manufacturing or
> > > > > > production) environment then the plain key data is susceptible to
> > > > > > user-space compromises when the device is in the field.
> > > > >
> > > > > I agree this can happen. Key distribution in any scenario needs to be
> > > > > secure and this could also potentially be an issue if the key is first
> > > > > encrypted and then imported.
> > > >
> > > > Currently its not the case with encrypted keys. These are random keys
> > > > generated within the kernel and encrypted with master key within the
> > > > kernel and then exposed to user-space as encrypted blob only.
> > >
> > >
> > > There are two different ways to create encrypted keys. One is to have
> > > them generated within the kernel using random numbers, and the other
> > > is by importing them in their encrypted form from user-space.
> > > I was referring to the latter in my previous statement.
> > >
> >
> > So, from a key distribution security point of view, encrypted key
> > user-space import is **not equal to** plain key user-space import.
> > That's why we need to have a separate compile time option as every
> > device may come with its own threat model and may choose to enable or
> > disable this user-space plain key import feature.
> >
> > -Sumit
> >
> > > >
> > > > > We can make sure the documentation
> > > > > highlights the safety requirement.
> > > > >
> > > >
> > > > IMO, you should enable this feature as a compile time option. The help
> > > > text for that config option should highlight the use-case along with a
> > > > safety warning.
> > > >
> > > > -Sumit
> > > >
> > > > > >
> > > > > > And it sounds like we are diverting from basic definition [1] of encrypted keys:
> > > > > >
> > > > > > "Trusted and Encrypted Keys are two new key types added to the
> > > > > > existing kernel key ring service. Both of these new types are variable
> > > > > > length symmetric keys, and in both cases all keys are created in the
> > > > > > kernel, and **user space sees, stores, and loads** only encrypted
> > > > > > blobs."
> > > > > >
> > > > > > Also, as Jarrko mentioned earlier the use-case is still not clear to
> > > > > > me as well. Isn't user logon keys an alternative option for
> > > > > > non-readable user-space keys?
> > > > >
> > > > > The goal in this change is to allow key encryption from userspace,
> > > > > using user-provided decrypted data. This cannot be achieved in logon
> > > > > keys, which as you mentioned, are simply non-readable user type keys.
> > > > >
> > > > >
> > > > > >
> > > > > > [1] https://www.kernel.org/doc/html/v4.13/security/keys/trusted-encrypted.html
> > > > > >
> > > > > > -Sumit
> > > > > >
> > > > > > >
> > > > > > > thanks,
> > > > > > >
> > > > > > > Mimi
> > > > > > >
> > > > >
> > > > > Yael
>
> Hi Sumit,
> The encrypted-keys module is already controlled by the
> CONFIG_ENCRYPTED_KEYS option, which I think might give sufficient
> granularity to control the behavior. Do you still think a feature
> dedicated option is needed?
>

Since this feature will change security properties for encrypted keys
which can be considered as a regression for existing users during
uprev. So it needs to be an optional key import feature with a clear
user visible warning. Thinking more about this, you can even have a
module param to turn it on with a runtime warning as well.

-Sumit

> Yael

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

[Nayna]

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

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

[Jarkko Sakkinen]

On Wed, Jan 26, 2022 at 03:56:44PM -0500, Yael Tiomkin wrote:
> On Wed, Jan 26, 2022 at 9:51 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Wed, Jan 26, 2022 at 04:47:22PM +0200, Jarkko Sakkinen wrote:
> > > On Tue, Jan 18, 2022 at 01:26:05PM -0500, Martin Ross wrote:
> > > > Hi Jarkko,
> > > >
> > > > I have been working with Yael on this project so I thought I might add
> > > > a bit of background here around the use case that this series of
> > > > patches is trying to address.
> > > >
> > > > At a high level we are trying to provide users of encryption that have
> > > > key management hierarchies a better tradeoff between security and
> > > > availability.  For available and performance reasons master keys often
> > > > need to be released (or derived/wrapped keys created) outside of a KMS
> > > > to clients (which may in turn further wrap those keys in a series of
> > > > levels).  What we are trying to do is provide a mechanism where the
> > > > wrapping/unwrapping of these keys is not dependent on a remote call at
> > > > runtime.  e.g.  To unwrap a key if you are using AWS KMS or Google
> > > > Service you need to make an RPC.  In practice to defend against
> > > > availability or performance issues, designers end up building their
> > > > own kms and effectively encrypting everything with a DEK.  The DEK
> > > > encrypts same set as the master key thereby eliminating the security
> > > > benefit of keeping the master key segregated in the first place.
> >
> > Mainly this part (would be enough to explain why it is there).
> >
> > BR, Jarkko
> 
> Hi Jarkko,
> 
> As for the commit message, WDYT about the following:
> 
> KEYS: encrypted: Instantiate key with user-provided decrypted data
> 
> For availability and performance reasons master keys often need to be
> released outside of a KMS to clients. It would be beneficial to provide a
> mechanism where the wrapping/unwrapping of DEKs is not dependent
> on a remote call at runtime yet security is not (or only minimally) compromised.
> Master keys could be securely stored in the Kernel and be used to wrap/unwrap
> keys from userspace.
> 
> 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 inject and encrypt user-provided
> decrypted data.
> 
> 
> I want to make sure we're on the same page before publishing a new version.
> 
> Thanks,
> Yael

It looks really good.

/Jarkko

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

[Yael Tiomkin]

On Wed, Jan 26, 2022 at 9:51 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Wed, Jan 26, 2022 at 04:47:22PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Jan 18, 2022 at 01:26:05PM -0500, Martin Ross wrote:
> > > Hi Jarkko,
> > >
> > > I have been working with Yael on this project so I thought I might add
> > > a bit of background here around the use case that this series of
> > > patches is trying to address.
> > >
> > > At a high level we are trying to provide users of encryption that have
> > > key management hierarchies a better tradeoff between security and
> > > availability.  For available and performance reasons master keys often
> > > need to be released (or derived/wrapped keys created) outside of a KMS
> > > to clients (which may in turn further wrap those keys in a series of
> > > levels).  What we are trying to do is provide a mechanism where the
> > > wrapping/unwrapping of these keys is not dependent on a remote call at
> > > runtime.  e.g.  To unwrap a key if you are using AWS KMS or Google
> > > Service you need to make an RPC.  In practice to defend against
> > > availability or performance issues, designers end up building their
> > > own kms and effectively encrypting everything with a DEK.  The DEK
> > > encrypts same set as the master key thereby eliminating the security
> > > benefit of keeping the master key segregated in the first place.
>
> Mainly this part (would be enough to explain why it is there).
>
> BR, Jarkko

Hi Jarkko,

As for the commit message, WDYT about the following:

KEYS: encrypted: Instantiate key with user-provided decrypted data

For availability and performance reasons master keys often need to be
released outside of a KMS to clients. It would be beneficial to provide a
mechanism where the wrapping/unwrapping of DEKs is not dependent
on a remote call at runtime yet security is not (or only minimally) compromised.
Master keys could be securely stored in the Kernel and be used to wrap/unwrap
keys from userspace.

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 inject and encrypt user-provided
decrypted data.


I want to make sure we're on the same page before publishing a new version.

Thanks,
Yael

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

[Jarkko Sakkinen]

On Wed, Jan 26, 2022 at 04:47:22PM +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 18, 2022 at 01:26:05PM -0500, Martin Ross wrote:
> > Hi Jarkko,
> > 
> > I have been working with Yael on this project so I thought I might add
> > a bit of background here around the use case that this series of
> > patches is trying to address.
> > 
> > At a high level we are trying to provide users of encryption that have
> > key management hierarchies a better tradeoff between security and
> > availability.  For available and performance reasons master keys often
> > need to be released (or derived/wrapped keys created) outside of a KMS
> > to clients (which may in turn further wrap those keys in a series of
> > levels).  What we are trying to do is provide a mechanism where the
> > wrapping/unwrapping of these keys is not dependent on a remote call at
> > runtime.  e.g.  To unwrap a key if you are using AWS KMS or Google
> > Service you need to make an RPC.  In practice to defend against
> > availability or performance issues, designers end up building their
> > own kms and effectively encrypting everything with a DEK.  The DEK
> > encrypts same set as the master key thereby eliminating the security
> > benefit of keeping the master key segregated in the first place.

Mainly this part (would be enough to explain why it is there).

BR, Jarkko

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

[Jarkko Sakkinen]

On Tue, Jan 18, 2022 at 01:26:05PM -0500, Martin Ross wrote:
> Hi Jarkko,
> 
> I have been working with Yael on this project so I thought I might add
> a bit of background here around the use case that this series of
> patches is trying to address.
> 
> At a high level we are trying to provide users of encryption that have
> key management hierarchies a better tradeoff between security and
> availability.  For available and performance reasons master keys often
> need to be released (or derived/wrapped keys created) outside of a KMS
> to clients (which may in turn further wrap those keys in a series of
> levels).  What we are trying to do is provide a mechanism where the
> wrapping/unwrapping of these keys is not dependent on a remote call at
> runtime.  e.g.  To unwrap a key if you are using AWS KMS or Google
> Service you need to make an RPC.  In practice to defend against
> availability or performance issues, designers end up building their
> own kms and effectively encrypting everything with a DEK.  The DEK
> encrypts same set as the master key thereby eliminating the security
> benefit of keeping the master key segregated in the first place.
> 
> We are building a mechanism to  create a security boundary in the
> kernel that allows these master keys to be stored in the kernel and
> used to wrap/unwrap keys via less trusted user processes.  The other
> goal here is to eliminate the complexity and statefulness required to
> do this today which would be to create a trusted daemon or process on
> the machine.  Concretely this means that since the user process will
> not have the master key the system designer has better options.  One
> obvious advantage is that any core dumps or code injection attacks
> won't be able to trivially grab the master key from the process or the
> linux keyring.  Once in the kernel this functionality can be
> transparently integrated into user space crypto libraries that have
> existing key management functionality.
> 
> Hope this helps and happy to answer any further questions!
> 
> M

Thank you.

It indeed does. I think it is a good explanation. Maybe the way to move
forward would be bring this context at leat a bit to the documentation
update?

/Jarkko

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

[Martin Ross]

Hi Jarkko,

I have been working with Yael on this project so I thought I might add
a bit of background here around the use case that this series of
patches is trying to address.

At a high level we are trying to provide users of encryption that have
key management hierarchies a better tradeoff between security and
availability.  For available and performance reasons master keys often
need to be released (or derived/wrapped keys created) outside of a KMS
to clients (which may in turn further wrap those keys in a series of
levels).  What we are trying to do is provide a mechanism where the
wrapping/unwrapping of these keys is not dependent on a remote call at
runtime.  e.g.  To unwrap a key if you are using AWS KMS or Google
Service you need to make an RPC.  In practice to defend against
availability or performance issues, designers end up building their
own kms and effectively encrypting everything with a DEK.  The DEK
encrypts same set as the master key thereby eliminating the security
benefit of keeping the master key segregated in the first place.

We are building a mechanism to  create a security boundary in the
kernel that allows these master keys to be stored in the kernel and
used to wrap/unwrap keys via less trusted user processes.  The other
goal here is to eliminate the complexity and statefulness required to
do this today which would be to create a trusted daemon or process on
the machine.  Concretely this means that since the user process will
not have the master key the system designer has better options.  One
obvious advantage is that any core dumps or code injection attacks
won't be able to trivially grab the master key from the process or the
linux keyring.  Once in the kernel this functionality can be
transparently integrated into user space crypto libraries that have
existing key management functionality.

Hope this helps and happy to answer any further questions!

M