Message-ID: <YQA2fHPwH6EsH9BR@sol.localdomain>
Date:   Tue, 27 Jul 2021 09:38:20 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Ahmad Fatoum <a.fatoum@...gutronix.de>
Cc:     "Theodore Y. Ts'o" <tytso@....edu>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        James Bottomley <jejb@...ux.ibm.com>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        Sumit Garg <sumit.garg@...aro.org>,
        David Howells <dhowells@...hat.com>,
        linux-fscrypt@...r.kernel.org, linux-crypto@...r.kernel.org,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, keyrings@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v1] fscrypt: support encrypted and trusted keys

On Tue, Jul 27, 2021 at 04:43:49PM +0200, Ahmad Fatoum wrote:
> For both v1 and v2 key setup mechanisms, userspace supplies the raw key
> material to the kernel after which it is never again disclosed to
> userspace.
> 
> Use of encrypted and trusted keys offers stronger guarantees:
> The key material is generated within the kernel and is never disclosed to
> userspace in clear text and, in the case of trusted keys, can be
> directly rooted to a trust source like a TPM chip.

Please include a proper justification for this feature and update the relevant
sections of Documentation/filesystems/fscrypt.rst to explain why someone would
want to use this feature and what it accomplishes.

As-is, this feature doesn't seem to have a very strong justification.  Please
also see previous threads where this feature was discussed/requested:
https://lkml.kernel.org/linux-fscrypt/20180110124418.24385-1-git@andred.net/T/#u,
https://lkml.kernel.org/linux-fscrypt/20180118131359.8365-1-git@andred.net/T/#u,
https://lkml.kernel.org/linux-fscrypt/20200116193228.GA266386@vader/T/#u

Note that there are several design flaws with the encrypted and trusted key
types:

- By default, trusted keys are generated using the TPM's RNG rather than the
  kernel's RNG, which places all trust in an unauditable black box.

- trusted and encrypted keys aren't restricted to specific uses in the kernel
  (like the fscrypt-provisioning key type is) but rather are general-purpose.
  Hence, it may be possible to leak their contents to userspace by requesting
  their use for certain algorithms/features, e.g. to encrypt a dm-crypt target
  using a weak cipher that is vulnerable to key recovery attacks.

- "encrypted" keys that use a master key of type "user" are supported, despite
  these being easily obtainable in the clear by userspace providing their own
  master key.  This violates one of the main design goals of "encrypted" keys.

Also, using the "trusted" key type isn't necessary to achieve TPM-bound
encryption, as TPM binding can be handled in userspace instead.

So I really would like to see a proper justification for this feature, and have
it be properly documented.

One comment on the UAPI below.

> 
> Add support for trusted and encrypted keys by repurposing
> fscrypt_add_key_arg::raw to hold the key description when the new
> FSCRYPT_KEY_ARG_TYPE_DESC flag is supplied. The location of the flag
> was previously reserved and enforced by ioctl code to be zero, so this
> change won't break backwards compatibility.
> 
> Corresponding userspace patches are available for fscryptctl:
> https://github.com/google/fscryptctl/pull/23
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@...gutronix.de>
> ---
> key_extract_material used by this patch is added in
> <cover.b2fdd70b830d12853b12a12e32ceb0c8162c1346.1626945419.git-series.a.fatoum@...gutronix.de>
> which still awaits feedback.
> 
> Sending this RFC out anyway to get some feedback from the fscrypt
> developers whether this is the correct way to go about it.
> 
> To: "Theodore Y. Ts'o" <tytso@....edu>
> To: Jaegeuk Kim <jaegeuk@...nel.org>
> To: Eric Biggers <ebiggers@...nel.org>
> Cc: Jarkko Sakkinen <jarkko@...nel.org>
> Cc: James Morris <jmorris@...ei.org>
> Cc: "Serge E. Hallyn" <serge@...lyn.com>
> Cc: James Bottomley <jejb@...ux.ibm.com>
> Cc: Mimi Zohar <zohar@...ux.ibm.com>
> Cc: Sumit Garg <sumit.garg@...aro.org>
> Cc: David Howells <dhowells@...hat.com>
> Cc: linux-fscrypt@...r.kernel.org
> Cc: linux-crypto@...r.kernel.org
> Cc: linux-integrity@...r.kernel.org
> Cc: linux-security-module@...r.kernel.org
> Cc: keyrings@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> ---
>  Documentation/filesystems/fscrypt.rst | 24 ++++++++---
>  fs/crypto/keyring.c                   | 59 ++++++++++++++++++++++++---
>  include/uapi/linux/fscrypt.h          | 16 +++++++-
>  3 files changed, 87 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
> index 44b67ebd6e40..83738af2afa3 100644
> --- a/Documentation/filesystems/fscrypt.rst
> +++ b/Documentation/filesystems/fscrypt.rst
> @@ -681,11 +681,15 @@ It can be executed on any file or directory on the target filesystem,
>  but using the filesystem's root directory is recommended.  It takes in
>  a pointer to struct fscrypt_add_key_arg, defined as follows::
>  
> +    #define FSCRYPT_KEY_ADD_RAW_ASIS		0
> +    #define FSCRYPT_KEY_ADD_RAW_DESC		1
> +
>      struct fscrypt_add_key_arg {
>              struct fscrypt_key_specifier key_spec;
>              __u32 raw_size;
>              __u32 key_id;
> -            __u32 __reserved[8];
> +            __u32 raw_flags;     /* one of FSCRYPT_KEY_ADD_RAW_* */
> +            __u32 __reserved[7];
>              __u8 raw[];
>      };
>  
> @@ -732,8 +736,11 @@ as follows:
>    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
> +- If ``key_id`` is 0, the raw key is given directly in the ``raw``
> +  field if ``raw_flags == FSCRYPT_KEY_ADD_RAW_ASIS``. With
> +  ``raw_flags == FSCRYPT_KEY_ADD_RAW_DESC``, ``raw`` is instead
> +  interpreted as the description of an encrypted or trusted key.
> +  Otherwise ``key_id`` is the ID of a Linux keyring key of
>    type "fscrypt-provisioning" whose payload is
>    struct fscrypt_provisioning_key_payload whose ``raw`` field contains
>    the raw key and whose ``type`` field matches ``key_spec.type``.
> @@ -748,8 +755,15 @@ as follows:
>    without having to store the raw keys in userspace memory.

Why not just allow the key_id field to specify a "trusted" or "encrypted" key?
Why is it necessary for FS_IOC_ADD_ENCRYPTION_KEY to support two different ways
of looking up keyring keys -- by ID and by description?  Looking up by ID works
fine for "fscrypt-provisioning" keys; why are "trusted" and "encrypted" keys
different in this regard?

- Eric

Powered by blists - more mailing lists