linux-fscrypt.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] fscrypt: add support for the encrypted key type
@ 2018-01-18 13:13 André Draszik
  2018-01-26  0:36 ` Eric Biggers
  2018-01-26  0:37 ` Eric Biggers
  0 siblings, 2 replies; 4+ messages in thread
From: André Draszik @ 2018-01-18 13:13 UTC (permalink / raw)
  To: linux-kernel
  Cc: André Draszik, Mimi Zohar, David Howells, James Morris,
	Serge E. Hallyn, Theodore Y. Ts'o, Jaegeuk Kim, Kees Cook,
	Eric Biggers, linux-integrity, keyrings, linux-security-module,
	linux-fscrypt

fscrypt uses a master key for each directory policy from
which all further keys for that policy are derived, and
at the moment such a master key has to be inserted into
a kernel keyring as a 'logon' key by user-space.

While 'logon' keys have the nice property of not being
readable by user-space (keyctl dump etc.), the fscrypt
master key still needs to be generated initially, in a
secure way, and it needs to be saved somewhere for
subsequent reboots, and hence also needs securing itself.

Usage of the 'logon' key means that it is up to user-space
to do all that, opening up the possibility of creating
cryptographically non-sound master keys, or not storing
the master key securely across reboots.

One approach for securing the master key would be to do
that using a TPM and while one could manually do so e.g.
using tpm-tools, that still leaves creation and actual
correct usage of tpm-tools up to user-space, though. As an
aside, tpm-tools needs the tcsd daemon running, which makes
it awkward to use from within an initramfs, and while other
libraries for interfacing with a TPM do exist, there
appears to be a better way:

The kernel already has a concept of trusted as well as
encrypted keys. Trusted keys are TPM backed keys, which can
be sealed to PCRs and also easily be re-sealed against new
future PCRs, and encrypted keys are keys that are encrypted
using another key, e.g. a trusted key. All are generated
automatically by the kernel using the RNG and never leave
kernel memory space (bar any kernel or hardware bugs).

So it seems reasonable to allow the fscrypt subsystem to
work with encrypted keys as well. This is what this change
here does.

We can utilise keys that never ever exist in user-space,
not even at initial creation time, as well as simplifying
usage / configuration. Something very very similar exists
for eCrypts already.

With these patches, we can

    kmk_id=$(keyctl add trusted kmk "new 128 [pcrinfo=...]" @u)
    fscrypt_id=$(keyctl add encrypted fscrypt:1234567890123456 "new default trusted:kmk 64" @u)
    fscryptctl set_policy 1234567890123456 /encrypted

then we can save those keys for after reboot (optionally
TPM-sealed as per the pcrinfo= argument above):

    keyctl pipe ${kmk_id} > /keys/kmk.blob
    keyctl pipe ${fscrypt_id} > /keys/fscrypt.blob

and on subsequent boots we can simply insert them again
(optionally benefitting from the TPM's capability to only
unseal kmk.blob when the system is in the expected state,
thereby also protecting fscrypt.blob):

    keyctl add trusted kmk "load $(cat /keys/kmk.blob)" @u
    keyctl add encrypted fscrypt:1234567890123456 "load $(cat /keys/fscrypt.blob)" @u

Signed-off-by: André Draszik <git@andred.net>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: James Morris <james.l.morris@oracle.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: "Theodore Y. Ts'o" <tytso@mit.edu>
Cc: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Eric Biggers <ebiggers@google.com>
Cc: linux-integrity@vger.kernel.org
Cc: keyrings@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Cc: linux-fscrypt@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
changes in v3:
* merge documentation changes into this commit
* update derive_key_aes() signature to not take
  a struct fscrypt_key and update all callers
* try to use the 'encrypted' key as fallback only
  if the a 'logon' key isn't present (ENOKEY)
* update fscrypt_get_encrypted_key() to take a
  'description', not a 'sig'
* update commit message
* re-add keyrings mailing list and encrypted-keys
  maintainers to Cc

changes in v2:
* dropped the previously added 'fscrypt' encrypted-key,
  and just use the 'default' format
---
 Documentation/filesystems/fscrypt.rst | 56 +++++++++++++++++++--
 fs/crypto/keyinfo.c                   | 92 ++++++++++++++++++++++-------------
 2 files changed, 110 insertions(+), 38 deletions(-)

diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
index 776ddc655f79..852ac2900b66 100644
--- a/Documentation/filesystems/fscrypt.rst
+++ b/Documentation/filesystems/fscrypt.rst
@@ -368,11 +368,19 @@ Adding keys
 To provide a master key, userspace must add it to an appropriate
 keyring using the add_key() system call (see:
 ``Documentation/security/keys/core.rst``).  The key type must be
-"logon"; keys of this type are kept in kernel memory and cannot be
-read back by userspace.  The key description must be "fscrypt:"
-followed by the 16-character lower case hex representation of the
-``master_key_descriptor`` that was set in the encryption policy.  The
-key payload must conform to the following structure::
+either "logon" or "encrypted"; "logon" keys are kept in kernel
+memory and cannot be read back by userspace while "encrypted"
+keys can be rooted in a "trusted" key and thus are protected by
+a TPM and cannot be read by userspace in unencrypted form. Note
+that while an "encrypted" key can also be rooted in a "user" key,
+any "encrypted" key rooted in a "user" key can effectively be
+retrieved in the clear, hence only rooting the key in a "trusted"
+key has any useful security properties!
+
+The key description must be "fscrypt:" followed by the 16-character
+lower case hex representation of the ``master_key_descriptor`` that
+was set in the encryption policy.  For a "logon" key, key payload
+must conform to the following structure::
 
     #define FS_MAX_KEY_SIZE 64
 
