lkml.org 
[lkml]   [2017]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.10 082/111] fscrypt: remove broken support for detecting keyring key revocation
    Date
    4.10-stable review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Eric Biggers <ebiggers@google.com>

    commit 1b53cf9815bb4744958d41f3795d5d5a1d365e2d upstream.

    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")
    Signed-off-by: Eric Biggers <ebiggers@google.com>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>
    Acked-by: Michael Halcrow <mhalcrow@google.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

    ---
    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(-)

    --- a/fs/crypto/crypto.c
    +++ b/fs/crypto/crypto.c
    @@ -394,7 +394,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)
    @@ -406,18 +405,11 @@ static int fscrypt_d_revalidate(struct d
    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);

    /*
    --- a/fs/crypto/fname.c
    +++ b/fs/crypto/fname.c
    @@ -350,7 +350,7 @@ int fscrypt_setup_filename(struct inode
    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;

    --- 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];
    };

    @@ -87,7 +86,4 @@ struct fscrypt_completion_result {
    /* crypto.c */
    int fscrypt_initialize(unsigned int cop_flags);

    -/* keyinfo.c */
    -extern int fscrypt_get_crypt_info(struct inode *);
    -
    #endif /* _FSCRYPT_PRIVATE_H */
    --- a/fs/crypto/keyinfo.c
    +++ b/fs/crypto/keyinfo.c
    @@ -99,6 +99,7 @@ static int validate_user_key(struct fscr
    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
    @@ -106,11 +107,9 @@ static int validate_user_key(struct fscr
    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;
    @@ -121,17 +120,11 @@ static int validate_user_key(struct fscr
    "%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;
    }
    @@ -173,12 +166,11 @@ static void put_crypt_info(struct fscryp
    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;
    @@ -188,21 +180,15 @@ int fscrypt_get_crypt_info(struct 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) {
    @@ -230,7 +216,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));

    @@ -286,14 +271,8 @@ got_key:
    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;
    @@ -301,6 +280,7 @@ out:
    kzfree(raw_key);
    return res;
    }
    +EXPORT_SYMBOL(fscrypt_get_encryption_info);

    void fscrypt_put_encryption_info(struct inode *inode, struct fscrypt_info *ci)
    {
    @@ -318,17 +298,3 @@ void fscrypt_put_encryption_info(struct
    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);

    \
     
     \ /
      Last update: 2017-03-28 14:42    [W:2.743 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site