ACK: [Yakkety SRU] Backport fix for CVE-2017-7374

Colin Ian King colin.king at canonical.com
Wed Jun 7 08:52:27 UTC 2017


On 06/06/17 13:22, Stefan Bader wrote:
> Xenial and Zesty already are fixed. But for Yakkety the patch
> needed some adjustments. This backport was test-compiled for
> amd64/yakkety. Of course some more eyeyballing never hurts.
> 
> -Stefan
> 
> 
> ---
> 
> From 1a87bc35c4392db1efa5bbabf99c341ed9962e00 Mon Sep 17 00:00:00 2001
> From: Eric Biggers <ebiggers at google.com>
> Date: Tue, 21 Feb 2017 15:07:11 -0800
> Subject: [PATCH] fscrypt: remove broken support for detecting keyring key
>  revocation
> 
> Filesystem encryption ostensibly supported revoking a keyring key that
> had been used to "unlock" encrypted files, causing those files to become
> "locked" again.  This was, however, buggy for several reasons, the most
> severe of which was that when key revocation happened to be detected for
> an inode, its fscrypt_info was immediately freed, even while other
> threads could be using it for encryption or decryption concurrently.
> This could be exploited to crash the kernel or worse.
> 
> This patch fixes the use-after-free by removing the code which detects
> the keyring key having been revoked, invalidated, or expired.  Instead,
> an encrypted inode that is "unlocked" now simply remains unlocked until
> it is evicted from memory.  Note that this is no worse than the case for
> block device-level encryption, e.g. dm-crypt, and it still remains
> possible for a privileged user to evict unused pages, inodes, and
> dentries by running 'sync; echo 3 > /proc/sys/vm/drop_caches', or by
> simply unmounting the filesystem.  In fact, one of those actions was
> already needed anyway for key revocation to work even somewhat sanely.
> This change is not expected to break any applications.
> 
> In the future I'd like to implement a real API for fscrypt key
> revocation that interacts sanely with ongoing filesystem operations ---
> waiting for existing operations to complete and blocking new operations,
> and invalidating and sanitizing key material and plaintext from the VFS
> caches.  But this is a hard problem, and for now this bug must be fixed.
> 
> This bug affected almost all versions of ext4, f2fs, and ubifs
> encryption, and it was potentially reachable in any kernel configured
> with encryption support (CONFIG_EXT4_ENCRYPTION=y,
> CONFIG_EXT4_FS_ENCRYPTION=y, CONFIG_F2FS_FS_ENCRYPTION=y, or
> CONFIG_UBIFS_FS_ENCRYPTION=y).  Note that older kernels did not use the
> shared fs/crypto/ code, but due to the potential security implications
> of this bug, it may still be worthwhile to backport this fix to them.
> 
> Fixes: b7236e21d55f ("ext4 crypto: reorganize how we store keys in the inode")
> Cc: stable at vger.kernel.org # v4.2+
> Signed-off-by: Eric Biggers <ebiggers at google.com>
> Signed-off-by: Theodore Ts'o <tytso at mit.edu>
> Acked-by: Michael Halcrow <mhalcrow at google.com>
> 
> CVE-2017-7374
> 
> (backportded from 1b53cf9815bb4744958d41f3795d5d5a1d365e2d)
> 
> [function fscrypt_get_crypt_info was still named get_crypt_info,
>  private header does not yet exist]
> 
> Signed-off-by: Stefan Bader <stefan.bader at canonical.com>
> ---
>  fs/crypto/crypto.c       | 10 +---------
>  fs/crypto/fname.c        |  2 +-
>  fs/crypto/keyinfo.c      | 50 +++++++++---------------------------------------
>  include/linux/fscrypto.h |  2 --
>  4 files changed, 11 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index 55d64fb..624807c 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -353,7 +353,6 @@ EXPORT_SYMBOL(fscrypt_zeroout_range);
>  static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
>  {
>  	struct dentry *dir;
> -	struct fscrypt_info *ci;
>  	int dir_has_key, cached_with_key;
>  
>  	if (flags & LOOKUP_RCU)
> @@ -365,18 +364,11 @@ static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
>  		return 0;
>  	}
>  
> -	ci = d_inode(dir)->i_crypt_info;
> -	if (ci && ci->ci_keyring_key &&
> -	    (ci->ci_keyring_key->flags & ((1 << KEY_FLAG_INVALIDATED) |
> -					  (1 << KEY_FLAG_REVOKED) |
> -					  (1 << KEY_FLAG_DEAD))))
> -		ci = NULL;
> -
>  	/* this should eventually be an flag in d_flags */
>  	spin_lock(&dentry->d_lock);
>  	cached_with_key = dentry->d_flags & DCACHE_ENCRYPTED_WITH_KEY;
>  	spin_unlock(&dentry->d_lock);
> -	dir_has_key = (ci != NULL);
> +	dir_has_key = (d_inode(dir)->i_crypt_info != NULL);
>  	dput(dir);
>  
>  	/*
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 5d6d491..8dcbfee 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -360,7 +360,7 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
>  		fname->disk_name.len = iname->len;
>  		return 0;
>  	}
> -	ret = get_crypt_info(dir);
> +	ret = fscrypt_get_encryption_info(dir);
>  	if (ret && ret != -EOPNOTSUPP)
>  		return ret;
>  
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 1ac263e..8ec4371 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -102,6 +102,7 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
>  	kfree(full_key_descriptor);
>  	if (IS_ERR(keyring_key))
>  		return PTR_ERR(keyring_key);
> +	down_read(&keyring_key->sem);
>  
>  	if (keyring_key->type != &key_type_logon) {
>  		printk_once(KERN_WARNING
> @@ -109,11 +110,9 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
>  		res = -ENOKEY;
>  		goto out;
>  	}
> -	down_read(&keyring_key->sem);
>  	ukp = user_key_payload(keyring_key);
>  	if (ukp->datalen != sizeof(struct fscrypt_key)) {
>  		res = -EINVAL;
> -		up_read(&keyring_key->sem);
>  		goto out;
>  	}
>  	master_key = (struct fscrypt_key *)ukp->data;
> @@ -124,17 +123,11 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
>  				"%s: key size incorrect: %d\n",
>  				__func__, master_key->size);
>  		res = -ENOKEY;
> -		up_read(&keyring_key->sem);
>  		goto out;
>  	}
>  	res = derive_key_aes(ctx->nonce, master_key->raw, raw_key);
> -	up_read(&keyring_key->sem);
> -	if (res)
> -		goto out;
> -
> -	crypt_info->ci_keyring_key = keyring_key;
> -	return 0;
>  out:
> +	up_read(&keyring_key->sem);
>  	key_put(keyring_key);
>  	return res;
>  }
> @@ -144,12 +137,11 @@ static void put_crypt_info(struct fscrypt_info *ci)
>  	if (!ci)
>  		return;
>  
> -	key_put(ci->ci_keyring_key);
>  	crypto_free_skcipher(ci->ci_ctfm);
>  	kmem_cache_free(fscrypt_info_cachep, ci);
>  }
>  
> -int get_crypt_info(struct inode *inode)
> +int fscrypt_get_encryption_info(struct inode *inode)
>  {
>  	struct fscrypt_info *crypt_info;
>  	struct fscrypt_context ctx;
> @@ -159,21 +151,15 @@ int get_crypt_info(struct inode *inode)
>  	u8 mode;
>  	int res;
>  
> +	if (inode->i_crypt_info)
> +		return 0;
> +
>  	res = fscrypt_initialize();
>  	if (res)
>  		return res;
>  
>  	if (!inode->i_sb->s_cop->get_context)
>  		return -EOPNOTSUPP;
> -retry:
> -	crypt_info = ACCESS_ONCE(inode->i_crypt_info);
> -	if (crypt_info) {
> -		if (!crypt_info->ci_keyring_key ||
> -				key_validate(crypt_info->ci_keyring_key) == 0)
> -			return 0;
> -		fscrypt_put_encryption_info(inode, crypt_info);
> -		goto retry;
> -	}
>  
>  	res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
>  	if (res < 0) {
> @@ -195,7 +181,6 @@ retry:
>  	crypt_info->ci_data_mode = ctx.contents_encryption_mode;
>  	crypt_info->ci_filename_mode = ctx.filenames_encryption_mode;
>  	crypt_info->ci_ctfm = NULL;
> -	crypt_info->ci_keyring_key = NULL;
>  	memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
>  				sizeof(crypt_info->ci_master_key));
>  	if (S_ISREG(inode->i_mode))
> @@ -257,12 +242,8 @@ got_key:
>  	if (res)
>  		goto out;
>  
> -	memzero_explicit(raw_key, sizeof(raw_key));
> -	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) != NULL) {
> -		put_crypt_info(crypt_info);
> -		goto retry;
> -	}
> -	return 0;
> +	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) == NULL)
> +		crypt_info = NULL;
>  
>  out:
>  	if (res == -ENOKEY)
> @@ -271,6 +252,7 @@ out:
>  	memzero_explicit(raw_key, sizeof(raw_key));
>  	return res;
>  }
> +EXPORT_SYMBOL(fscrypt_get_encryption_info);
>  
>  void fscrypt_put_encryption_info(struct inode *inode, struct fscrypt_info *ci)
>  {
> @@ -288,17 +270,3 @@ void fscrypt_put_encryption_info(struct inode *inode, struct fscrypt_info *ci)
>  	put_crypt_info(ci);
>  }
>  EXPORT_SYMBOL(fscrypt_put_encryption_info);
> -
> -int fscrypt_get_encryption_info(struct inode *inode)
> -{
> -	struct fscrypt_info *ci = inode->i_crypt_info;
> -
> -	if (!ci ||
> -		(ci->ci_keyring_key &&
> -		 (ci->ci_keyring_key->flags & ((1 << KEY_FLAG_INVALIDATED) |
> -					       (1 << KEY_FLAG_REVOKED) |
> -					       (1 << KEY_FLAG_DEAD)))))
> -		return get_crypt_info(inode);
> -	return 0;
> -}
> -EXPORT_SYMBOL(fscrypt_get_encryption_info);
> diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
> index 76cff18..8e0bc584 100644
> --- a/include/linux/fscrypto.h
> +++ b/include/linux/fscrypto.h
> @@ -79,7 +79,6 @@ struct fscrypt_info {
>  	u8 ci_filename_mode;
>  	u8 ci_flags;
>  	struct crypto_skcipher *ci_ctfm;
> -	struct key *ci_keyring_key;
>  	u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
>  };
>  
> @@ -280,7 +279,6 @@ extern int fscrypt_has_permitted_context(struct inode *, struct inode *);
>  extern int fscrypt_inherit_context(struct inode *, struct inode *,
>  					void *, bool);
>  /* keyinfo.c */
> -extern int get_crypt_info(struct inode *);
>  extern int fscrypt_get_encryption_info(struct inode *);
>  extern void fscrypt_put_encryption_info(struct inode *, struct fscrypt_info *);
>  
> 

Looks OK to me, builds OK, not tested it though.

Acked-by: Colin Ian King <colin.king at canonical.com>



More information about the kernel-team mailing list