@@ -386,6 +394,17 @@ key payload must conform to the following structure::
 ``raw`` with ``size`` indicating its size in bytes.  That is, the
 bytes ``raw[0..size-1]`` (inclusive) are the actual key.
 
+When using an "encrypted" key, only the actual ``raw`` key from above
+fscrypt_key structure is needed::
+
+    keyctl add encrypted "fscrypt:``master_key_descriptor``" "new default trusted:``master-key-name`` ``size``" ``ring``
+    keyctl add encrypted "fscrypt:``master_key_descriptor``" "load ``hex_blob``" ``ring``
+
+Where::
+
+    master-key-name:= name of the trusted key this fscrypt master key
+                      shall be rooted in
+
 The key description prefix "fscrypt:" may alternatively be replaced
 with a filesystem-specific prefix such as "ext4:".  However, the
 filesystem-specific prefixes are deprecated and should not be used in
@@ -412,6 +431,33 @@ evicted.  In the future there probably should be a way to provide keys
 directly to the filesystem instead, which would make the intended
 semantics clearer.
 
+Complete Examples
+------------------
+
+Set fscrypt policy on an (empty) encrypted directory, /encrypted::
+
+    $ fscryptctl set_policy 1234567890123456 /encrypted
+
+Create an encrypted key "1234567890123456" of length 64 bytes with format
+'fscrypt' and root it in a previously loaded trusted "kmk"::
+
+    $ keyctl add encrypted "fscrypt:1234567890123456" "new default trusted:kmk 64" @u
+    839715473
+
+    $ keyctl print 839715473
+    default trusted:kmk 64 e98a49dc11eb9312f46530879aac869300ee734035100f4ee
+    5441279369a4c9d83d6e59b8158d0a3de01790c0bb99af82e9603cb6977c7d1229338cda
+    80375aaf034678405a00c19806d6fb12490e39b1d7ca603c491b58a962345160e344ae51
+    83483e066692d05f5ab3d8b9ea39cab0e
+
+    $ keyctl pipe 839715473 > fscrypt.blob
+
+The directory policy will remain across reboots, so after a reboot the key
+generated earlier will simply have to be loaded into the kernel keyring
+again::
+
+    $ keyctl add encrypted fscrypt:1234567890123456 "load $(cat fscrypt.blob)" @u
+
 Access semantics
 ================
 
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 5e6e846f5a24..3d20addadcd4 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -10,6 +10,7 @@
  */
 
 #include <keys/user-type.h>
+#include <keys/encrypted-type.h>
 #include <linux/scatterlist.h>
 #include <linux/ratelimit.h>
 #include <crypto/aes.h>
@@ -20,14 +21,16 @@ static struct crypto_shash *essiv_hash_tfm;
 
 /**
  * derive_key_aes() - Derive a key using AES-128-ECB
- * @deriving_key: Encryption key used for derivation.
- * @source_key:   Source key to which to apply derivation.
- * @derived_raw_key:  Derived raw key.
+ * @deriving_key:    Encryption key used for derivation.
+ * @source_key:      Raw source key to which to apply derivation.
+ * @source_key_len:  Length of the source key.
+ * @derived_raw_key: Derived raw key.
  *
  * Return: Zero on success; non-zero otherwise.
  */
 static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
-				const struct fscrypt_key *source_key,
+				const u8 source_key[FS_MAX_KEY_SIZE],
+				u32 source_key_len,
 				u8 derived_raw_key[FS_MAX_KEY_SIZE])
 {
 	int res = 0;
@@ -55,9 +58,9 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
 	if (res < 0)
 		goto out;
 
-	sg_init_one(&src_sg, source_key->raw, source_key->size);
-	sg_init_one(&dst_sg, derived_raw_key, source_key->size);
-	skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size,
+	sg_init_one(&src_sg, source_key, source_key_len);
+	sg_init_one(&dst_sg, derived_raw_key, source_key_len);
+	skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key_len,
 				   NULL);
 	res = crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
 out:
@@ -66,14 +69,21 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
 	return res;
 }
 
