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:

quoted

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?

quoted

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_.

quoted

+  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.

quoted

@@ -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;
}

quoted

+		/* 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.

quoted

+		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/ (local)


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 |