[4/6] fscrypt: verify that the correct master key was supplied

From: Eric Biggers <ebiggers@google.com>

Currently, while a fscrypt master key is required to have a certain
description in the keyring, its payload is never verified to be correct.
While sufficient for well-behaved userspace, this is insecure in a
multi-user system where a user has been given only read-only access to
an encrypted file or directory.  Specifically, if an encrypted file or
directory does not yet have its key cached by the kernel, the first user
who accesses it can provide an arbitrary key in their own keyring, which
the kernel will then associate with the inode and use for read(),
write(), readdir(), etc. by other users as well.

Consequently, it's trivial for a user with *read-only* access to an
encrypted file or directory to make it appear as garbage to everyone.
Creative users might be able to accomplish more sophisticated attacks by
careful choice of the key, e.g. choosing a key causes certain bytes of
file contents to have certain values or that causes filenames containing
the '/' character to appear.

Solve the problem for v2 encryption policies by storing a "hash" of the
master encryption key in the encryption xattr and verifying it before
accepting the user-provided key.  We generate the "hash" using
HKDF-SHA512 by passing a distinct application-specific info string.
This produces a value which is cryptographically isolated and can be
stored in the clear without leaking any information about the master key
or any other derived keys (in a computational sense).  Reusing HKDF is
better than doing e.g. SHA-512(master_key) because it avoids passing the
same key into different cryptographic primitives.

We make the hash field 16 bytes long, as this should provide sufficient
collision and preimage resistance while not wasting too much space for
the encryption xattr.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fscrypt_private.h |  4 ++++
 fs/crypto/keyinfo.c         | 46 +++++++++++++++++++++++++++++++++++++
 fs/crypto/policy.c          | 55 ++++++++++++++++++++++++++++++++++++---------
 3 files changed, 95 insertions(+), 10 deletions(-)

On Wed, Jul 12, 2017 at 02:00:33PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Currently, while a fscrypt master key is required to have a certain
> description in the keyring, its payload is never verified to be correct.
> While sufficient for well-behaved userspace, this is insecure in a
> multi-user system where a user has been given only read-only access to
> an encrypted file or directory.  Specifically, if an encrypted file or
> directory does not yet have its key cached by the kernel, the first user
> who accesses it can provide an arbitrary key in their own keyring, which
> the kernel will then associate with the inode and use for read(),
> write(), readdir(), etc. by other users as well.
> 
> Consequently, it's trivial for a user with *read-only* access to an
> encrypted file or directory to make it appear as garbage to everyone.
> Creative users might be able to accomplish more sophisticated attacks by
> careful choice of the key, e.g. choosing a key causes certain bytes of
> file contents to have certain values or that causes filenames containing
> the '/' character to appear.
> 
> Solve the problem for v2 encryption policies by storing a "hash" of the
> master encryption key in the encryption xattr and verifying it before
> accepting the user-provided key.  We generate the "hash" using
> HKDF-SHA512 by passing a distinct application-specific info string.
> This produces a value which is cryptographically isolated and can be
> stored in the clear without leaking any information about the master key
> or any other derived keys (in a computational sense).  Reusing HKDF is
> better than doing e.g. SHA-512(master_key) because it avoids passing the
> same key into different cryptographic primitives.
> 
> We make the hash field 16 bytes long, as this should provide sufficient
> collision and preimage resistance while not wasting too much space for
> the encryption xattr.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Michael Halcrow <mhalcrow@google.com>