-static int validate_user_key(struct fscrypt_info *crypt_info,
+static inline struct key *fscrypt_get_encrypted_key(const char *description)
+{
+	if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS))
+		return request_key(&key_type_encrypted, description, NULL);
+	return ERR_PTR(-ENOKEY);
+}
+
+static int validate_keyring_key(struct fscrypt_info *crypt_info,
 			struct fscrypt_context *ctx, u8 *raw_key,
 			const char *prefix, int min_keysize)
 {
 	char *description;
 	struct key *keyring_key;
-	struct fscrypt_key *master_key;
-	const struct user_key_payload *ukp;
+	const u8 *master_key;
+	u32 master_key_len;
 	int res;
 
 	description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
@@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
 		return -ENOMEM;
 
 	keyring_key = request_key(&key_type_logon, description, NULL);
+	if (keyring_key == ERR_PTR(-ENOKEY))
+		keyring_key = fscrypt_get_encrypted_key(description);
 	kfree(description);
 	if (IS_ERR(keyring_key))
 		return PTR_ERR(keyring_key);
 	down_read(&keyring_key->sem);
 
-	if (keyring_key->type != &key_type_logon) {
+	if (keyring_key->type == &key_type_logon) {
+		const struct user_key_payload *ukp;
+		const struct fscrypt_key *fk;
+
+		ukp = user_key_payload_locked(keyring_key);
+		if (!ukp) {
+			/* key was revoked before we acquired its semaphore */
+			res = -EKEYREVOKED;
+			goto out;
+		}
+		if (ukp->datalen != sizeof(struct fscrypt_key)) {
+			res = -EINVAL;
+			goto out;
+		}
+		fk = (struct fscrypt_key *)ukp->data;
+		master_key = fk->raw;
+		master_key_len = fk->size;
+	} else if (keyring_key->type == &key_type_encrypted) {
+		const struct encrypted_key_payload *ekp;
+
+		ekp = keyring_key->payload.data[0];
+		master_key = ekp->decrypted_data;
+		master_key_len = ekp->decrypted_datalen;
+	} else {
 		printk_once(KERN_WARNING
-				"%s: key type must be logon\n", __func__);
+				"%s: key type must be logon or encrypted\n",
+				__func__);
 		res = -ENOKEY;
 		goto out;
 	}
-	ukp = user_key_payload_locked(keyring_key);
-	if (!ukp) {
-		/* key was revoked before we acquired its semaphore */
-		res = -EKEYREVOKED;
-		goto out;
-	}
-	if (ukp->datalen != sizeof(struct fscrypt_key)) {
-		res = -EINVAL;
-		goto out;
-	}
-	master_key = (struct fscrypt_key *)ukp->data;
 	BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
 
-	if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
-	    || master_key->size % AES_BLOCK_SIZE != 0) {
+	if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE
+	    || master_key_len % AES_BLOCK_SIZE != 0) {
 		printk_once(KERN_WARNING
-				"%s: key size incorrect: %d\n",
-				__func__, master_key->size);
+				"%s: key size incorrect: %u\n",
+				__func__, master_key_len);
 		res = -ENOKEY;
 		goto out;
 	}
-	res = derive_key_aes(ctx->nonce, master_key, raw_key);
+	res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key);
+
 out:
 	up_read(&keyring_key->sem);
 	key_put(keyring_key);
@@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	if (!raw_key)
 		goto out;
 
-	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
-				keysize);
+	res = validate_keyring_key(crypt_info, &ctx, raw_key,
+				   FS_KEY_DESC_PREFIX, keysize);
 	if (res && inode->i_sb->s_cop->key_prefix) {
-		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
-					     inode->i_sb->s_cop->key_prefix,
-					     keysize);
+		int res2 = validate_keyring_key(crypt_info, &ctx, raw_key,
+						inode->i_sb->s_cop->key_prefix,
+						keysize);
 		if (res2) {
 			if (res2 == -ENOKEY)
 				res = -ENOKEY;
-- 
2.15.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] fscrypt: add support for the encrypted key type
  2018-01-18 13:13 [PATCH v3] fscrypt: add support for the encrypted key type André Draszik
@ 2018-01-26  0:36 ` Eric Biggers
  2018-01-26  0:37 ` Eric Biggers
  1 sibling, 0 replies; 4+ messages in thread
From: Eric Biggers @ 2018-01-26  0:36 UTC (permalink / raw)
  To: André Draszik
  Cc: linux-kernel, Mimi Zohar, David Howells, James Morris,
	Serge E. Hallyn, Theodore Y. Ts'o, Jaegeuk Kim, Kees Cook,
	Eric Biggers, linux-integrity, keyrings, linux-security-module,
	linux-fscrypt

On Thu, Jan 18, 2018 at 01:13:59PM +0000, Andr� Draszik wrote:
> fscrypt uses a master key for each directory policy from
> which all further keys for that policy are derived, and
> at the moment such a master key has to be inserted into
> a kernel keyring as a 'logon' key by user-space.
> 
> While 'logon' keys have the nice property of not being
> readable by user-space (keyctl dump etc.), the fscrypt

There is no 'keyctl dump' command.  Probably you meant 'keyctl read'.

> master key still needs to be generated initially, in a
> secure way, and it needs to be saved somewhere for
> subsequent reboots, and hence also needs securing itself.
> 
> Usage of the 'logon' key means that it is up to user-space
> to do all that, opening up the possibility of creating
> cryptographically non-sound master keys, or not storing
> the master key securely across reboots.
> 
> One approach for securing the master key would be to do
> that using a TPM and while one could manually do so e.g.
> using tpm-tools, that still leaves creation and actual
> correct usage of tpm-tools up to user-space, though. As an
> aside, tpm-tools needs the tcsd daemon running, which makes
> it awkward to use from within an initramfs, and while other
> libraries for interfacing with a TPM do exist, there
> appears to be a better way:
> 
> The kernel already has a concept of trusted as well as
> encrypted keys. Trusted keys are TPM backed keys, which can
> be sealed to PCRs and also easily be re-sealed against new
> future PCRs, and encrypted keys are keys that are encrypted
> using another key, e.g. a trusted key. All are generated
> automatically by the kernel using the RNG and never leave
> kernel memory space (bar any kernel or hardware bugs).

