| Message ID | 20170221230711.85222-1-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Feb 21, 2017 at 03:07:11PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@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: stable@vger.kernel.org # v4.2+ > Signed-off-by: Eric Biggers <ebiggers@google.com> Acked-by: Michael Halcrow <mhalcrow@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 > -- To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 21, 2017 at 03:07:11PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@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. > > 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@vger.kernel.org # v4.2+ > Signed-off-by: Eric Biggers <ebiggers@google.com> Ted, can this be sent to Linus soon? This needs to be fixed as it's a security vulnerability on some systems. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 21, 2017 at 03:07:11PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@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. > > 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@vger.kernel.org # v4.2+ > Signed-off-by: Eric Biggers <ebiggers@google.com> Thanks, applied. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);