Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Wed, Nov 06, 2019 at 04:12:59PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Extend the FS_IOC_ADD_ENCRYPTION_KEY ioctl to allow the raw key to be
> specified by a Linux keyring key, rather than specified directly.
> 
> This is useful because fscrypt keys belong to a particular filesystem
> instance, so they are destroyed when that filesystem is unmounted.
> Usually this is desired.  But in some cases, userspace may need to
> unmount and re-mount the filesystem while keeping the keys, e.g. during
> a system update.  This requires keeping the keys somewhere else too.
> 
> The keys could be kept in memory in a userspace daemon.  But depending
> on the security architecture and assumptions, it can be preferable to
> keep them only in kernel memory, where they are unreadable by userspace.
> 
> We also can't solve this by going back to the original fscrypt API
> (where for each file, the master key was looked up in the process's
> keyring hierarchy) because that caused lots of problems of its own.
> 
> Therefore, add the ability for FS_IOC_ADD_ENCRYPTION_KEY to accept a
> Linux keyring key.  This solves the problem by allowing userspace to (if
> needed) save the keys securely in a Linux keyring for re-provisioning,
> while still using the new fscrypt key management ioctls.
> 
> This is analogous to how dm-crypt accepts a Linux keyring key, but the
> key is then stored internally in the dm-crypt data structures rather
> than being looked up again each time the dm-crypt device is accessed.
> 
> Use a custom key type "fscrypt-provisioning" rather than one of the
> existing key types such as "logon".  This is strongly desired because it
> enforces that these keys are only usable for a particular purpose: for
> fscrypt as input to a particular KDF.  Otherwise, the keys could also be
> passed to any kernel API that accepts a "logon" key with any service
> prefix, e.g. dm-crypt, UBIFS, or (recently proposed) AF_ALG.  This would
> risk leaking information about the raw key despite it ostensibly being
> unreadable.  Of course, this mistake has already been made for multiple
> kernel APIs; but since this is a new API, let's do it right.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  Documentation/filesystems/fscrypt.rst |  35 ++++++-
>  fs/crypto/keyring.c                   | 126 ++++++++++++++++++++++++--
>  include/uapi/linux/fscrypt.h          |  13 ++-
>  3 files changed, 162 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
> index 471a511c75088d..4d15dda36402e0 100644
> --- a/Documentation/filesystems/fscrypt.rst
> +++ b/Documentation/filesystems/fscrypt.rst
> @@ -638,7 +638,8 @@ follows::
>      struct fscrypt_add_key_arg {
>              struct fscrypt_key_specifier key_spec;
>              __u32 raw_size;
> -            __u32 __reserved[9];
> +            __u32 key_id;
> +            __u32 __reserved[8];
>              __u8 raw[];
>      };
>  
> @@ -655,6 +656,12 @@ follows::
>              } u;
>      };
>  
> +    struct fscrypt_key_provisioning_payload {
> +            __u32 type;
> +            __u32 __reserved;
> +            __u8 raw[];
> +    };
> +
>  :c:type:`struct fscrypt_add_key_arg` must be zeroed, then initialized
>  as follows:
>  
> @@ -677,9 +684,26 @@ as follows:
>    ``Documentation/security/keys/core.rst``).
>  
>  - ``raw_size`` must be the size of the ``raw`` key provided, in bytes.
> +  Alternatively, if ``key_id`` is nonzero, this field must be 0, since
> +  in that case the size is implied by the specified Linux keyring key.
> +
> +- ``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 a ``struct
> +  fscrypt_key_provisioning_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_key_provisioning_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,
> +  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.
> +  key, ``raw_size`` bytes long.  Alternatively, if ``key_id`` is
> +  nonzero, then this field is unused.
>  
>  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
> @@ -701,11 +725,16 @@ FS_IOC_ADD_ENCRYPTION_KEY can fail with the following errors:
>  
>  - ``EACCES``: FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR was specified, but the
>    caller does not have the CAP_SYS_ADMIN capability in the initial
> -  user namespace
> +  user namespace; or the raw key was specified by Linux key ID but the
> +  process lacks Search permission on the key.
>  - ``EDQUOT``: the key quota for this user would be exceeded by adding
>    the key
>  - ``EINVAL``: invalid key size or key specifier type, or reserved bits
>    were set
> +- ``EKEYREJECTED``: the raw key was specified by Linux key ID, but the
> +  key has the wrong type
> +- ``ENOKEY``: the raw key was specified by Linux key ID, but no key
> +  exists with that ID
>  - ``ENOTTY``: this type of filesystem does not implement encryption
>  - ``EOPNOTSUPP``: the kernel was not configured with encryption
>    support for this filesystem, or the filesystem superblock has not
> diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
> index 040df1f5e1c8b1..ef5b171c0f1d64 100644
> --- a/fs/crypto/keyring.c
> +++ b/fs/crypto/keyring.c
> @@ -465,6 +465,103 @@ static int add_master_key(struct super_block *sb,
>  	return err;
>  }
>  
> +static int fscrypt_provisioning_key_preparse(struct key_preparsed_payload *prep)
> +{
> +	const struct fscrypt_key_provisioning_payload *payload = prep->data;
> +
> +	if (prep->datalen < sizeof(*payload) + FSCRYPT_MIN_KEY_SIZE ||
> +	    prep->datalen > sizeof(*payload) + FSCRYPT_MAX_KEY_SIZE)
> +		return -EINVAL;

<empty line>

> +	if (payload->type != FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR &&
> +	    payload->type != FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER)
> +		return -EINVAL;

<empty line>

> +	if (payload->__reserved)
> +		return -EINVAL;

<empty line>

> +	prep->payload.data[0] = kmemdup(payload, prep->datalen, GFP_KERNEL);
> +	if (!prep->payload.data[0])
> +		return -ENOMEM;

<empty line>

> +	prep->quotalen = prep->datalen;

<empty line>

> +	return 0;
> +}
> +
> +static void fscrypt_provisioning_key_free_preparse(
> +					struct key_preparsed_payload *prep)
> +{
> +	kzfree(prep->payload.data[0]);
> +}
> +
> +static void fscrypt_provisioning_key_describe(const struct key *key,
> +					      struct seq_file *m)
> +{
> +	seq_puts(m, key->description);
> +	if (key_is_positive(key)) {
> +		const struct fscrypt_key_provisioning_payload *payload =
> +			key->payload.data[0];
> +
> +		seq_printf(m, ": %u [%u]", key->datalen, payload->type);
> +	}
> +}
> +
> +static void fscrypt_provisioning_key_destroy(struct key *key)
> +{
> +	kzfree(key->payload.data[0]);
> +}
> +
> +static struct key_type key_type_fscrypt_provisioning = {
> +	.name			= "fscrypt-provisioning",
> +	.preparse		= fscrypt_provisioning_key_preparse,
> +	.free_preparse		= fscrypt_provisioning_key_free_preparse,
> +	.instantiate		= generic_key_instantiate,
> +	.describe		= fscrypt_provisioning_key_describe,
> +	.destroy		= fscrypt_provisioning_key_destroy,
> +};
> +
> +/*
> + * Retrieve the raw key from the Linux keyring key specified by 'key_id', and
> + * store it into 'secret'.
> + *
> + * The key must be of type "fscrypt-provisioning" and must have the field
> + * fscrypt_key_provisioning_payload::type set to 'type', indicating that it's
> + * only usable with fscrypt with the particular KDF version identified by
> + * 'type'.  We don't use the "logon" key type because there's no way to
> + * completely restrict the use of such keys; they can be used by any kernel API
> + * that accepts "logon" keys and doesn't require a specific service prefix.
> + *
> + * The ability to specify the key via Linux keyring key is intended for cases
> + * where userspace needs to re-add keys after the filesystem is unmounted and
> + * re-mounted.  Most users should just provide the raw key directly instead.
> + */
> +static int get_keyring_key(u32 key_id, u32 type,
> +			   struct fscrypt_master_key_secret *secret)
> +{
> +	key_ref_t ref;
> +	struct key *key;
> +	const struct fscrypt_key_provisioning_payload *payload;
> +	int err;
> +
> +	ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
> +	if (IS_ERR(ref))
> +		return PTR_ERR(ref);

<empty line>

> +	key = key_ref_to_ptr(ref);
> +	if (key->type != &key_type_fscrypt_provisioning)
> +		goto bad_key;

<empty line>

> +	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;
> +
> +	secret->size = key->datalen - sizeof(*payload);
> +	memcpy(secret->raw, payload->raw, secret->size);
> +	err = 0;
> +	goto out_put;

<empty line>

> +bad_key:
> +	err = -EKEYREJECTED;

<empty line>

> +out_put:
> +	key_ref_put(ref);
> +	return err;
> +}
> +
>  /*
>   * Add a master encryption key to the filesystem, causing all files which were
>   * encrypted with it to appear "unlocked" (decrypted) when accessed.
> @@ -503,18 +600,25 @@ int fscrypt_ioctl_add_key(struct file *filp, void __user *_uarg)
>  	if (!valid_key_spec(&arg.key_spec))
>  		return -EINVAL;
>  
> -	if (arg.raw_size < FSCRYPT_MIN_KEY_SIZE ||
> -	    arg.raw_size > FSCRYPT_MAX_KEY_SIZE)
> -		return -EINVAL;
> -
>  	if (memchr_inv(arg.__reserved, 0, sizeof(arg.__reserved)))
>  		return -EINVAL;
>  
>  	memset(&secret, 0, sizeof(secret));
> -	secret.size = arg.raw_size;
> -	err = -EFAULT;
> -	if (copy_from_user(secret.raw, uarg->raw, secret.size))
> -		goto out_wipe_secret;
> +	if (arg.key_id) {
> +		if (arg.raw_size != 0)
> +			return -EINVAL;
> +		err = get_keyring_key(arg.key_id, arg.key_spec.type, &secret);
> +		if (err)
> +			goto out_wipe_secret;
> +	} else {
> +		if (arg.raw_size < FSCRYPT_MIN_KEY_SIZE ||
> +		    arg.raw_size > FSCRYPT_MAX_KEY_SIZE)
> +			return -EINVAL;
> +		secret.size = arg.raw_size;
> +		err = -EFAULT;
> +		if (copy_from_user(secret.raw, uarg->raw, secret.size))
> +			goto out_wipe_secret;
> +	}
>  
>  	switch (arg.key_spec.type) {
>  	case FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR:
> @@ -978,8 +1082,14 @@ int __init fscrypt_init_keyring(void)
>  	if (err)
>  		goto err_unregister_fscrypt;
>  
> +	err = register_key_type(&key_type_fscrypt_provisioning);
> +	if (err)
> +		goto err_unregister_fscrypt_user;
> +
>  	return 0;
>  
> +err_unregister_fscrypt_user:
> +	unregister_key_type(&key_type_fscrypt_user);
>  err_unregister_fscrypt:
>  	unregister_key_type(&key_type_fscrypt);
>  	return err;
> diff --git a/include/uapi/linux/fscrypt.h b/include/uapi/linux/fscrypt.h
> index 1beb174ad95056..605dde7343a4e4 100644
> --- a/include/uapi/linux/fscrypt.h
> +++ b/include/uapi/linux/fscrypt.h
> @@ -109,11 +109,22 @@ struct fscrypt_key_specifier {
>  	} u;
>  };
>  
> +/*
> + * Payload for Linux keyring key of type "fscrypt-provisioning", referenced by
> + * fscrypt_add_key_arg::key_id as an alternative to fscrypt_add_key_arg::raw.
> + */
> +struct fscrypt_key_provisioning_payload {
> +	__u32 type;
> +	__u32 __reserved;
> +	__u8 raw[];
> +};
> +
>  /* Struct passed to FS_IOC_ADD_ENCRYPTION_KEY */
>  struct fscrypt_add_key_arg {
>  	struct fscrypt_key_specifier key_spec;
>  	__u32 raw_size;
> -	__u32 __reserved[9];
> +	__u32 key_id;
> +	__u32 __reserved[8];
>  	__u8 raw[];
>  };
>  
> -- 
> 2.24.0.rc1.363.gb1bccd3e3d-goog
> 

I don't see anything obviously wrong. Just would reformat it a bit.
How you tested it?

/Jarkko