Saying "the RNG" is unclear, since "encrypted" keys are generated using the
kernel's RNG whereas "trusted" keys are generated using the TPM's RNG.

> 
> So it seems reasonable to allow the fscrypt subsystem to
> work with encrypted keys as well. This is what this change
> here does.
> 
> We can utilise keys that never ever exist in user-space,
> not even at initial creation time, as well as simplifying
> usage / configuration. Something very very similar exists
> for eCrypts already.

typo: eCrypts => eCryptfs

[...]
> diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
> index 776ddc655f79..852ac2900b66 100644
> --- a/Documentation/filesystems/fscrypt.rst
> +++ b/Documentation/filesystems/fscrypt.rst
> @@ -368,11 +368,19 @@ Adding keys
>  To provide a master key, userspace must add it to an appropriate
>  keyring using the add_key() system call (see:
>  ``Documentation/security/keys/core.rst``).  The key type must be
> -"logon"; keys of this type are kept in kernel memory and cannot be
> -read back by userspace.  The key description must be "fscrypt:"
> -followed by the 16-character lower case hex representation of the
> -``master_key_descriptor`` that was set in the encryption policy.  The
> -key payload must conform to the following structure::
> +either "logon" or "encrypted"; "logon" keys are kept in kernel
> +memory and cannot be read back by userspace while "encrypted"
> +keys can be rooted in a "trusted" key and thus are protected by
> +a TPM and cannot be read by userspace in unencrypted form. Note
> +that while an "encrypted" key can also be rooted in a "user" key,
> +any "encrypted" key rooted in a "user" key can effectively be
> +retrieved in the clear, hence only rooting the key in a "trusted"
> +key has any useful security properties!
> +
> +The key description must be "fscrypt:" followed by the 16-character
> +lower case hex representation of the ``master_key_descriptor`` that
> +was set in the encryption policy.  For a "logon" key, key payload
> +must conform to the following structure::
>  
>      #define FS_MAX_KEY_SIZE 64
>  
> @@ -386,6 +394,17 @@ key payload must conform to the following structure::
>  ``raw`` with ``size`` indicating its size in bytes.  That is, the
>  bytes ``raw[0..size-1]`` (inclusive) are the actual key.
>  
> +When using an "encrypted" key, only the actual ``raw`` key from above
> +fscrypt_key structure is needed::
> +
> +    keyctl add encrypted "fscrypt:``master_key_descriptor``" "new default trusted:``master-key-name`` ``size``" ``ring``
> +    keyctl add encrypted "fscrypt:``master_key_descriptor``" "load ``hex_blob``" ``ring``
> +
> +Where::
> +
> +    master-key-name:= name of the trusted key this fscrypt master key
> +                      shall be rooted in
> +

We need to be very careful with the wording.  For example it's implied that
encrypted keys cannot be read by userspace because they are protected by a TPM,
but that's not true.  They can be *wrapped* by a key which in turn can be
TPM-sealed, but once unwrapped they are present in the clear in kernel memory,
and it's only the policy of the kernel code that they can't be read back in the
clear.

Also the "logon" and "encrypted" key types should probably be in their own
subsections, so that there is space to explain them properly.

I've tried reorganizing the section a bit and this is what I came up with:

Adding keys
-----------

To provide a master key, userspace must add it to an appropriate
keyring using the add_key() system call (see:
``Documentation/security/keys/core.rst``).

The key description must be "fscrypt:" followed by the 16-character
lower case hex representation of the ``master_key_descriptor`` that
was set in the encryption policy.  The "fscrypt:" prefix can
alternatively be replaced with a filesystem-specific prefix such as
"ext4:".  However, the filesystem-specific prefixes are deprecated and
should not be used in new programs.

The key type must be "logon" or "encrypted", as described below.
Support for the "logon" key type will always be available when any
filesystem has fscrypt support enabled, whereas support for the
"encrypted" key type additionally requires CONFIG_ENCRYPTED_KEYS=y.

Logon key type
~~~~~~~~~~~~~~

With the "logon" type, userspace is responsible for generating or
unwrapping the master key, then passing it to the kernel in the
following format:

    #define FS_MAX_KEY_SIZE 64

    struct fscrypt_key {
            u32 mode;
            u8 raw[FS_MAX_KEY_SIZE];
            u32 size;
    };

``mode`` is ignored; just set it to 0.  The actual key is provided in
``raw`` with ``size`` indicating its size in bytes.  That is, the
bytes ``raw[0..size-1]`` (inclusive) are the actual key.

The "logon" key will then be stored in kernel memory for the
filesystem to use.  There is no API for userspace to read it back.

Encrypted key type
~~~~~~~~~~~~~~~~~~

