[PATCH v2] fscrypt: support trusted keys

[PATCH v2] fscrypt: support trusted keys

[Ahmad Fatoum]

Kernel trusted keys don't require userspace knowledge of the raw key
material and instead export a sealed blob, which can be persisted to
unencrypted storage. Userspace can then load this blob into the kernel,
where it's unsealed and from there on usable for kernel crypto.

This is incompatible with fscrypt, where userspace is supposed to supply
the raw key material. For TPMs, a work around is to do key unsealing in
userspace, but this may not be feasible for other trusted key backends.

Make it possible to benefit from both fscrypt and trusted key sealing
by extending fscrypt_add_key_arg::key_id to hold either the ID of a
fscrypt-provisioning or a trusted key.

A non fscrypt-provisioning key_id was so far prohibited, so additionally
allowing trusted keys won't break backwards compatibility.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
Tested with:
https://github.com/google/fscryptctl/pull/23

v1 here:
https://lore.kernel.org/linux-fscrypt/20210727144349.11215-1-a.fatoum@pengutronix.de/T/#u

v1 -> v2:
  - Drop encrypted key support and key_extract_material
  - Use key_id instead of repurposing raw (Eric)
  - Shift focus to trusted key sealing for non-TPM as a rationale
    why this integration is worthwhile (Eric)
  - Extend documentation with rationale on why one would
    use trusted keys and warn about trusted key reuse

To: "Theodore Y. Ts'o" <tytso@mit.edu>
To: Jaegeuk Kim <jaegeuk@kernel.org>
To: Eric Biggers <ebiggers@kernel.org>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Sumit Garg <sumit.garg@linaro.org>
Cc: David Howells <dhowells@redhat.com>
Cc: linux-fscrypt@vger.kernel.org
Cc: linux-crypto@vger.kernel.org
Cc: linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Cc: keyrings@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 Documentation/filesystems/fscrypt.rst | 31 ++++++++++++++-----
 fs/crypto/keyring.c                   | 43 +++++++++++++++++++--------
 2 files changed, 54 insertions(+), 20 deletions(-)

diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
index 44b67ebd6e40..c1811fa4285a 100644
--- a/Documentation/filesystems/fscrypt.rst
+++ b/Documentation/filesystems/fscrypt.rst
@@ -734,23 +734,40 @@ as follows:
 
 - ``key_id`` is 0 if the raw key is given directly in the ``raw``
   field.  Otherwise ``key_id`` is the ID of a Linux keyring key of
-  type "fscrypt-provisioning" whose payload is
+  type "fscrypt-provisioning" or "trusted":
+  "fscrypt-provisioning" keys have a payload of
   struct fscrypt_provisioning_key_payload whose ``raw`` field contains
   the raw key and whose ``type`` field matches ``key_spec.type``.
   Since ``raw`` is variable-length, the total size of this key's
   payload must be ``sizeof(struct fscrypt_provisioning_key_payload)``
-  plus the raw key size.  The process must have Search permission on
-  this key.
-
-  Most users should leave this 0 and specify the raw key directly.
-  The support for specifying a Linux keyring key is intended mainly to
-  allow re-adding keys after a filesystem is unmounted and re-mounted,
+  plus the raw key size.
+  For "trusted" keys, the payload is directly taken as the raw key.
+
+  The process must have Search permission on this key.
+
+  Most users leave this 0 and specify the raw key directly.
+  "trusted" keys are useful to leverage kernel support for sealing
+  and unsealing key material. Sealed keys can be persisted to
+  unencrypted storage and later be used to decrypt the file system
+  without requiring userspace to have knowledge of the raw key
+  material.
+  "fscrypt-provisioning" key support is intended mainly to allow
+  re-adding keys after a filesystem is unmounted and re-mounted,
   without having to store the raw keys in userspace memory.
 
 - ``raw`` is a variable-length field which must contain the actual
   key, ``raw_size`` bytes long.  Alternatively, if ``key_id`` is
   nonzero, then this field is unused.
 
+.. note::
+
+   Users should take care not to reuse the fscrypt key material with
+   different ciphers or in multiple contexts as this may make it
+   easier to deduce the key.
+   This also applies when the key material is supplied indirectly
+   via a kernel trusted key. In this case, the trusted key should
+   perferably be used only in a single context.
+
 For v2 policy keys, the kernel keeps track of which user (identified
 by effective user ID) added the key, and only allows the key to be
 removed by that user --- or by "root", if they use
diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index 0b3ffbb4faf4..721f5da51416 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -20,6 +20,7 @@
 
 #include <crypto/skcipher.h>
 #include <linux/key-type.h>
+#include <keys/trusted-type.h>
 #include <linux/random.h>
 #include <linux/seq_file.h>
 
