On Tue, Feb 21, 2017 at 03:07:11PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebigg...@google.com>
>
> 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.
Removing the attempt at that functionality seems like the right
approach.
> 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.
I don't see any problem with this reasoning.
> 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.
Agreed.
> Fixes: b7236e21d55f ("ext4 crypto: reorganize how we store keys in the inode")
> Cc: sta...@vger.kernel.org # v4.2+
> Signed-off-by: Eric Biggers <ebigg...@google.com>
Acked-by: Michael Halcrow <mhalc...@google.com>
> ---
> fs/crypto/crypto.c | 10 +--------
> fs/crypto/fname.c | 2 +-
> fs/crypto/fscrypt_private.h | 4 ----
> fs/crypto/keyinfo.c | 52
> ++++++++-------------------------------------
> 4 files changed, 11 insertions(+), 57 deletions(-)
>
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index 02a7a9286449..6d6eca394d4d 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -327,7 +327,6 @@ EXPORT_SYMBOL(fscrypt_decrypt_page);
> 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)
> @@ -339,18 +338,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 13052b85c393..37b49894c762 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -350,7 +350,7 @@ int fscrypt_setup_filename(struct inode *dir, const
> struct qstr *iname,
> fname->disk_name.len = iname->len;
> return 0;
> }
> - ret = fscrypt_get_crypt_info(dir);
> + ret = fscrypt_get_encryption_info(dir);
> if (ret && ret != -EOPNOTSUPP)
> return ret;
>
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index fdbb8af32eaf..e39696e64494 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -67,7 +67,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];
> };
>
> @@ -101,7 +100,4 @@ extern int fscrypt_do_page_crypto(const struct inode
> *inode,
> extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
> gfp_t gfp_flags);
>
> -/* keyinfo.c */
> -extern int fscrypt_get_crypt_info(struct inode *);
> -
> #endif /* _FSCRYPT_PRIVATE_H */
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 02eb6b9e4438..cb3e82abf034 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -95,6 +95,7 @@ static int validate_user_key(struct fscrypt_info
> *crypt_info,
> kfree(description);
> 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
> @@ -102,11 +103,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;
> @@ -117,17 +116,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;
> }
> @@ -169,12 +162,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 fscrypt_get_crypt_info(struct inode *inode)
> +int fscrypt_get_encryption_info(struct inode *inode)
> {
> struct fscrypt_info *crypt_info;
> struct fscrypt_context ctx;
> @@ -184,21 +176,15 @@ int fscrypt_get_crypt_info(struct inode *inode)
> u8 *raw_key = NULL;
> int res;
>
> + if (inode->i_crypt_info)
> + return 0;
> +
> res = fscrypt_initialize(inode->i_sb->s_cop->flags);
> 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) {
> @@ -229,7 +215,6 @@ int fscrypt_get_crypt_info(struct inode *inode)
> 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));
>
> @@ -273,14 +258,8 @@ int fscrypt_get_crypt_info(struct inode *inode)
> if (res)
> goto out;
>
> - kzfree(raw_key);
> - raw_key = NULL;
> - 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)
> res = 0;
> @@ -288,6 +267,7 @@ int fscrypt_get_crypt_info(struct inode *inode)
> kzfree(raw_key);
> return res;
> }
> +EXPORT_SYMBOL(fscrypt_get_encryption_info);
>
> void fscrypt_put_encryption_info(struct inode *inode, struct fscrypt_info
> *ci)
> {
> @@ -305,17 +285,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 fscrypt_get_crypt_info(inode);
> - return 0;
> -}
> -EXPORT_SYMBOL(fscrypt_get_encryption_info);
> --
> 2.11.0.483.g087da7b7c-goog
>