With the "encrypted" key type, the key is initially randomly generated
by the kernel.  Then, userspace can read it back as a blob wrapped by
another key.  In particular, it can be wrapped by a "trusted" key,
which can be sealed to a TPM in a certain state, with only the
TPM-sealed blob being made available to userspace.  Then, userspace
can later re-instantiate the "encrypted" key by first re-instantiating
the needed "trusted" key, then providing the encrypted-key blob to be
unwrapped by the kernel.  For example:

    # create a new encrypted-key
    keyctl add encrypted "fscrypt:``master_key_descriptor``" "new default trusted:``master-key-desc`` ``size``" ``ring``

    # unwrap an existing encrypted-key blob
    keyctl add encrypted "fscrypt:``master_key_descriptor``" "load ``hex_blob``" ``ring``

Where::

    master-key-desc:= description of the trusted key this fscrypt master key
                      shall be wrapped by

    size := desired size of the fscrypt master key in bytes

    ring := keyring to add the key to

The "encrypted" key must be instantiated using the "default" format,
since its decrypted payload is treated as the raw master key; that is,
the ``fscrypt_key`` structure is not used.

Note that just like "logon" keys, "encrypted" keys are actually
present in kernel memory in the clear after being added.  Thus, they
are not safe from attacks that compromise kernel memory.  However,
they cannot be read back in the clear using the system call interface.

Choice of keyring
~~~~~~~~~~~~~~~~~

There are several different types of keyrings in which fscrypt master
keys may be placed, such as a session keyring, a user session keyring,
or a user keyring.  Each key must be placed in a keyring that is
"attached" to all processes that might need to access files encrypted
with it, in the sense that request_key() will find the key.
Generally, if only processes belonging to a specific user need to
access a given encrypted directory and no session keyring has been
installed, then that directory's key should be placed in that user's
user session keyring or user keyring.  Otherwise, a session keyring
should be installed if needed, and the key should be linked into that
session keyring, or in a keyring linked into that session keyring.