> ---
>  fs/crypto/fscrypt_private.h |  4 ++++
>  fs/crypto/keyinfo.c         | 46 +++++++++++++++++++++++++++++++++++++
>  fs/crypto/policy.c          | 55 ++++++++++++++++++++++++++++++++++++---------
>  3 files changed, 95 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 095e7c16483a..a7baeac92575 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -92,6 +92,7 @@ fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size)
>  struct fscrypt_master_key {
>  	struct crypto_shash	*mk_hmac;
>  	unsigned int		mk_size;
> +	u8			mk_hash[FSCRYPT_KEY_HASH_SIZE];
>  };
>  
>  /*
> @@ -155,6 +156,9 @@ extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
>  					      gfp_t gfp_flags);
>  
>  /* keyinfo.c */
> +extern int fscrypt_compute_key_hash(const struct inode *inode,
> +				    const struct fscrypt_policy *policy,
> +				    u8 hash[FSCRYPT_KEY_HASH_SIZE]);
>  extern void __exit fscrypt_essiv_cleanup(void);
>  
>  #endif /* _FSCRYPT_PRIVATE_H */
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 7ed1a7fb1308..12a60eacf819 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -39,8 +39,11 @@ static struct crypto_shash *essiv_hash_tfm;
>   *
>   * Keys derived with different info strings are cryptographically isolated from
>   * each other --- knowledge of one derived key doesn't reveal any others.
> + * (This property is particularly important for the derived key used as the
> + * "key hash", as that is stored in the clear.)
>   */
>  #define HKDF_CONTEXT_PER_FILE_KEY	1
> +#define HKDF_CONTEXT_KEY_HASH		2
>  
>  /*
>   * HKDF consists of two steps:
> @@ -212,6 +215,12 @@ alloc_master_key(const struct fscrypt_key *payload)
>  	err = crypto_shash_setkey(k->mk_hmac, prk, sizeof(prk));
>  	if (err)
>  		goto fail;
> +
> +	/* Calculate the "key hash" */
> +	err = hkdf_expand(k->mk_hmac, HKDF_CONTEXT_KEY_HASH, NULL, 0,
> +			  k->mk_hash, FSCRYPT_KEY_HASH_SIZE);
> +	if (err)
> +		goto fail;
>  out:
>  	memzero_explicit(prk, sizeof(prk));
>  	return k;
> @@ -537,6 +546,31 @@ void __exit fscrypt_essiv_cleanup(void)
>  	crypto_free_shash(essiv_hash_tfm);
>  }
>  
> +int fscrypt_compute_key_hash(const struct inode *inode,
> +			     const struct fscrypt_policy *policy,
> +			     u8 hash[FSCRYPT_KEY_HASH_SIZE])
> +{
> +	struct fscrypt_master_key *k;
> +	unsigned int min_keysize;
> +
> +	/*
> +	 * Require that the master key be long enough for both the
> +	 * contents and filenames encryption modes.
> +	 */
> +	min_keysize =
> +		max(available_modes[policy->contents_encryption_mode].keysize,
> +		    available_modes[policy->filenames_encryption_mode].keysize);
> +
> +	k = load_master_key_from_keyring(inode, policy->master_key_descriptor,
> +					 min_keysize);
> +	if (IS_ERR(k))
> +		return PTR_ERR(k);
> +
> +	memcpy(hash, k->mk_hash, FSCRYPT_KEY_HASH_SIZE);
> +	put_master_key(k);
> +	return 0;
> +}
> +
>  int fscrypt_get_encryption_info(struct inode *inode)
>  {
>  	struct fscrypt_info *crypt_info;
> @@ -613,6 +647,18 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  			goto out;
>  		}
>  
> +		/*
> +		 * Make sure the master key we found has the correct hash.
> +		 * Buggy or malicious userspace may provide the wrong key.
> +		 */
> +		if (memcmp(crypt_info->ci_master_key->mk_hash, ctx.key_hash,
> +			   FSCRYPT_KEY_HASH_SIZE)) {
> +			pr_warn_ratelimited("fscrypt: wrong encryption key supplied for inode %lu\n",
> +					    inode->i_ino);
> +			res = -ENOKEY;
> +			goto out;
> +		}
> +
>  		res = derive_key_hkdf(crypt_info->ci_master_key, &ctx,
>  				      derived_key, derived_keysize);
>  	}
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index 81c59f8e45c0..2934bc2bff4b 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -40,7 +40,8 @@ static u8 context_version_for_policy(const struct fscrypt_policy *policy)
>   */
>  static bool is_encryption_context_consistent_with_policy(
>  				const struct fscrypt_context *ctx,
> -				const struct fscrypt_policy *policy)
> +				const struct fscrypt_policy *policy,
> +				const u8 key_hash[FSCRYPT_KEY_HASH_SIZE])
>  {
>  	return (ctx->version == context_version_for_policy(policy)) &&
>  		(memcmp(ctx->master_key_descriptor,
> @@ -50,11 +51,14 @@ static bool is_encryption_context_consistent_with_policy(
>  		(ctx->contents_encryption_mode ==
>  		 policy->contents_encryption_mode) &&
>  		(ctx->filenames_encryption_mode ==
> -		 policy->filenames_encryption_mode);
> +		 policy->filenames_encryption_mode) &&
> +		(ctx->version == FSCRYPT_CONTEXT_V1 ||
> +		 (memcmp(ctx->key_hash, key_hash, FSCRYPT_KEY_HASH_SIZE) == 0));
>  }
>  
>  static int create_encryption_context_from_policy(struct inode *inode,
> -				const struct fscrypt_policy *policy)
> +				const struct fscrypt_policy *policy,
> +				const u8 key_hash[FSCRYPT_KEY_HASH_SIZE])
>  {
>  	struct fscrypt_context ctx;
>  
> @@ -74,7 +78,7 @@ static int create_encryption_context_from_policy(struct inode *inode,
>  	BUILD_BUG_ON(sizeof(ctx.nonce) != FS_KEY_DERIVATION_NONCE_SIZE);
>  	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
>  	if (ctx.version != FSCRYPT_CONTEXT_V1)
> -		memset(ctx.key_hash, 0, FSCRYPT_KEY_HASH_SIZE);
> +		memcpy(ctx.key_hash, key_hash, FSCRYPT_KEY_HASH_SIZE);
>  
>  	return inode->i_sb->s_cop->set_context(inode, &ctx,
>  					       fscrypt_context_size(&ctx),
> @@ -87,6 +91,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
>  	struct inode *inode = file_inode(filp);
>  	int ret;
>  	struct fscrypt_context ctx;
> +	u8 key_hash[FSCRYPT_KEY_HASH_SIZE];
>  
>  	if (copy_from_user(&policy, arg, sizeof(policy)))
>  		return -EFAULT;
> @@ -98,6 +103,25 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
>  	    policy.version != FS_POLICY_VERSION_HKDF)
>  		return -EINVAL;
>  
> +	if (policy.version == FS_POLICY_VERSION_ORIGINAL) {
> +		/*
> +		 * Originally no key verification was implemented, which was
> +		 * insufficient for scenarios where multiple users share
> +		 * encrypted files.  The new encryption policy version fixes
> +		 * this and also implements an improved key derivation function.
> +		 * So as long as the key can be in the keyring at the time the
> +		 * policy is set and compatibility with old kernels isn't
> +		 * required, it's recommended to use the new policy version
> +		 * (fscrypt_policy.version = 2).
> +		 */
> +		pr_warn_once("%s (pid %d) is setting less secure v0 encryption policy; recommend upgrading to v2.\n",
> +			     current->comm, current->pid);
> +	} else {
> +		ret = fscrypt_compute_key_hash(inode, &policy, key_hash);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = mnt_want_write_file(filp);
>  	if (ret)
>  		return ret;
> @@ -112,10 +136,12 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
>  			ret = -ENOTEMPTY;
>  		else
>  			ret = create_encryption_context_from_policy(inode,
> -								    &policy);
> +								    &policy,
> +								    key_hash);
>  	} else if (ret >= 0 && fscrypt_valid_context_format(&ctx, ret) &&
>  		   is_encryption_context_consistent_with_policy(&ctx,
> -								&policy)) {
> +								&policy,
> +								key_hash)) {
>  		/* The file already uses the same encryption policy. */
>  		ret = 0;
>  	} else if (ret >= 0 || ret == -ERANGE) {
> @@ -232,7 +258,11 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
>  			(parent_ci->ci_data_mode == child_ci->ci_data_mode) &&
>  			(parent_ci->ci_filename_mode ==
>  			 child_ci->ci_filename_mode) &&
> -			(parent_ci->ci_flags == child_ci->ci_flags);
> +			(parent_ci->ci_flags == child_ci->ci_flags) &&
> +			(parent_ci->ci_context_version == FSCRYPT_CONTEXT_V1 ||
> +			 (memcmp(parent_ci->ci_master_key->mk_hash,
> +				 child_ci->ci_master_key->mk_hash,
> +				 FSCRYPT_KEY_HASH_SIZE) == 0));
>  	}
>  
>  	res = cops->get_context(parent, &parent_ctx, sizeof(parent_ctx));
> @@ -251,7 +281,10 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
>  		 child_ctx.contents_encryption_mode) &&
>  		(parent_ctx.filenames_encryption_mode ==
>  		 child_ctx.filenames_encryption_mode) &&
> -		(parent_ctx.flags == child_ctx.flags);
> +		(parent_ctx.flags == child_ctx.flags) &&
> +		(parent_ctx.version == FSCRYPT_CONTEXT_V1 ||
> +		 (memcmp(parent_ctx.key_hash, child_ctx.key_hash,
> +			 FSCRYPT_KEY_HASH_SIZE) == 0));
>  }
>  EXPORT_SYMBOL(fscrypt_has_permitted_context);
>  
> @@ -286,8 +319,10 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
>  	memcpy(ctx.master_key_descriptor, ci->ci_master_key_descriptor,
>  	       FS_KEY_DESCRIPTOR_SIZE);
>  	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
> -	if (ctx.version != FSCRYPT_CONTEXT_V1)
> -		memset(ctx.key_hash, 0, FSCRYPT_KEY_HASH_SIZE);
> +	if (ctx.version != FSCRYPT_CONTEXT_V1) {
> +		memcpy(ctx.key_hash, ci->ci_master_key->mk_hash,
> +		       FSCRYPT_KEY_HASH_SIZE);
> +	}
>  
>  	BUILD_BUG_ON(sizeof(ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
>  	res = parent->i_sb->s_cop->set_context(child, &ctx,
> -- 
> 2.13.2.932.g7449e964c-goog
>

On Wed, Jul 12, 2017 at 5:00 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> From: Eric Biggers <ebiggers@google.com>
>
>....
> Solve the problem for v2 encryption policies by storing a "hash" of the
> master encryption key in the encryption xattr and verifying it before
> accepting the user-provided key.
> ...

Forgive my ignorance... Doesn't that setup an oracle so an attacker
can query keys?

It seems like the problem is deeper into the design. Namely, the
caching and sharing of keys.

Jeff

Hi Jeff,

On Fri, Jul 14, 2017 at 01:34:48PM -0400, Jeffrey Walton wrote:
> On Wed, Jul 12, 2017 at 5:00 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > From: Eric Biggers <ebiggers@google.com>
> >
> >....
> > Solve the problem for v2 encryption policies by storing a "hash" of the
> > master encryption key in the encryption xattr and verifying it before
> > accepting the user-provided key.
> > ...
> 
> Forgive my ignorance... Doesn't that setup an oracle so an attacker
> can query keys?
> 
> It seems like the problem is deeper into the design. Namely, the
> caching and sharing of keys.
> 

Your concerns are a little vague, so I'll try to cover both major attack
scenarios.  We can assume that key_hash is public information, available to the
attacker.  (For offline attacks that's obviously the case.  For online attacks
it's not obvious since no API exposes it yet, but we'll consider it available
nonetheless, perhaps via a side-channel attack.)

The first attack scenario is breaking the encryption itself, and specifically
whether having key_hash makes it easier.  Fundamentally, adding a 128-bit
key_hash reduces by a factor of 2^128 the number of keys which may be the
correct one.  Thus, to not weaken the encryption, it needs to be infeasible to
enumerate the possible keys which hash to a particular value.  If that could be
done faster than brute force, then recovering the key would in turn be faster
than brute force, so the cryptosystem would be broken in a theoretical sense.

However, in practice I don't think we should be too concerned about it.  SHA-512
has been holding up pretty well to preimage attacks, and enumerating all
possible source messages of a particular length is much more specific than even
a preimage attack actually.  And even if this scheme were, incredibly, fully
broken to the extent that the hash might as well just be the first 128 bits of
the key, if the recommended AES-256-XTS mode is used the master key is actually
512 bits, so there would *still* be 384 bits remaining to brute-force.

The second attack scenario would be not breaking the encryption directly, but
rather causing "someone else's" data to be encrypted (or decrypted) with the
wrong key, or even left unencrypted.  I'll limit the focus to online attacks,
since offline attacks are a different story for this.  Previously we had no
protection against this attack beyond visibility of files: anyone with *read*
access to someone's encrypted directory or files could provision the wrong key.
With this patchset this problem is solved, at least in practice: to provision
the wrong key, the attacker will now need to compute a preimage of key_hash,
which as noted is intended to be computationally expensive.  Note that
brute-forcing a 128-bit hash's preimage would take over 10 million years if you
had 1 trillion computers each performing 1 trillion hashes per second --- so
clearly a practical attack would have to be much more clever.

Now, even if a preimage could be found much more easily, as long as it still had
a decent cost I really, really doubt that an attacker --- who, given that the
attack scenario is *online* very likely already has code execution on the system
--- would perform an expensive cryptographic attack to *maybe* gain the ability
to later decrypt some particular new file contents belonging to one particular
user on one particular device, vs. just exploiting a security vulnerability to
elevate privileges or gain kernel code execution, then reading everything from
memory.  Remember, attacks take the path of least resistance.

Therefore, I actually felt confident enough in the hash to use it as the only
identifier in ->s_master_keys, even though that expands the "providing the wrong
key" attack surface to the whole filesystem rather than just the files visible
to the attacker.  If we are truly concerned about this, we could add raw_key to
struct fscrypt_master_key, always do the keyring lookup, and compare the raw_key
in the keyring key with the fscrypt_master_key.  It would arguably be a step
backwards towards relying less on keyrings and their visibility problems, and it
would leave another copy of the master key in memory, but it could be done.

I think another scenario which you may be getting at is whether the presence of
key_hash makes it easier to test whether a candidate key is the right one.  The
answer is potentially yes, but it doesn't really matter.  If an attacker is
actually able to enumerate your key, then you are already screwed as they can
likely validate it in some other efficient way, e.g. by encrypting some known
plaintext blocks and comparing it to the ciphertext.  If your "encryption" key
doesn't contain enough randomness to prevent it from being enumerated in a
practical sense then it's not encryption --- it's obfuscation.

Eric