@@ -577,28 +578,44 @@ static int get_keyring_key(u32 key_id, u32 type,
 	key_ref_t ref;
 	struct key *key;
 	const struct fscrypt_provisioning_key_payload *payload;
-	int err;
+	int err = 0;
 
 	ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
 	if (IS_ERR(ref))
 		return PTR_ERR(ref);
 	key = key_ref_to_ptr(ref);
 
-	if (key->type != &key_type_fscrypt_provisioning)
-		goto bad_key;
-	payload = key->payload.data[0];
+	if (key->type == &key_type_fscrypt_provisioning) {
+		payload = key->payload.data[0];
 
-	/* Don't allow fscrypt v1 keys to be used as v2 keys and vice versa. */
-	if (payload->type != type)
-		goto bad_key;
+		/* Don't allow fscrypt v1 keys to be used as v2 keys and vice versa. */
+		if (payload->type != type) {
+			err = -EKEYREJECTED;
+			goto out_put;
+		}
 
-	secret->size = key->datalen - sizeof(*payload);
-	memcpy(secret->raw, payload->raw, secret->size);
-	err = 0;
-	goto out_put;
+		secret->size = key->datalen - sizeof(*payload);
+		memcpy(secret->raw, payload->raw, secret->size);
+	} else if (IS_REACHABLE(CONFIG_TRUSTED_KEYS) && key->type == &key_type_trusted) {
+		struct trusted_key_payload *tkp;
+
+		/* avoid reseal changing payload while we memcpy key */
+		down_read(&key->sem);
+		tkp = key->payload.data[0];
+		if (!tkp || tkp->key_len < FSCRYPT_MIN_KEY_SIZE ||
+		    tkp->key_len > FSCRYPT_MAX_KEY_SIZE) {
+			up_read(&key->sem);
+			err = -EINVAL;
+			goto out_put;
+		}
+
+		secret->size = tkp->key_len;
+		memcpy(secret->raw, tkp->key, secret->size);
+		up_read(&key->sem);
+	} else {
+		err = -EKEYREJECTED;
+	}
 
-bad_key:
-	err = -EKEYREJECTED;
 out_put:
 	key_ref_put(ref);
 	return err;
-- 
2.30.2

Re: [PATCH v2] fscrypt: support trusted keys

[Jarkko Sakkinen]

On Fri, Aug 06, 2021 at 05:09:28PM +0200, Ahmad Fatoum wrote:
> Kernel trusted keys don't require userspace knowledge of the raw key
> material and instead export a sealed blob, which can be persisted to
> unencrypted storage. Userspace can then load this blob into the kernel,
> where it's unsealed and from there on usable for kernel crypto.
> 
> This is incompatible with fscrypt, where userspace is supposed to supply
> the raw key material. For TPMs, a work around is to do key unsealing in
> userspace, but this may not be feasible for other trusted key backends.
> 
> Make it possible to benefit from both fscrypt and trusted key sealing
> by extending fscrypt_add_key_arg::key_id to hold either the ID of a
> fscrypt-provisioning or a trusted key.
> 
> A non fscrypt-provisioning key_id was so far prohibited, so additionally
> allowing trusted keys won't break backwards compatibility.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> Tested with:
> https://github.com/google/fscryptctl/pull/23
> 
> v1 here:
> https://lore.kernel.org/linux-fscrypt/20210727144349.11215-1-a.fatoum@pengutronix.de/T/#u
> 
> v1 -> v2:
>   - Drop encrypted key support and key_extract_material
>   - Use key_id instead of repurposing raw (Eric)
>   - Shift focus to trusted key sealing for non-TPM as a rationale
>     why this integration is worthwhile (Eric)
>   - Extend documentation with rationale on why one would
>     use trusted keys and warn about trusted key reuse
> 
> To: "Theodore Y. Ts'o" <tytso@mit.edu>
> To: Jaegeuk Kim <jaegeuk@kernel.org>
> To: Eric Biggers <ebiggers@kernel.org>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> Cc: David Howells <dhowells@redhat.com>
> Cc: linux-fscrypt@vger.kernel.org
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Cc: keyrings@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  Documentation/filesystems/fscrypt.rst | 31 ++++++++++++++-----
>  fs/crypto/keyring.c                   | 43 +++++++++++++++++++--------
>  2 files changed, 54 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
> index 44b67ebd6e40..c1811fa4285a 100644
> --- a/Documentation/filesystems/fscrypt.rst
> +++ b/Documentation/filesystems/fscrypt.rst
> @@ -734,23 +734,40 @@ as follows:
>  
>  - ``key_id`` is 0 if the raw key is given directly in the ``raw``
>    field.  Otherwise ``key_id`` is the ID of a Linux keyring key of
> -  type "fscrypt-provisioning" whose payload is
> +  type "fscrypt-provisioning" or "trusted":
> +  "fscrypt-provisioning" keys have a payload of
>    struct fscrypt_provisioning_key_payload whose ``raw`` field contains
>    the raw key and whose ``type`` field matches ``key_spec.type``.
>    Since ``raw`` is variable-length, the total size of this key's
>    payload must be ``sizeof(struct fscrypt_provisioning_key_payload)``
> -  plus the raw key size.  The process must have Search permission on
> -  this key.
> -
> -  Most users should leave this 0 and specify the raw key directly.
> -  The support for specifying a Linux keyring key is intended mainly to
> -  allow re-adding keys after a filesystem is unmounted and re-mounted,
> +  plus the raw key size.
> +  For "trusted" keys, the payload is directly taken as the raw key.
> +
> +  The process must have Search permission on this key.
> +
> +  Most users leave this 0 and specify the raw key directly.
> +  "trusted" keys are useful to leverage kernel support for sealing
> +  and unsealing key material. Sealed keys can be persisted to
> +  unencrypted storage and later be used to decrypt the file system
> +  without requiring userspace to have knowledge of the raw key
> +  material.
> +  "fscrypt-provisioning" key support is intended mainly to allow
> +  re-adding keys after a filesystem is unmounted and re-mounted,
>    without having to store the raw keys in userspace memory.
>  
>  - ``raw`` is a variable-length field which must contain the actual
>    key, ``raw_size`` bytes long.  Alternatively, if ``key_id`` is
>    nonzero, then this field is unused.
>  
> +.. note::
> +
> +   Users should take care not to reuse the fscrypt key material with
> +   different ciphers or in multiple contexts as this may make it
> +   easier to deduce the key.
> +   This also applies when the key material is supplied indirectly
> +   via a kernel trusted key. In this case, the trusted key should
> +   perferably be used only in a single context.
> +
>  For v2 policy keys, the kernel keeps track of which user (identified
>  by effective user ID) added the key, and only allows the key to be
>  removed by that user --- or by "root", if they use
> diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
> index 0b3ffbb4faf4..721f5da51416 100644
> --- a/fs/crypto/keyring.c
> +++ b/fs/crypto/keyring.c
> @@ -20,6 +20,7 @@
>  
>  #include <crypto/skcipher.h>
>  #include <linux/key-type.h>
> +#include <keys/trusted-type.h>
>  #include <linux/random.h>
>  #include <linux/seq_file.h>
>  
> @@ -577,28 +578,44 @@ static int get_keyring_key(u32 key_id, u32 type,
>  	key_ref_t ref;
>  	struct key *key;
>  	const struct fscrypt_provisioning_key_payload *payload;
> -	int err;
> +	int err = 0;
>  
>  	ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
>  	if (IS_ERR(ref))
>  		return PTR_ERR(ref);
>  	key = key_ref_to_ptr(ref);
>  
> -	if (key->type != &key_type_fscrypt_provisioning)
> -		goto bad_key;
> -	payload = key->payload.data[0];
> +	if (key->type == &key_type_fscrypt_provisioning) {

Why does fscrypt have own key type, and does not extend 'encrypted' with a
new format [*]?

> +		payload = key->payload.data[0];
>  
> -	/* Don't allow fscrypt v1 keys to be used as v2 keys and vice versa. */
> -	if (payload->type != type)
> -		goto bad_key;
> +		/* Don't allow fscrypt v1 keys to be used as v2 keys and vice versa. */
> +		if (payload->type != type) {
> +			err = -EKEYREJECTED;
> +			goto out_put;
> +		}
>  
> -	secret->size = key->datalen - sizeof(*payload);
> -	memcpy(secret->raw, payload->raw, secret->size);
> -	err = 0;
> -	goto out_put;
> +		secret->size = key->datalen - sizeof(*payload);
> +		memcpy(secret->raw, payload->raw, secret->size);
> +	} else if (IS_REACHABLE(CONFIG_TRUSTED_KEYS) && key->type == &key_type_trusted) {
> +		struct trusted_key_payload *tkp;
> +
> +		/* avoid reseal changing payload while we memcpy key */
> +		down_read(&key->sem);
> +		tkp = key->payload.data[0];
> +		if (!tkp || tkp->key_len < FSCRYPT_MIN_KEY_SIZE ||
> +		    tkp->key_len > FSCRYPT_MAX_KEY_SIZE) {
> +			up_read(&key->sem);
> +			err = -EINVAL;
> +			goto out_put;
> +		}
> +
> +		secret->size = tkp->key_len;
> +		memcpy(secret->raw, tkp->key, secret->size);
> +		up_read(&key->sem);
> +	} else {


I don't think this is right, or at least it does not follow the pattern
in [*]. I.e. you should rather use trusted key to seal your fscrypt key.


> +		err = -EKEYREJECTED;
> +	}
>  
> -bad_key:
> -	err = -EKEYREJECTED;
>  out_put:
>  	key_ref_put(ref);
>  	return err;
> -- 
> 2.30.2
> 
> 

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

/Jarkko

Re: [PATCH v2] fscrypt: support trusted keys

[Ahmad Fatoum]

Hello Jarkko,

On 09.08.21 11:44, Jarkko Sakkinen wrote:
> On Fri, Aug 06, 2021 at 05:09:28PM +0200, Ahmad Fatoum wrote:
>> Kernel trusted keys don't require userspace knowledge of the raw key
>> material and instead export a sealed blob, which can be persisted to
>> unencrypted storage. Userspace can then load this blob into the kernel,
>> where it's unsealed and from there on usable for kernel crypto.
>>
>> This is incompatible with fscrypt, where userspace is supposed to supply
>> the raw key material. For TPMs, a work around is to do key unsealing in
>> userspace, but this may not be feasible for other trusted key backends.
>>
>> Make it possible to benefit from both fscrypt and trusted key sealing
>> by extending fscrypt_add_key_arg::key_id to hold either the ID of a
>> fscrypt-provisioning or a trusted key.
>>
>> A non fscrypt-provisioning key_id was so far prohibited, so additionally
>> allowing trusted keys won't break backwards compatibility.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> Tested with:
>> https://github.com/google/fscryptctl/pull/23
>> -	if (key->type != &key_type_fscrypt_provisioning)
>> -		goto bad_key;
>> -	payload = key->payload.data[0];
>> +	if (key->type == &key_type_fscrypt_provisioning) {
> 
> Why does fscrypt have own key type, and does not extend 'encrypted' with a
> new format [*]?

See the commit[1] adding it for more information. TL;DR:

fscrypt maintainers would've preferred keys to be associated with
a "domain". So an encrypted key generated for fscrypt use couldn't be reused
for e.g. dm-crypt. They are wary of fscrypt users being more exposed if their
keys can be used with weaker ciphers via other kernel functionality that could
be used to extract information about the raw key material.

Eric also mentioned dislike of the possibility of rooting encrypted keys to
user keys. v2 is only restricted to v2, so we didn't discuss this further.

Restricting the key to fscrypt-only precludes this reuse.

My commit makes no attempts in changing that. It just adds a new way to pass
raw key material into fscrypt. For more information, see the commit[1] adding
that key type.

> [*] https://www.kernel.org/doc/html/v5.13/security/keys/trusted-encrypted.html

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=93edd392ca

Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] fscrypt: support trusted keys

[Ahmad Fatoum]

On 09.08.21 12:00, Ahmad Fatoum wrote:
> Hello Jarkko,
> 
> On 09.08.21 11:44, Jarkko Sakkinen wrote:
>> On Fri, Aug 06, 2021 at 05:09:28PM +0200, Ahmad Fatoum wrote:
>>> Kernel trusted keys don't require userspace knowledge of the raw key
>>> material and instead export a sealed blob, which can be persisted to
>>> unencrypted storage. Userspace can then load this blob into the kernel,
>>> where it's unsealed and from there on usable for kernel crypto.
>>>
>>> This is incompatible with fscrypt, where userspace is supposed to supply
>>> the raw key material. For TPMs, a work around is to do key unsealing in
>>> userspace, but this may not be feasible for other trusted key backends.
>>>
>>> Make it possible to benefit from both fscrypt and trusted key sealing
>>> by extending fscrypt_add_key_arg::key_id to hold either the ID of a
>>> fscrypt-provisioning or a trusted key.
>>>
>>> A non fscrypt-provisioning key_id was so far prohibited, so additionally
>>> allowing trusted keys won't break backwards compatibility.
>>>
>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>> ---
>>> Tested with:
>>> https://github.com/google/fscryptctl/pull/23
>>> -	if (key->type != &key_type_fscrypt_provisioning)
>>> -		goto bad_key;
>>> -	payload = key->payload.data[0];
>>> +	if (key->type == &key_type_fscrypt_provisioning) {
>>
>> Why does fscrypt have own key type, and does not extend 'encrypted' with a
>> new format [*]?
> 
> See the commit[1] adding it for more information. TL;DR:
> 
> fscrypt maintainers would've preferred keys to be associated with
> a "domain". So an encrypted key generated for fscrypt use couldn't be reused
> for e.g. dm-crypt. They are wary of fscrypt users being more exposed if their
> keys can be used with weaker ciphers via other kernel functionality that could
> be used to extract information about the raw key material.
> 
> Eric also mentioned dislike of the possibility of rooting encrypted keys to
> user keys. v2 is only restricted to v2, so we didn't discuss this further.

Typo: v2 (of my series) is only restricted to s/v2/trusted keys/

> 
> Restricting the key to fscrypt-only precludes this reuse.
> 
> My commit makes no attempts in changing that. It just adds a new way to pass
> raw key material into fscrypt. For more information, see the commit[1] adding
> that key type.
> 
>> [*] https://www.kernel.org/doc/html/v5.13/security/keys/trusted-encrypted.html
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=93edd392ca
> 
> Cheers,
> Ahmad
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] fscrypt: support trusted keys

[Eric Biggers]

On Mon, Aug 09, 2021 at 12:44:08PM +0300, Jarkko Sakkinen wrote:
> > @@ -577,28 +578,44 @@ static int get_keyring_key(u32 key_id, u32 type,
> >  	key_ref_t ref;
> >  	struct key *key;
> >  	const struct fscrypt_provisioning_key_payload *payload;
> > -	int err;
> > +	int err = 0;
> >  
> >  	ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
> >  	if (IS_ERR(ref))
> >  		return PTR_ERR(ref);
> >  	key = key_ref_to_ptr(ref);
> >  
> > -	if (key->type != &key_type_fscrypt_provisioning)
> > -		goto bad_key;
> > -	payload = key->payload.data[0];
> > +	if (key->type == &key_type_fscrypt_provisioning) {
> 
> Why does fscrypt have own key type, and does not extend 'encrypted' with a
> new format [*]?

Are you referring to the "fscrypt-provisioning" key type?  That is an existing
feature (which in most cases isn't used, but there is a use case that requires
it), not something being added by this patch.  We just needed a key type where
userspace can add a raw key to the kernel and not be able to read it back (so
like the "logon" key type), but also have the kernel enforce that that key is
only used for fscrypt with a particular KDF version, and not with other random
kernel features.  The "encrypted" key type wouldn't have worked for this at all;
it's a totally different thing.

> > +	} else if (IS_REACHABLE(CONFIG_TRUSTED_KEYS) && key->type == &key_type_trusted) {
> > +		struct trusted_key_payload *tkp;
> > +
> > +		/* avoid reseal changing payload while we memcpy key */
> > +		down_read(&key->sem);
> > +		tkp = key->payload.data[0];
> > +		if (!tkp || tkp->key_len < FSCRYPT_MIN_KEY_SIZE ||
> > +		    tkp->key_len > FSCRYPT_MAX_KEY_SIZE) {
> > +			up_read(&key->sem);
> > +			err = -EINVAL;
> > +			goto out_put;
> > +		}
> > +
> > +		secret->size = tkp->key_len;
> > +		memcpy(secret->raw, tkp->key, secret->size);
> > +		up_read(&key->sem);
> > +	} else {
> 
> 
> I don't think this is right, or at least it does not follow the pattern
> in [*]. I.e. you should rather use trusted key to seal your fscrypt key.

What's the benefit of the extra layer of indirection over just using a "trusted"
key directly?  The use case for "encrypted" keys is not at all clear to me.

- Eric

Re: [PATCH v2] fscrypt: support trusted keys

[Eric Biggers]

Hi Ahmad,

This generally looks okay, but I have some comments below.

On Fri, Aug 06, 2021 at 05:09:28PM +0200, Ahmad Fatoum wrote:
> Kernel trusted keys don't require userspace knowledge of the raw key
> material and instead export a sealed blob, which can be persisted to
> unencrypted storage. Userspace can then load this blob into the kernel,
> where it's unsealed and from there on usable for kernel crypto.

Please be explicit about where and how the keys get generated in this case.

> This is incompatible with fscrypt, where userspace is supposed to supply
> the raw key material. For TPMs, a work around is to do key unsealing in
> userspace, but this may not be feasible for other trusted key backends.

As far as I can see, "Key unsealing in userspace" actually is the preferred way
to implement TPM-bound encryption.  So it doesn't seem fair to call it a "work
around".

> +  Most users leave this 0 and specify the raw key directly.
> +  "trusted" keys are useful to leverage kernel support for sealing
> +  and unsealing key material. Sealed keys can be persisted to
> +  unencrypted storage and later be used to decrypt the file system
> +  without requiring userspace to have knowledge of the raw key
> +  material.
> +  "fscrypt-provisioning" key support is intended mainly to allow
> +  re-adding keys after a filesystem is unmounted and re-mounted,
>    without having to store the raw keys in userspace memory.
>  
>  - ``raw`` is a variable-length field which must contain the actual
>    key, ``raw_size`` bytes long.  Alternatively, if ``key_id`` is
>    nonzero, then this field is unused.
>  
> +.. note::
> +
> +   Users should take care not to reuse the fscrypt key material with
> +   different ciphers or in multiple contexts as this may make it
> +   easier to deduce the key.
> +   This also applies when the key material is supplied indirectly
> +   via a kernel trusted key. In this case, the trusted key should
> +   perferably be used only in a single context.

Again, please be explicit about key generation.  Note that key generation is
already discussed in a different section, "Master Keys".  There should be a
mention of trusted keys there.  The above note about not reusing keys probably
belongs there too.  (The section you're editing here is
"FS_IOC_ADD_ENCRYPTION_KEY", which is primarily intended to just document the
ioctl, so it's not necessarily the best place for this type of information.)

> @@ -577,28 +578,44 @@ static int get_keyring_key(u32 key_id, u32 type,
>  	key_ref_t ref;
>  	struct key *key;
>  	const struct fscrypt_provisioning_key_payload *payload;
> -	int err;
> +	int err = 0;
>  
>  	ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
>  	if (IS_ERR(ref))
>  		return PTR_ERR(ref);
>  	key = key_ref_to_ptr(ref);
>  
> -	if (key->type != &key_type_fscrypt_provisioning)
> -		goto bad_key;
> -	payload = key->payload.data[0];
> +	if (key->type == &key_type_fscrypt_provisioning) {

This function is getting long; it probably should be broken this up into several
functions.  E.g.:

static int get_keyring_key(u32 key_id, u32 type,
                           struct fscrypt_master_key_secret *secret)
{
        key_ref_t ref;
        struct key *key;
        int err;

        ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
        if (IS_ERR(ref))
                return PTR_ERR(ref);
        key = key_ref_to_ptr(ref);

        if (key->type == &key_type_fscrypt_provisioning) {
                err = fscrypt_get_provisioning_key(key, type, secret);
        } else if (IS_REACHABLE(CONFIG_TRUSTED_KEYS) &&
                   key->type == &key_type_trusted) {
                err = fscrypt_get_trusted_key(key, secret);
        } else {
                err = -EKEYREJECTED;
        }
        key_ref_put(ref);
        return err;
}

> +		/* Don't allow fscrypt v1 keys to be used as v2 keys and vice versa. */

Please avoid overly-long lines.

> +		tkp = key->payload.data[0];
> +		if (!tkp || tkp->key_len < FSCRYPT_MIN_KEY_SIZE ||
> +		    tkp->key_len > FSCRYPT_MAX_KEY_SIZE) {
> +			up_read(&key->sem);
> +			err = -EINVAL;
> +			goto out_put;
> +		}

What does the !tkp case mean?  For "user" and "logon" keys it means "key
revoked", but the "trusted" key type doesn't implement revoke.  Is this included
just to be safe?  That might be reasonable, but perhaps the error code in that
case (but not the invalid length cases) should be -EKEYREVOKED instead?

- Eric

Re: [PATCH v2] fscrypt: support trusted keys

[Ahmad Fatoum]

Hello Eric,

On 09.08.21 23:24, Eric Biggers wrote:
> Hi Ahmad,
> 
> This generally looks okay, but I have some comments below.
> 
> On Fri, Aug 06, 2021 at 05:09:28PM +0200, Ahmad Fatoum wrote:
>> Kernel trusted keys don't require userspace knowledge of the raw key
>> material and instead export a sealed blob, which can be persisted to
>> unencrypted storage. Userspace can then load this blob into the kernel,
>> where it's unsealed and from there on usable for kernel crypto.
> 
> Please be explicit about where and how the keys get generated in this case.

I intentionally avoided talking about this. You see, the trusted key documentation[1]
phrases it as "all keys are created in the kernel", but you consider
"'The key material is generated
 within the kernel' [a] misleading claim'. [2]

Also, I hope patches to force kernel RNG and CAAM support (using kernel RNG as
default) will soon be accepted, which would invalidate any further claims in the
commit message without a means to correct them.

I thus restricted my commit message to the necessary bit that are needed to
understand the patch, which is: userspace knowledge of the key material is
not required. If you disagree, could you provide me the text you'd prefer?

>> This is incompatible with fscrypt, where userspace is supposed to supply
>> the raw key material. For TPMs, a work around is to do key unsealing in
>> userspace, but this may not be feasible for other trusted key backends.
> 
> As far as I can see, "Key unsealing in userspace" actually is the preferred way
> to implement TPM-bound encryption.  So it doesn't seem fair to call it a "work
> around".

In the context of *kernel trusted keys*, direct interaction with the TPM
outside the kernel to decrypt a kernel-encrypted blob is surely not the
preferred way.

For TPM-bound encryption completely in userspace? Maybe. But that's not
what this patch is about. It's about kernel trusted keys and offloading
part of its functionality to userspace to _work around_ lack of kernel-side
integration is exactly that: a _work around_.

>> +  Most users leave this 0 and specify the raw key directly.
>> +  "trusted" keys are useful to leverage kernel support for sealing
>> +  and unsealing key material. Sealed keys can be persisted to
>> +  unencrypted storage and later be used to decrypt the file system
>> +  without requiring userspace to have knowledge of the raw key
>> +  material.
>> +  "fscrypt-provisioning" key support is intended mainly to allow
>> +  re-adding keys after a filesystem is unmounted and re-mounted,
>>    without having to store the raw keys in userspace memory.
>>  
>>  - ``raw`` is a variable-length field which must contain the actual
>>    key, ``raw_size`` bytes long.  Alternatively, if ``key_id`` is
>>    nonzero, then this field is unused.
>>  
>> +.. note::
>> +
>> +   Users should take care not to reuse the fscrypt key material with
>> +   different ciphers or in multiple contexts as this may make it
>> +   easier to deduce the key.
>> +   This also applies when the key material is supplied indirectly
>> +   via a kernel trusted key. In this case, the trusted key should
>> +   perferably be used only in a single context.
> 
> Again, please be explicit about key generation.  Note that key generation is
> already discussed in a different section, "Master Keys".  There should be a
> mention of trusted keys there.  The above note about not reusing keys probably
> belongs there too.  (The section you're editing here is
> "FS_IOC_ADD_ENCRYPTION_KEY", which is primarily intended to just document the
> ioctl, so it's not necessarily the best place for this type of information.)

Yes. The content of the note is more appropriate there.

>> @@ -577,28 +578,44 @@ static int get_keyring_key(u32 key_id, u32 type,
>>  	key_ref_t ref;
>>  	struct key *key;
>>  	const struct fscrypt_provisioning_key_payload *payload;
>> -	int err;
>> +	int err = 0;
>>  
>>  	ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
>>  	if (IS_ERR(ref))
>>  		return PTR_ERR(ref);
>>  	key = key_ref_to_ptr(ref);
>>  
>> -	if (key->type != &key_type_fscrypt_provisioning)
>> -		goto bad_key;
>> -	payload = key->payload.data[0];
>> +	if (key->type == &key_type_fscrypt_provisioning) {
> 
> This function is getting long; it probably should be broken this up into several
> functions.  E.g.:

Will do for v3.

> static int get_keyring_key(u32 key_id, u32 type,
>                            struct fscrypt_master_key_secret *secret)
> {
>         key_ref_t ref;
>         struct key *key;
>         int err;
> 
>         ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
>         if (IS_ERR(ref))
>                 return PTR_ERR(ref);
>         key = key_ref_to_ptr(ref);
> 
>         if (key->type == &key_type_fscrypt_provisioning) {
>                 err = fscrypt_get_provisioning_key(key, type, secret);
>         } else if (IS_REACHABLE(CONFIG_TRUSTED_KEYS) &&
>                    key->type == &key_type_trusted) {
>                 err = fscrypt_get_trusted_key(key, secret);
>         } else {
>                 err = -EKEYREJECTED;
>         }
>         key_ref_put(ref);
>         return err;
> }
> 
>> +		/* Don't allow fscrypt v1 keys to be used as v2 keys and vice versa. */
> 
> Please avoid overly-long lines.

For v3 with helper functions, there will be one indentation level less,
so this will be less 79 again instead of 87.

> 
>> +		tkp = key->payload.data[0];
>> +		if (!tkp || tkp->key_len < FSCRYPT_MIN_KEY_SIZE ||
>> +		    tkp->key_len > FSCRYPT_MAX_KEY_SIZE) {
>> +			up_read(&key->sem);
>> +			err = -EINVAL;
>> +			goto out_put;
>> +		}
> 
> What does the !tkp case mean?  For "user" and "logon" keys it means "key
> revoked", but the "trusted" key type doesn't implement revoke.  Is this included
> just to be safe?

Oh, good point. I think I cargo-culted it off encrypted key support for
eCryptfs and dm-crypt. Encrypted keys don't support revoke either..

That might be reasonable, but perhaps the error code in that
> case (but not the invalid length cases) should be -EKEYREVOKED instead?

Yes. It was like this for v1, but I missed it when dropping the
dependency on the key_extract_material patch. Will fix for v3.

[1]: https://www.kernel.org/doc/html/v5.14-rc5/security/keys/trusted-encrypted.html
[2]: https://lore.kernel.org/linux-fscrypt/YQLzOwnPF1434kUk@gmail.com/


Cheers and thanks for the review,
Ahmad


> 
> - Eric
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] fscrypt: support trusted keys

[Eric Biggers]

On Tue, Aug 10, 2021 at 09:41:20AM +0200, Ahmad Fatoum wrote:
> Hello Eric,
> 
> On 09.08.21 23:24, Eric Biggers wrote:
> > Hi Ahmad,
> > 
> > This generally looks okay, but I have some comments below.
> > 
> > On Fri, Aug 06, 2021 at 05:09:28PM +0200, Ahmad Fatoum wrote:
> >> Kernel trusted keys don't require userspace knowledge of the raw key
> >> material and instead export a sealed blob, which can be persisted to
> >> unencrypted storage. Userspace can then load this blob into the kernel,
> >> where it's unsealed and from there on usable for kernel crypto.
> > 
> > Please be explicit about where and how the keys get generated in this case.
> 
> I intentionally avoided talking about this. You see, the trusted key documentation[1]
> phrases it as "all keys are created in the kernel", but you consider
> "'The key material is generated
>  within the kernel' [a] misleading claim'. [2]
> 
> Also, I hope patches to force kernel RNG and CAAM support (using kernel RNG as
> default) will soon be accepted, which would invalidate any further claims in the
> commit message without a means to correct them.
> 
> I thus restricted my commit message to the necessary bit that are needed to
> understand the patch, which is: userspace knowledge of the key material is
> not required. If you disagree, could you provide me the text you'd prefer?

Just write that the trusted key subsystem is responsible for generating the
keys.  And please fix the trusted keys documentation to properly document key
generation, or better yet just fix the trusted keys subsystem to generate the
keys properly.

> >> This is incompatible with fscrypt, where userspace is supposed to supply
> >> the raw key material. For TPMs, a work around is to do key unsealing in
> >> userspace, but this may not be feasible for other trusted key backends.
> > 
> > As far as I can see, "Key unsealing in userspace" actually is the preferred way
> > to implement TPM-bound encryption.  So it doesn't seem fair to call it a "work
> > around".
> 
> In the context of *kernel trusted keys*, direct interaction with the TPM
> outside the kernel to decrypt a kernel-encrypted blob is surely not the
> preferred way.
> 
> For TPM-bound encryption completely in userspace? Maybe. But that's not
> what this patch is about. It's about kernel trusted keys and offloading
> part of its functionality to userspace to _work around_ lack of kernel-side
> integration is exactly that: a _work around_.

As I said before, there's no need for kernel trusted keys at all in cases where
the TPM userspace tools can be used.  This is existing, well-documented process,
e.g. see: https://wiki.archlinux.org/title/Trusted_Platform_Module.  You are
starting with a solution ("I'm going to use kernel trusted keys") and not a
problem ("I want my fscrypt key(s) to be TPM-bound").  So please fix this patch
to explain the situation(s) in which it actually solves a problem that isn't
already solved.

- Eric

Re: [PATCH v2] fscrypt: support trusted keys

[Jarkko Sakkinen]

On Mon, Aug 09, 2021 at 12:00:40PM +0200, Ahmad Fatoum wrote:
> Hello Jarkko,
> 
> On 09.08.21 11:44, Jarkko Sakkinen wrote:
> > On Fri, Aug 06, 2021 at 05:09:28PM +0200, Ahmad Fatoum wrote:
> >> Kernel trusted keys don't require userspace knowledge of the raw key
> >> material and instead export a sealed blob, which can be persisted to
> >> unencrypted storage. Userspace can then load this blob into the kernel,
> >> where it's unsealed and from there on usable for kernel crypto.
> >>
> >> This is incompatible with fscrypt, where userspace is supposed to supply
> >> the raw key material. For TPMs, a work around is to do key unsealing in
> >> userspace, but this may not be feasible for other trusted key backends.
> >>
> >> Make it possible to benefit from both fscrypt and trusted key sealing
> >> by extending fscrypt_add_key_arg::key_id to hold either the ID of a
> >> fscrypt-provisioning or a trusted key.
> >>
> >> A non fscrypt-provisioning key_id was so far prohibited, so additionally
> >> allowing trusted keys won't break backwards compatibility.
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >> Tested with:
> >> https://github.com/google/fscryptctl/pull/23
> >> -	if (key->type != &key_type_fscrypt_provisioning)
> >> -		goto bad_key;
> >> -	payload = key->payload.data[0];
> >> +	if (key->type == &key_type_fscrypt_provisioning) {
> > 
> > Why does fscrypt have own key type, and does not extend 'encrypted' with a
> > new format [*]?
> 
> See the commit[1] adding it for more information. TL;DR:
> 
> fscrypt maintainers would've preferred keys to be associated with
> a "domain". So an encrypted key generated for fscrypt use couldn't be reused
> for e.g. dm-crypt. They are wary of fscrypt users being more exposed if their
> keys can be used with weaker ciphers via other kernel functionality that could
> be used to extract information about the raw key material.
> 
> Eric also mentioned dislike of the possibility of rooting encrypted keys to
> user keys. v2 is only restricted to v2, so we didn't discuss this further.
> 
> Restricting the key to fscrypt-only precludes this reuse.
> 
> My commit makes no attempts in changing that. It just adds a new way to pass
> raw key material into fscrypt. For more information, see the commit[1] adding
> that key type.
> 
> > [*] https://www.kernel.org/doc/html/v5.13/security/keys/trusted-encrypted.html
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=93edd392ca

OK, so why does the trusted key does not seal a fscrypt key, but instead
its key material is directly used?

> Cheers,
> Ahmad

/Jarkko

Re: [PATCH v2] fscrypt: support trusted keys

[Jarkko Sakkinen]

On Mon, Aug 09, 2021 at 01:52:01PM -0700, Eric Biggers wrote:
> On Mon, Aug 09, 2021 at 12:44:08PM +0300, Jarkko Sakkinen wrote:
> > > @@ -577,28 +578,44 @@ static int get_keyring_key(u32 key_id, u32 type,
> > >  	key_ref_t ref;
> > >  	struct key *key;
> > >  	const struct fscrypt_provisioning_key_payload *payload;
> > > -	int err;
> > > +	int err = 0;
> > >  
> > >  	ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
> > >  	if (IS_ERR(ref))
> > >  		return PTR_ERR(ref);
> > >  	key = key_ref_to_ptr(ref);
> > >  
> > > -	if (key->type != &key_type_fscrypt_provisioning)
> > > -		goto bad_key;
> > > -	payload = key->payload.data[0];
> > > +	if (key->type == &key_type_fscrypt_provisioning) {
> > 
> > Why does fscrypt have own key type, and does not extend 'encrypted' with a
> > new format [*]?
> 
> Are you referring to the "fscrypt-provisioning" key type?  That is an existing
> feature (which in most cases isn't used, but there is a use case that requires
> it), not something being added by this patch.  We just needed a key type where
> userspace can add a raw key to the kernel and not be able to read it back (so
> like the "logon" key type), but also have the kernel enforce that that key is
> only used for fscrypt with a particular KDF version, and not with other random
> kernel features.  The "encrypted" key type wouldn't have worked for this at all;
> it's a totally different thing.
> 
> > > +	} else if (IS_REACHABLE(CONFIG_TRUSTED_KEYS) && key->type == &key_type_trusted) {
> > > +		struct trusted_key_payload *tkp;
> > > +
> > > +		/* avoid reseal changing payload while we memcpy key */
> > > +		down_read(&key->sem);
> > > +		tkp = key->payload.data[0];
> > > +		if (!tkp || tkp->key_len < FSCRYPT_MIN_KEY_SIZE ||
> > > +		    tkp->key_len > FSCRYPT_MAX_KEY_SIZE) {
> > > +			up_read(&key->sem);
> > > +			err = -EINVAL;
> > > +			goto out_put;
> > > +		}
> > > +
> > > +		secret->size = tkp->key_len;
> > > +		memcpy(secret->raw, tkp->key, secret->size);
> > > +		up_read(&key->sem);
> > > +	} else {
> > 
> > 
> > I don't think this is right, or at least it does not follow the pattern
> > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> 
> What's the benefit of the extra layer of indirection over just using a "trusted"
> key directly?  The use case for "encrypted" keys is not at all clear to me.

Because it is more robust to be able to use small amount of trusted keys,
which are not entirely software based.

And since it's also a pattern on existing kernel features utilizing trusted
keys, the pressure here to explain why break the pattern, should be on the
side of the one who breaks it.

/Jarkko

Re: [PATCH v2] fscrypt: support trusted keys

[Eric Biggers]

On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > > 
> > > I don't think this is right, or at least it does not follow the pattern
> > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> > 
> > What's the benefit of the extra layer of indirection over just using a "trusted"
> > key directly?  The use case for "encrypted" keys is not at all clear to me.
> 
> Because it is more robust to be able to use small amount of trusted keys,
> which are not entirely software based.
> 
> And since it's also a pattern on existing kernel features utilizing trusted
> keys, the pressure here to explain why break the pattern, should be on the
> side of the one who breaks it.

This is a new feature, so it's on the person proposing the feature to explain
why it's useful.  The purpose of "encrypted" keys is not at all clear, and the
documentation for them is heavily misleading.  E.g.:

    "user space sees, stores, and loads only encrypted blobs"
    (Not necessarily true, as I've explained previously.)

    "Encrypted keys do not depend on a trust source" ...  "The main disadvantage
    of encrypted keys is that if they are not rooted in a trusted key"
    (Not necessarily true, and in fact it seems they're only useful when they
    *do* depend on a trust source.  At least that's the use case that is being
    proposed here, IIUC.)

I do see a possible use for the layer of indirection that "encrypted" keys are,
which is that it would reduce the overhead of having lots of keys be directly
encrypted by the TPM/TEE/CAAM.  Is this the use case?  If so, it needs to be
explained.

- Eric

Re: [PATCH v2] fscrypt: support trusted keys

[Jarkko Sakkinen]

On Tue, Aug 10, 2021 at 11:46:49AM -0700, Eric Biggers wrote:
> On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > > > 
> > > > I don't think this is right, or at least it does not follow the pattern
> > > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> > > 
> > > What's the benefit of the extra layer of indirection over just using a "trusted"
> > > key directly?  The use case for "encrypted" keys is not at all clear to me.
> > 
> > Because it is more robust to be able to use small amount of trusted keys,
> > which are not entirely software based.
> > 
> > And since it's also a pattern on existing kernel features utilizing trusted
> > keys, the pressure here to explain why break the pattern, should be on the
> > side of the one who breaks it.
> 
> This is a new feature, so it's on the person proposing the feature to explain
> why it's useful.  The purpose of "encrypted" keys is not at all clear, and the
> documentation for them is heavily misleading.  E.g.:
> 
>     "user space sees, stores, and loads only encrypted blobs"
>     (Not necessarily true, as I've explained previously.)
> 
>     "Encrypted keys do not depend on a trust source" ...  "The main disadvantage
>     of encrypted keys is that if they are not rooted in a trusted key"
>     (Not necessarily true, and in fact it seems they're only useful when they
>     *do* depend on a trust source.  At least that's the use case that is being
>     proposed here, IIUC.)
> 
> I do see a possible use for the layer of indirection that "encrypted" keys are,
> which is that it would reduce the overhead of having lots of keys be directly
> encrypted by the TPM/TEE/CAAM.  Is this the use case?  If so, it needs to be
> explained.

If trusted keys are used directly, it's an introduction of a bottleneck.
If they are used indirectly, you can still choose to have one trusted
key per fscrypt key.

Thus, it's obviously a bad idea to use them directly.

/Jarkko

Re: [PATCH v2] fscrypt: support trusted keys

[Eric Biggers]

On Wed, Aug 11, 2021 at 12:21:40AM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 10, 2021 at 11:46:49AM -0700, Eric Biggers wrote:
> > On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > > > > 
> > > > > I don't think this is right, or at least it does not follow the pattern
> > > > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> > > > 
> > > > What's the benefit of the extra layer of indirection over just using a "trusted"
> > > > key directly?  The use case for "encrypted" keys is not at all clear to me.
> > > 
> > > Because it is more robust to be able to use small amount of trusted keys,
> > > which are not entirely software based.
> > > 
> > > And since it's also a pattern on existing kernel features utilizing trusted
> > > keys, the pressure here to explain why break the pattern, should be on the
> > > side of the one who breaks it.
> > 
> > This is a new feature, so it's on the person proposing the feature to explain
> > why it's useful.  The purpose of "encrypted" keys is not at all clear, and the
> > documentation for them is heavily misleading.  E.g.:
> > 
> >     "user space sees, stores, and loads only encrypted blobs"
> >     (Not necessarily true, as I've explained previously.)
> > 
> >     "Encrypted keys do not depend on a trust source" ...  "The main disadvantage
> >     of encrypted keys is that if they are not rooted in a trusted key"
> >     (Not necessarily true, and in fact it seems they're only useful when they
> >     *do* depend on a trust source.  At least that's the use case that is being
> >     proposed here, IIUC.)
> > 
> > I do see a possible use for the layer of indirection that "encrypted" keys are,
> > which is that it would reduce the overhead of having lots of keys be directly
> > encrypted by the TPM/TEE/CAAM.  Is this the use case?  If so, it needs to be
> > explained.
> 
> If trusted keys are used directly, it's an introduction of a bottleneck.
> If they are used indirectly, you can still choose to have one trusted
> key per fscrypt key.
> 
> Thus, it's obviously a bad idea to use them directly.
> 

So actually explain that in the documentation.  It's not obvious at all.  And
does this imply that the support for trusted keys in dm-crypt is a mistake?

- Eric

Re: [PATCH v2] fscrypt: support trusted keys

[Jarkko Sakkinen]

On Tue, Aug 10, 2021 at 02:27:24PM -0700, Eric Biggers wrote:
> On Wed, Aug 11, 2021 at 12:21:40AM +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 10, 2021 at 11:46:49AM -0700, Eric Biggers wrote:
> > > On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > > > > > 
> > > > > > I don't think this is right, or at least it does not follow the pattern
> > > > > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> > > > > 
> > > > > What's the benefit of the extra layer of indirection over just using a "trusted"
> > > > > key directly?  The use case for "encrypted" keys is not at all clear to me.
> > > > 
> > > > Because it is more robust to be able to use small amount of trusted keys,
> > > > which are not entirely software based.
> > > > 
> > > > And since it's also a pattern on existing kernel features utilizing trusted
> > > > keys, the pressure here to explain why break the pattern, should be on the
> > > > side of the one who breaks it.
> > > 
> > > This is a new feature, so it's on the person proposing the feature to explain
> > > why it's useful.  The purpose of "encrypted" keys is not at all clear, and the
> > > documentation for them is heavily misleading.  E.g.:
> > > 
> > >     "user space sees, stores, and loads only encrypted blobs"
> > >     (Not necessarily true, as I've explained previously.)
> > > 
> > >     "Encrypted keys do not depend on a trust source" ...  "The main disadvantage
> > >     of encrypted keys is that if they are not rooted in a trusted key"
> > >     (Not necessarily true, and in fact it seems they're only useful when they
> > >     *do* depend on a trust source.  At least that's the use case that is being
> > >     proposed here, IIUC.)
> > > 
> > > I do see a possible use for the layer of indirection that "encrypted" keys are,
> > > which is that it would reduce the overhead of having lots of keys be directly
> > > encrypted by the TPM/TEE/CAAM.  Is this the use case?  If so, it needs to be
> > > explained.
> > 
> > If trusted keys are used directly, it's an introduction of a bottleneck.
> > If they are used indirectly, you can still choose to have one trusted
> > key per fscrypt key.
> > 
> > Thus, it's obviously a bad idea to use them directly.
> > 
> 
> So actually explain that in the documentation.  It's not obvious at all.  And
> does this imply that the support for trusted keys in dm-crypt is a mistake?

Looking at dm-crypt implementation, you can choose to use 'encrypted' key
type, which you can seal with a trusted key.

Note: I have not been involved when the feature was added to dm-crypt.

/Jarkko

Re: [PATCH v2] fscrypt: support trusted keys

[Mimi Zohar]

On Wed, 2021-08-11 at 03:17 +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 10, 2021 at 02:27:24PM -0700, Eric Biggers wrote:
> > On Wed, Aug 11, 2021 at 12:21:40AM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Aug 10, 2021 at 11:46:49AM -0700, Eric Biggers wrote:
> > > > On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > > > > > > 
> > > > > > > I don't think this is right, or at least it does not follow the pattern
> > > > > > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> > > > > > 
> > > > > > What's the benefit of the extra layer of indirection over just using a "trusted"
> > > > > > key directly?  The use case for "encrypted" keys is not at all clear to me.
> > > > > 
> > > > > Because it is more robust to be able to use small amount of trusted keys,
> > > > > which are not entirely software based.
> > > > > 
> > > > > And since it's also a pattern on existing kernel features utilizing trusted
> > > > > keys, the pressure here to explain why break the pattern, should be on the
> > > > > side of the one who breaks it.
> > > > 
> > > > This is a new feature, so it's on the person proposing the feature to explain
> > > > why it's useful.  The purpose of "encrypted" keys is not at all clear, and the
> > > > documentation for them is heavily misleading.  E.g.:
> > > > 
> > > >     "user space sees, stores, and loads only encrypted blobs"
> > > >     (Not necessarily true, as I've explained previously.)
> > > > 
> > > >     "Encrypted keys do not depend on a trust source" ...  "The main disadvantage
> > > >     of encrypted keys is that if they are not rooted in a trusted key"
> > > >     (Not necessarily true, and in fact it seems they're only useful when they
> > > >     *do* depend on a trust source.  At least that's the use case that is being
> > > >     proposed here, IIUC.)
> > > > 
> > > > I do see a possible use for the layer of indirection that "encrypted" keys are,
> > > > which is that it would reduce the overhead of having lots of keys be directly
> > > > encrypted by the TPM/TEE/CAAM.  Is this the use case?  If so, it needs to be
> > > > explained.
> > > 
> > > If trusted keys are used directly, it's an introduction of a bottleneck.
> > > If they are used indirectly, you can still choose to have one trusted
> > > key per fscrypt key.
> > > 
> > > Thus, it's obviously a bad idea to use them directly.
> > 
> > So actually explain that in the documentation.  It's not obvious at all.  And
> > does this imply that the support for trusted keys in dm-crypt is a mistake?
> 
> Looking at dm-crypt implementation, you can choose to use 'encrypted' key
> type, which you can seal with a trusted key.
> 
> Note: I have not been involved when the feature was added to dm-crypt.

At least for TPM 1.2,  "trusted" keys may be sealed to a PCR and then
extended to prevent subsequent usage.  For example, in the initramfs
all of the "encrypted" keys could be decrypted by a single "trusted"
key, before extending the PCR.

Mimi

Re: [PATCH v2] fscrypt: support trusted keys

[Eric Biggers]

On Wed, Aug 11, 2021 at 07:34:18AM -0400, Mimi Zohar wrote:
> On Wed, 2021-08-11 at 03:17 +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 10, 2021 at 02:27:24PM -0700, Eric Biggers wrote:
> > > On Wed, Aug 11, 2021 at 12:21:40AM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 10, 2021 at 11:46:49AM -0700, Eric Biggers wrote:
> > > > > On Tue, Aug 10, 2021 at 09:06:36PM +0300, Jarkko Sakkinen wrote:
> > > > > > > > 
> > > > > > > > I don't think this is right, or at least it does not follow the pattern
> > > > > > > > in [*]. I.e. you should rather use trusted key to seal your fscrypt key.
> > > > > > > 
> > > > > > > What's the benefit of the extra layer of indirection over just using a "trusted"
> > > > > > > key directly?  The use case for "encrypted" keys is not at all clear to me.
> > > > > > 
> > > > > > Because it is more robust to be able to use small amount of trusted keys,
> > > > > > which are not entirely software based.
> > > > > > 
> > > > > > And since it's also a pattern on existing kernel features utilizing trusted
> > > > > > keys, the pressure here to explain why break the pattern, should be on the
> > > > > > side of the one who breaks it.
> > > > > 
> > > > > This is a new feature, so it's on the person proposing the feature to explain
> > > > > why it's useful.  The purpose of "encrypted" keys is not at all clear, and the
> > > > > documentation for them is heavily misleading.  E.g.:
> > > > > 
> > > > >     "user space sees, stores, and loads only encrypted blobs"
> > > > >     (Not necessarily true, as I've explained previously.)
> > > > > 
> > > > >     "Encrypted keys do not depend on a trust source" ...  "The main disadvantage
> > > > >     of encrypted keys is that if they are not rooted in a trusted key"
> > > > >     (Not necessarily true, and in fact it seems they're only useful when they
> > > > >     *do* depend on a trust source.  At least that's the use case that is being
> > > > >     proposed here, IIUC.)
> > > > > 
> > > > > I do see a possible use for the layer of indirection that "encrypted" keys are,
> > > > > which is that it would reduce the overhead of having lots of keys be directly
> > > > > encrypted by the TPM/TEE/CAAM.  Is this the use case?  If so, it needs to be
> > > > > explained.
> > > > 
> > > > If trusted keys are used directly, it's an introduction of a bottleneck.
> > > > If they are used indirectly, you can still choose to have one trusted
> > > > key per fscrypt key.
> > > > 
> > > > Thus, it's obviously a bad idea to use them directly.
> > > 
> > > So actually explain that in the documentation.  It's not obvious at all.  And
> > > does this imply that the support for trusted keys in dm-crypt is a mistake?
> > 
> > Looking at dm-crypt implementation, you can choose to use 'encrypted' key
> > type, which you can seal with a trusted key.
> > 
> > Note: I have not been involved when the feature was added to dm-crypt.
> 
> At least for TPM 1.2,  "trusted" keys may be sealed to a PCR and then
> extended to prevent subsequent usage.  For example, in the initramfs
> all of the "encrypted" keys could be decrypted by a single "trusted"
> key, before extending the PCR.
> 
> Mimi
> 

Neither of you actually answered my question, which is whether the support for
trusted keys in dm-crypt is a mistake.  I think you're saying that it is?  That
would imply that fscrypt shouldn't support trusted keys, but rather encrypted
keys -- which conflicts with Ahmad's patch which is adding support for trusted
keys.  Note that your reasoning for this is not documented at all in the
trusted-encrypted keys documentation; it needs to be (email threads don't really
matter), otherwise how would anyone know when/how to use this feature?

- Eric

Re: [PATCH v2] fscrypt: support trusted keys

[Mimi Zohar]

On Wed, 2021-08-11 at 10:16 -0700, Eric Biggers wrote:

> Neither of you actually answered my question, which is whether the support for
> trusted keys in dm-crypt is a mistake.  I think you're saying that it is?  That
> would imply that fscrypt shouldn't support trusted keys, but rather encrypted
> keys -- which conflicts with Ahmad's patch which is adding support for trusted
> keys.  Note that your reasoning for this is not documented at all in the
> trusted-encrypted keys documentation; it needs to be (email threads don't really
> matter), otherwise how would anyone know when/how to use this feature?

True, but all of the trusted-encrypted key examples in the
documentation are "encrypted" type keys, encrypted/decrypted based on a
"trusted" type key.  There are no examples of using the "trusted" key
type directly.  Before claiming that adding "trusted" key support in
dm-crypt was a mistake, we should ask Ahmad why he felt dm-crypt needed
to directly support "trusted" type keys.

Mimi

Re: [PATCH v2] fscrypt: support trusted keys

[Ahmad Fatoum]

Hi,

On 12.08.21 02:54, Mimi Zohar wrote:
> On Wed, 2021-08-11 at 10:16 -0700, Eric Biggers wrote:
> 
>> Neither of you actually answered my question, which is whether the support for
>> trusted keys in dm-crypt is a mistake.  I think you're saying that it is?  That
>> would imply that fscrypt shouldn't support trusted keys, but rather encrypted
>> keys -- which conflicts with Ahmad's patch which is adding support for trusted
>> keys.  Note that your reasoning for this is not documented at all in the
>> trusted-encrypted keys documentation; it needs to be (email threads don't really
>> matter), otherwise how would anyone know when/how to use this feature?
> 
> True, but all of the trusted-encrypted key examples in the
> documentation are "encrypted" type keys, encrypted/decrypted based on a
> "trusted" type key.  There are no examples of using the "trusted" key
> type directly.  Before claiming that adding "trusted" key support in
> dm-crypt was a mistake, we should ask Ahmad why he felt dm-crypt needed
> to directly support "trusted" type keys.

I wanted to persist the dm-crypt key as a sealed blob. With encrypted keys,
I would have to persist and unseal two blobs (load trusted key blob, load
encrypted key blob rooted to trusted key) with no extra benefit.

I thus added direct support for trusted keys. Jarkko even commented on the
thread, but didn't voice objection to the approach (or agreement for that
matter), so I assumed the approach is fine.

I can see the utility of using a single trusted key for TPMs, but for CAAM,
I see none and having an encrypted key for every trusted key just makes
it more cumbersome.

In v1 here, I added encrypted key support as well, but dropped it for v2,
because I am not in a position to justify its use. Now that you and Eric
discussed it, should I send v3 with support for both encrypted and trusted
keys like with dm-crypt or how should we proceed?

Cheers,
Ahmad

> 
> Mimi
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] fscrypt: support trusted keys

[Mimi Zohar]

On Tue, 2021-08-17 at 15:04 +0200, Ahmad Fatoum wrote:
> Hi,
> 
> On 12.08.21 02:54, Mimi Zohar wrote:
> > On Wed, 2021-08-11 at 10:16 -0700, Eric Biggers wrote:
> > 
> >> Neither of you actually answered my question, which is whether the support for
> >> trusted keys in dm-crypt is a mistake.  I think you're saying that it is?  That
> >> would imply that fscrypt shouldn't support trusted keys, but rather encrypted
> >> keys -- which conflicts with Ahmad's patch which is adding support for trusted
> >> keys.  Note that your reasoning for this is not documented at all in the
> >> trusted-encrypted keys documentation; it needs to be (email threads don't really
> >> matter), otherwise how would anyone know when/how to use this feature?
> > 
> > True, but all of the trusted-encrypted key examples in the
> > documentation are "encrypted" type keys, encrypted/decrypted based on a
> > "trusted" type key.  There are no examples of using the "trusted" key
> > type directly.  Before claiming that adding "trusted" key support in
> > dm-crypt was a mistake, we should ask Ahmad why he felt dm-crypt needed
> > to directly support "trusted" type keys.
> 
> I wanted to persist the dm-crypt key as a sealed blob. With encrypted keys,
> I would have to persist and unseal two blobs (load trusted key blob, load
> encrypted key blob rooted to trusted key) with no extra benefit.
> 
> I thus added direct support for trusted keys. Jarkko even commented on the
> thread, but didn't voice objection to the approach (or agreement for that
> matter), so I assumed the approach is fine.
> 
> I can see the utility of using a single trusted key for TPMs, but for CAAM,
> I see none and having an encrypted key for every trusted key just makes
> it more cumbersome.
> 
> In v1 here, I added encrypted key support as well, but dropped it for v2,
> because I am not in a position to justify its use. Now that you and Eric
> discussed it, should I send v3 with support for both encrypted and trusted
> keys like with dm-crypt or how should we proceed?

With some applications, the indirection is important.   It allows the
"encrypted" key type to be updated/re-encypted based on a new "trusted"
key, without affecting the on disk encrypted key usage.

As much as I expected, directly using "trusted" keys is a result of the
new trusted key sources.  I have no opinion as to whether this is/isn't
a valid usecase.

thanks,

Mimi

Re: [PATCH v2] fscrypt: support trusted keys

[Ahmad Fatoum]

On 17.08.21 15:55, Mimi Zohar wrote:
> On Tue, 2021-08-17 at 15:04 +0200, Ahmad Fatoum wrote:
>> Hi,
>>
>> On 12.08.21 02:54, Mimi Zohar wrote:
>>> On Wed, 2021-08-11 at 10:16 -0700, Eric Biggers wrote:
>>>
>>>> Neither of you actually answered my question, which is whether the support for
>>>> trusted keys in dm-crypt is a mistake.  I think you're saying that it is?  That
>>>> would imply that fscrypt shouldn't support trusted keys, but rather encrypted
>>>> keys -- which conflicts with Ahmad's patch which is adding support for trusted
>>>> keys.  Note that your reasoning for this is not documented at all in the
>>>> trusted-encrypted keys documentation; it needs to be (email threads don't really
>>>> matter), otherwise how would anyone know when/how to use this feature?
>>>
>>> True, but all of the trusted-encrypted key examples in the
>>> documentation are "encrypted" type keys, encrypted/decrypted based on a
>>> "trusted" type key.  There are no examples of using the "trusted" key
>>> type directly.  Before claiming that adding "trusted" key support in
>>> dm-crypt was a mistake, we should ask Ahmad why he felt dm-crypt needed
>>> to directly support "trusted" type keys.
>>
>> I wanted to persist the dm-crypt key as a sealed blob. With encrypted keys,
>> I would have to persist and unseal two blobs (load trusted key blob, load
>> encrypted key blob rooted to trusted key) with no extra benefit.
>>
>> I thus added direct support for trusted keys. Jarkko even commented on the
>> thread, but didn't voice objection to the approach (or agreement for that
>> matter), so I assumed the approach is fine.
>>
>> I can see the utility of using a single trusted key for TPMs, but for CAAM,
>> I see none and having an encrypted key for every trusted key just makes
>> it more cumbersome.
>>
>> In v1 here, I added encrypted key support as well, but dropped it for v2,
>> because I am not in a position to justify its use. Now that you and Eric
>> discussed it, should I send v3 with support for both encrypted and trusted
>> keys like with dm-crypt or how should we proceed?
> 
> With some applications, the indirection is important.   It allows the
> "encrypted" key type to be updated/re-encypted based on a new "trusted"
> key, without affecting the on disk encrypted key usage.

Those applications were already able to use the encrypted key support
in dm-crypt. For those where re-encryption/PCR-sealing isn't required,
direct trusted key support offers a simpler way to integrate.

> As much as I expected, directly using "trusted" keys is a result of the
> new trusted key sources.

More users = more use cases. You make it sound like a negative
thing.

> I have no opinion as to whether this is/isn't a valid usecase.

So you'd be fine with merging trusted key support as is and leave encrypted
key support to someone who has a valid use case and wants to argue
in its favor?

Cheers,
Ahmad

> 
> thanks,
> 
> Mimi
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] fscrypt: support trusted keys

[Mimi Zohar]

On Tue, 2021-08-17 at 16:13 +0200, Ahmad Fatoum wrote:
> On 17.08.21 15:55, Mimi Zohar wrote:
> > I have no opinion as to whether this is/isn't a valid usecase.
> 
> So you'd be fine with merging trusted key support as is and leave encrypted
> key support to someone who has a valid use case and wants to argue
> in its favor?

That decision as to whether it makes sense to support trusted keys
directly, based on the new trust sources, is a decision left up to the
maintainer(s) of the new usecase and the new trust sources maintainer
Jarkko.

thanks,

Mimi

Re: [PATCH v2] fscrypt: support trusted keys

[Jarkko Sakkinen]

On Tue, Aug 17, 2021 at 10:24:44AM -0400, Mimi Zohar wrote:
> On Tue, 2021-08-17 at 16:13 +0200, Ahmad Fatoum wrote:
> > On 17.08.21 15:55, Mimi Zohar wrote:
> > > I have no opinion as to whether this is/isn't a valid usecase.
> > 
> > So you'd be fine with merging trusted key support as is and leave encrypted
> > key support to someone who has a valid use case and wants to argue
> > in its favor?
> 
> That decision as to whether it makes sense to support trusted keys
> directly, based on the new trust sources, is a decision left up to the
> maintainer(s) of the new usecase and the new trust sources maintainer
> Jarkko.

I'm fine with "direct", as long as also "indirect" is supported.

/Jarkko

Re: [PATCH v2] fscrypt: support trusted keys

[Sumit Garg]

On Tue, 17 Aug 2021 at 19:54, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2021-08-17 at 16:13 +0200, Ahmad Fatoum wrote:
> > On 17.08.21 15:55, Mimi Zohar wrote:
> > > I have no opinion as to whether this is/isn't a valid usecase.
> >
> > So you'd be fine with merging trusted key support as is and leave encrypted
> > key support to someone who has a valid use case and wants to argue
> > in its favor?
>
> That decision as to whether it makes sense to support trusted keys
> directly, based on the new trust sources, is a decision left up to the
> maintainer(s) of the new usecase and the new trust sources maintainer
> Jarkko.
>

I would be in favor of supporting the use of trusted keys directly
when it comes to TEE as a trust source.

-Sumit

> thanks,
>
> Mimi
>