Note: introducing the complex visibility semantics of keyrings here
was arguably a mistake --- especially given that by design, after any
process successfully opens an encrypted file (thereby setting up the
per-file key), possessing the keyring key is not actually required for
any process to read/write the file until its in-memory inode is
evicted.  In the future there probably should be a way to provide keys
directly to the filesystem instead, which would make the intended
semantics clearer.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] fscrypt: add support for the encrypted key type
  2018-01-18 13:13 [PATCH v3] fscrypt: add support for the encrypted key type André Draszik
  2018-01-26  0:36 ` Eric Biggers
@ 2018-01-26  0:37 ` Eric Biggers
  2018-01-29 18:19   ` Eric Biggers
  1 sibling, 1 reply; 4+ messages in thread
From: Eric Biggers @ 2018-01-26  0:37 UTC (permalink / raw)
  To: André Draszik
  Cc: linux-kernel, Mimi Zohar, David Howells, James Morris,
	Serge E. Hallyn, Theodore Y. Ts'o, Jaegeuk Kim, Kees Cook,
	Eric Biggers, linux-integrity, keyrings, linux-security-module,
	linux-fscrypt

On Thu, Jan 18, 2018 at 01:13:59PM +0000, Andr� Draszik wrote:
> -static int validate_user_key(struct fscrypt_info *crypt_info,
> +static inline struct key *fscrypt_get_encrypted_key(const char *description)
> +{
> +	if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS))
> +		return request_key(&key_type_encrypted, description, NULL);
> +	return ERR_PTR(-ENOKEY);
> +}
> +
> +static int validate_keyring_key(struct fscrypt_info *crypt_info,
>  			struct fscrypt_context *ctx, u8 *raw_key,
>  			const char *prefix, int min_keysize)
>  {
>  	char *description;
>  	struct key *keyring_key;
> -	struct fscrypt_key *master_key;
> -	const struct user_key_payload *ukp;
> +	const u8 *master_key;
> +	u32 master_key_len;
>  	int res;
>  
>  	description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
> @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
>  		return -ENOMEM;
>  
>  	keyring_key = request_key(&key_type_logon, description, NULL);
> +	if (keyring_key == ERR_PTR(-ENOKEY))
> +		keyring_key = fscrypt_get_encrypted_key(description);
>  	kfree(description);
>  	if (IS_ERR(keyring_key))
>  		return PTR_ERR(keyring_key);
>  	down_read(&keyring_key->sem);
>  
> -	if (keyring_key->type != &key_type_logon) {
> +	if (keyring_key->type == &key_type_logon) {
> +		const struct user_key_payload *ukp;
> +		const struct fscrypt_key *fk;
> +
> +		ukp = user_key_payload_locked(keyring_key);
> +		if (!ukp) {
> +			/* key was revoked before we acquired its semaphore */
> +			res = -EKEYREVOKED;
> +			goto out;
> +		}
> +		if (ukp->datalen != sizeof(struct fscrypt_key)) {
> +			res = -EINVAL;
> +			goto out;
> +		}
> +		fk = (struct fscrypt_key *)ukp->data;
> +		master_key = fk->raw;
> +		master_key_len = fk->size;
> +	} else if (keyring_key->type == &key_type_encrypted) {
> +		const struct encrypted_key_payload *ekp;
> +
> +		ekp = keyring_key->payload.data[0];
> +		master_key = ekp->decrypted_data;
> +		master_key_len = ekp->decrypted_datalen;
> +	} else {
>  		printk_once(KERN_WARNING
> -				"%s: key type must be logon\n", __func__);
> +				"%s: key type must be logon or encrypted\n",
> +				__func__);
>  		res = -ENOKEY;
>  		goto out;
>  	}
> -	ukp = user_key_payload_locked(keyring_key);
> -	if (!ukp) {
> -		/* key was revoked before we acquired its semaphore */
> -		res = -EKEYREVOKED;
> -		goto out;
> -	}
> -	if (ukp->datalen != sizeof(struct fscrypt_key)) {
> -		res = -EINVAL;
> -		goto out;
> -	}
> -	master_key = (struct fscrypt_key *)ukp->data;
>  	BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
>  
> -	if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
> -	    || master_key->size % AES_BLOCK_SIZE != 0) {
> +	if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE
> +	    || master_key_len % AES_BLOCK_SIZE != 0) {
>  		printk_once(KERN_WARNING
> -				"%s: key size incorrect: %d\n",
> -				__func__, master_key->size);
> +				"%s: key size incorrect: %u\n",
> +				__func__, master_key_len);
>  		res = -ENOKEY;
>  		goto out;
>  	}
> -	res = derive_key_aes(ctx->nonce, master_key, raw_key);
> +	res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key);
> +
>  out:
>  	up_read(&keyring_key->sem);
>  	key_put(keyring_key);
> @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	if (!raw_key)
>  		goto out;
>  
> -	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> -				keysize);
> +	res = validate_keyring_key(crypt_info, &ctx, raw_key,
> +				   FS_KEY_DESC_PREFIX, keysize);
>  	if (res && inode->i_sb->s_cop->key_prefix) {
> -		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> -					     inode->i_sb->s_cop->key_prefix,
> -					     keysize);
> +		int res2 = validate_keyring_key(crypt_info, &ctx, raw_key,
> +						inode->i_sb->s_cop->key_prefix,
> +						keysize);
>  		if (res2) {
>  			if (res2 == -ENOKEY)
>  				res = -ENOKEY;
> -- 
> 2.15.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe keyrings" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

The code changes here look fine, but unfortunately there is a lock ordering
problem exposed by using a key type that implements the key_type ->read()
method.  The problem is that encrypted_read() can take a page fault during
copy_to_user() while holding the key semaphore, which then (with ext4) can
trigger the start of a jbd2 transaction.  Meanwhile
fscrypt_get_encryption_info() can be called from within a jbd2 transaction via
fscrypt_inherit_context(), which results in a different locking order.  We
probably will need to fix this by removing the call to
fscrypt_get_encryption_info() from fscrypt_inherit_context().

I've pasted the lockdep report I got below:

[  432.705339] 
[  432.705681] ======================================================
[  432.707015] WARNING: possible circular locking dependency detected
[  432.708155] 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 Not tainted
[  432.709365] ------------------------------------------------------
[  432.710482] keyctl/877 is trying to acquire lock:
[  432.711286]  (&mm->mmap_sem){++++}, at: [<00000000e8bba1c7>] __might_fault+0xce/0x1a0
[  432.712688] 
[  432.712688] but task is already holding lock:
[  432.713684]  (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[  432.715176] 
[  432.715176] which lock already depends on the new lock.
[  432.715176] 
[  432.716601] 
[  432.716601] the existing dependency chain (in reverse order) is:
[  432.717901] 
[  432.717901] -> #3 (&type->lock_class#2){++++}:
[  432.718924]        lock_acquire+0xaa/0x170
[  432.719632]        down_read+0x37/0xa0
[  432.720298]        validate_keyring_key.isra.1+0x97/0x410
[  432.721218]        fscrypt_get_encryption_info+0x724/0x1200
[  432.722233]        fscrypt_inherit_context+0x2c1/0x330
[  432.723067]        __ext4_new_inode+0x34f5/0x44d0
[  432.723830]        ext4_create+0x195/0x4a0
[  432.724499]        lookup_open+0xbe2/0x1400
[  432.725185]        path_openat+0xb31/0x1e50
[  432.725876]        do_filp_open+0x175/0x250
[  432.726572]        do_sys_open+0x219/0x3f0
[  432.727312]        entry_SYSCALL_64_fastpath+0x1e/0x8b
[  432.728153] 
[  432.728153] -> #2 (jbd2_handle){.+.+}:
[  432.729008]        lock_acquire+0xaa/0x170
[  432.729680]        start_this_handle+0x47b/0x1020
[  432.730463]        jbd2__journal_start+0x32e/0x5c0
[  432.731276]        __ext4_journal_start_sb+0xa8/0x190
[  432.732183]        ext4_truncate+0x4dd/0xd00
[  432.732868]        ext4_setattr+0xa62/0x1ac0
[  432.733528]        notify_change+0x672/0xdb0
[  432.734198]        do_truncate+0x111/0x1c0
[  432.734831]        path_openat+0xee6/0x1e50
[  432.735476]        do_filp_open+0x175/0x250
[  432.736149]        do_sys_open+0x219/0x3f0
[  432.736785]        entry_SYSCALL_64_fastpath+0x1e/0x8b
[  432.737576] 
[  432.737576] -> #1 (&ei->i_mmap_sem){++++}:
[  432.738508]        lock_acquire+0xaa/0x170
[  432.739148]        down_read+0x37/0xa0
[  432.739735]        ext4_filemap_fault+0x7b/0xb0
[  432.740446]        __do_fault+0x7a/0x200
[  432.741060]        __handle_mm_fault+0x6e0/0x2040
[  432.741801]        handle_mm_fault+0x194/0x320
[  432.742494]        __do_page_fault+0x4e1/0xa70
[  432.743184]        page_fault+0x2c/0x60
[  432.743784]        __clear_user+0x3d/0x60
[  432.744409]        clear_user+0x76/0xa0
[  432.745009]        load_elf_binary+0x2bf6/0x2f10
[  432.745753]        search_binary_handler+0x136/0x450
[  432.746522]        do_execveat_common.isra.12+0x1241/0x19c0
[  432.747339]        do_execve+0x2c/0x40
[  432.747881]        try_to_run_init_process+0x12/0x40
[  432.748611]        kernel_init+0xf2/0x180
[  432.749190]        ret_from_fork+0x3a/0x50
[  432.749785] 
[  432.749785] -> #0 (&mm->mmap_sem){++++}:
[  432.750576]        __lock_acquire+0x8d1/0x12d0
[  432.751232]        lock_acquire+0xaa/0x170
[  432.751848]        __might_fault+0x12b/0x1a0
[  432.752478]        _copy_to_user+0x27/0xc0
[  432.753080]        encrypted_read+0x54d/0x7b0
[  432.753734]        keyctl_read_key+0x1f2/0x240
[  432.754396]        SyS_keyctl+0x184/0x2a0
[  432.754995]        entry_SYSCALL_64_fastpath+0x1e/0x8b
[  432.755778] 
[  432.755778] other info that might help us debug this:
[  432.755778] 
[  432.756972] Chain exists of:
[  432.756972]   &mm->mmap_sem --> jbd2_handle --> &type->lock_class#2
[  432.756972] 
[  432.758498]  Possible unsafe locking scenario:
[  432.758498] 
[  432.759302]        CPU0                    CPU1
[  432.759919]        ----                    ----
[  432.760536]   lock(&type->lock_class#2);
[  432.761100]                                lock(jbd2_handle);
[  432.761926]                                lock(&type->lock_class#2);
[  432.762836]   lock(&mm->mmap_sem);
[  432.763348] 
[  432.763348]  *** DEADLOCK ***
[  432.763348] 
[  432.764209] 1 lock held by keyctl/877:
[  432.764765]  #0:  (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[  432.766045] 
[  432.766045] stack backtrace:
[  432.766671] CPU: 1 PID: 877 Comm: keyctl Not tainted 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31
[  432.768021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[  432.769211] Call Trace:
[  432.769559]  dump_stack+0x8d/0xd5
[  432.770041]  print_circular_bug.isra.20+0x321/0x5e0
[  432.770712]  ? save_trace+0xd6/0x250
[  432.771185]  check_prev_add.constprop.27+0xa2a/0x1670
[  432.771867]  ? depot_save_stack+0x2d5/0x490
[  432.772504]  ? check_usage+0x13c0/0x13c0
[  432.773047]  ? trace_hardirqs_on_caller+0x3dc/0x550
[  432.773712]  ? _raw_spin_unlock_irqrestore+0x39/0x60
[  432.774401]  ? depot_save_stack+0x2d5/0x490
[  432.774979]  ? kasan_kmalloc+0x13a/0x170
[  432.775522]  ? validate_chain.isra.24+0x13ee/0x2410
[  432.776187]  validate_chain.isra.24+0x13ee/0x2410
[  432.776831]  ? check_prev_add.constprop.27+0x1670/0x1670
[  432.777546]  __lock_acquire+0x8d1/0x12d0
[  432.778088]  lock_acquire+0xaa/0x170
[  432.778576]  ? __might_fault+0xce/0x1a0
[  432.779096]  __might_fault+0x12b/0x1a0
[  432.779608]  ? __might_fault+0xce/0x1a0
[  432.780029]  _copy_to_user+0x27/0xc0
[  432.780438]  encrypted_read+0x54d/0x7b0
[  432.780858]  ? key_task_permission+0x16e/0x3b0
[  432.781340]  ? encrypted_instantiate+0x8b0/0x8b0
[  432.781851]  ? creds_are_invalid+0x9/0x50
[  432.782287]  ? lookup_user_key+0x19a/0xf50
[  432.782736]  ? __lock_acquire+0x8d1/0x12d0
[  432.783182]  ? key_validate+0xb0/0xb0
[  432.783602]  ? keyctl_read_key+0x174/0x240
[  432.784058]  keyctl_read_key+0x1f2/0x240
[  432.784486]  SyS_keyctl+0x184/0x2a0
[  432.784875]  entry_SYSCALL_64_fastpath+0x1e/0x8b
[  432.785374] RIP: 0033:0x7f812b6c8259

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] fscrypt: add support for the encrypted key type
  2018-01-26  0:37 ` Eric Biggers
@ 2018-01-29 18:19   ` Eric Biggers
  0 siblings, 0 replies; 4+ messages in thread
From: Eric Biggers @ 2018-01-29 18:19 UTC (permalink / raw)
  To: André Draszik
  Cc: linux-kernel, Mimi Zohar, David Howells, James Morris,
	Serge E. Hallyn, Theodore Y. Ts'o, Jaegeuk Kim, Kees Cook,
	Eric Biggers, linux-integrity, keyrings, linux-security-module,
	linux-fscrypt

On Thu, Jan 25, 2018 at 04:37:48PM -0800, Eric Biggers wrote:
> On Thu, Jan 18, 2018 at 01:13:59PM +0000, Andr� Draszik wrote:
> > -static int validate_user_key(struct fscrypt_info *crypt_info,
> > +static inline struct key *fscrypt_get_encrypted_key(const char *description)
> > +{
> > +	if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS))
> > +		return request_key(&key_type_encrypted, description, NULL);
> > +	return ERR_PTR(-ENOKEY);
> > +}
> > +
> > +static int validate_keyring_key(struct fscrypt_info *crypt_info,
> >  			struct fscrypt_context *ctx, u8 *raw_key,
> >  			const char *prefix, int min_keysize)
> >  {
> >  	char *description;
> >  	struct key *keyring_key;
> > -	struct fscrypt_key *master_key;
> > -	const struct user_key_payload *ukp;
> > +	const u8 *master_key;
> > +	u32 master_key_len;
> >  	int res;
> >  
> >  	description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
> > @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
> >  		return -ENOMEM;
> >  
> >  	keyring_key = request_key(&key_type_logon, description, NULL);
> > +	if (keyring_key == ERR_PTR(-ENOKEY))
> > +		keyring_key = fscrypt_get_encrypted_key(description);
> >  	kfree(description);
> >  	if (IS_ERR(keyring_key))
> >  		return PTR_ERR(keyring_key);
> >  	down_read(&keyring_key->sem);
> >  
> > -	if (keyring_key->type != &key_type_logon) {
> > +	if (keyring_key->type == &key_type_logon) {
> > +		const struct user_key_payload *ukp;
> > +		const struct fscrypt_key *fk;
> > +
> > +		ukp = user_key_payload_locked(keyring_key);
> > +		if (!ukp) {
> > +			/* key was revoked before we acquired its semaphore */
> > +			res = -EKEYREVOKED;
> > +			goto out;
> > +		}
> > +		if (ukp->datalen != sizeof(struct fscrypt_key)) {
> > +			res = -EINVAL;
> > +			goto out;
> > +		}
> > +		fk = (struct fscrypt_key *)ukp->data;
> > +		master_key = fk->raw;
> > +		master_key_len = fk->size;
> > +	} else if (keyring_key->type == &key_type_encrypted) {
> > +		const struct encrypted_key_payload *ekp;
> > +
> > +		ekp = keyring_key->payload.data[0];
> > +		master_key = ekp->decrypted_data;
> > +		master_key_len = ekp->decrypted_datalen;
> > +	} else {
> >  		printk_once(KERN_WARNING
> > -				"%s: key type must be logon\n", __func__);
> > +				"%s: key type must be logon or encrypted\n",
> > +				__func__);
> >  		res = -ENOKEY;
> >  		goto out;
> >  	}
> > -	ukp = user_key_payload_locked(keyring_key);
> > -	if (!ukp) {
> > -		/* key was revoked before we acquired its semaphore */
> > -		res = -EKEYREVOKED;
> > -		goto out;
> > -	}
> > -	if (ukp->datalen != sizeof(struct fscrypt_key)) {
> > -		res = -EINVAL;
> > -		goto out;
> > -	}
> > -	master_key = (struct fscrypt_key *)ukp->data;
> >  	BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
> >  
> > -	if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
> > -	    || master_key->size % AES_BLOCK_SIZE != 0) {
> > +	if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE
> > +	    || master_key_len % AES_BLOCK_SIZE != 0) {
> >  		printk_once(KERN_WARNING
> > -				"%s: key size incorrect: %d\n",
> > -				__func__, master_key->size);
> > +				"%s: key size incorrect: %u\n",
> > +				__func__, master_key_len);
> >  		res = -ENOKEY;
> >  		goto out;
> >  	}
> 
> The code changes here look fine, but unfortunately there is a lock ordering
> problem exposed by using a key type that implements the key_type ->read()
> method.  The problem is that encrypted_read() can take a page fault during
> copy_to_user() while holding the key semaphore, which then (with ext4) can
> trigger the start of a jbd2 transaction.  Meanwhile
> fscrypt_get_encryption_info() can be called from within a jbd2 transaction via
> fscrypt_inherit_context(), which results in a different locking order.  We
> probably will need to fix this by removing the call to
> fscrypt_get_encryption_info() from fscrypt_inherit_context().
> 

Actually, a better idea might be to access the key payload under rcu_read_lock()
rather than the key semaphore.  It looks like that's possible with both the
"logon" and "encrypted" key types.

Note that derive_key_aes() can sleep, so the part under the rcu_read_lock()
would have to just copy the payload to a temporary buffer, and derive_key_aes()
would be done after rcu_read_unlock(), using the temporary buffer.  But I think
you can just reuse the 'raw_key' buffer, so that the encryption operation in
derive_key_aes() is done in-place.

Eric

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-01-29 18:19 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-18 13:13 [PATCH v3] fscrypt: add support for the encrypted key type André Draszik
2018-01-26  0:36 ` Eric Biggers
2018-01-26  0:37 ` Eric Biggers
2018-01-29 18:19   ` Eric Biggers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).