[PATCH 4.1 0/3] ext4 encryption fixes for 4.1-stable

[PATCH 4.1 0/3] ext4 encryption fixes for 4.1-stable

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Hi Sasha, can you please apply these backports of ext4 encryption fixes
to 4.1-stable?  They all have equivalent fixes in 4.4-stable.  Most
important is patch 1 which prevents unprivileged users from using (or
abusing) ext4 encryption when it hasn't been enabled on the filesystem
by a system administrator.  Patch 2 adds a missing permission check
(CVE-2016-10318), and patch 3 is a backport that Ted sent out some
months ago that seems to have been missed, for a bug in 4.1 that is very
similar to the bug in 4.2+ that was assigned CVE-2017-7374.

Note that ext4 encryption in 4.1 is still pretty broken and should not
be used (even just 4.4-stable is much better); these are just the most
important fixes that really ought to be in 4.1-stable.

Eric Biggers (1):
  fscrypto: add authorization check for setting encryption policy

Richard Weinberger (1):
  ext4: require encryption feature for EXT4_IOC_SET_ENCRYPTION_POLICY

Theodore Ts'o (1):
  ext4 crypto: don't regenerate the per-inode encryption key
    unnecessarily

 fs/ext4/crypto_fname.c  |  5 +++--
 fs/ext4/crypto_key.c    | 15 ++++++++++++---
 fs/ext4/crypto_policy.c |  3 +++
 fs/ext4/ext4.h          |  1 +
 fs/ext4/ioctl.c         |  3 +++
 fs/ext4/super.c         |  3 +++
 6 files changed, 25 insertions(+), 5 deletions(-)

-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 4.1 1/3] ext4: require encryption feature for EXT4_IOC_SET_ENCRYPTION_POLICY

[Eric Biggers]

From: Richard Weinberger <richard@nod.at>

commit 9a200d075e5d05be1fcad4547a0f8aee4e2f9a04 upstream.

...otherwise an user can enable encryption for certain files even
when the filesystem is unable to support it.
Such a case would be a filesystem created by mkfs.ext4's default
settings, 1KiB block size. Ext4 supports encyption only when block size
is equal to PAGE_SIZE.
But this constraint is only checked when the encryption feature flag
is set.

Signed-off-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 4196aa567784..dbe1ff511794 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -630,6 +630,9 @@ resizefs_out:
 		struct ext4_encryption_policy policy;
 		int err = 0;
 
+		if (!ext4_sb_has_crypto(sb))
+			return -EOPNOTSUPP;
+
 		if (copy_from_user(&policy,
 				   (struct ext4_encryption_policy __user *)arg,
 				   sizeof(policy))) {
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 4.1 2/3] fscrypto: add authorization check for setting encryption policy

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

commit 163ae1c6ad6299b19e22b4a35d5ab24a89791a98 upstream.

On an ext4 or f2fs filesystem with file encryption supported, a user
could set an encryption policy on any empty directory(*) to which they
had readonly access.  This is obviously problematic, since such a
directory might be owned by another user and the new encryption policy
would prevent that other user from creating files in their own directory
(for example).

Fix this by requiring inode_owner_or_capable() permission to set an
encryption policy.  This means that either the caller must own the file,
or the caller must have the capability CAP_FOWNER.

(*) Or also on any regular file, for f2fs v4.6 and later and ext4
    v4.8-rc1 and later; a separate bug fix is coming for that.

Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/crypto_policy.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ext4/crypto_policy.c b/fs/ext4/crypto_policy.c
index a6d6291aea16..591fc37dcd9e 100644
--- a/fs/ext4/crypto_policy.c
+++ b/fs/ext4/crypto_policy.c
@@ -85,6 +85,9 @@ static int ext4_create_encryption_context_from_policy(
 int ext4_process_policy(const struct ext4_encryption_policy *policy,
 			struct inode *inode)
 {
+	if (!inode_owner_or_capable(inode))
+		return -EACCES;
+
 	if (policy->version != 0)
 		return -EINVAL;
 
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH 4.1 3/3] ext4 crypto: don't regenerate the per-inode encryption key unnecessarily

[Eric Biggers]

From: Theodore Ts'o <tytso@mit.edu>

[ Relevant upstream commit: 1b53cf9815bb4744958d41f3795d5d5a1d365e2d ]

This fixes the same problem as upstream commit 1b53cf9815bb: "fscrypt:
remove broken support for detecting keyring key revocation".
Specifically, key revocations racing with readpage operations will
cause the kernel to crash and burn with a BUG_ON or a NULL pointer
dereference in a block I/O callback stemming from an ext4_readpage()
operation.

This fix is needed to fix prevent xfstests test runs from crashing
while running the generic/421 test.

The root cause is different in the 4.1 kernel, however, since the
4.1's encryption handling is so _primitive_ compared to later kernels.
The code isn't actually explicitly checking for revoked keys.
Instead, the code is neededly regenerating the per-file encryption key
on every mmap() or open() or directory operation (in the case of a
directory inode).  Yelch!

If the file is already opened and actively being read, and while an
open() is racing with the read operations, after the user's master key
has been revoked, there will be the same net effect as the problem
fixed by upstream commit 1b53cf9815bb --- the per-file key will be
marked as invalid and this will cause a BUG_ON.

The AOSP 3.18 and 4.4 android-common kernels have a much more modern
version of ext4 encryption have been backported, which incldes a
backport of upstream commit 1b53cf9815bb.  This is a (at least)
dozen-plus commits, and isn't really suitable for the Upstream LTS
kernel.  So instead, this is the simplest bug which fixes the same
high-level issue as the upstream commit, without dragging in all of
the other non-bug-fix improvements to ext4 encryption found in newer
kernels.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/crypto_fname.c |  5 +++--
 fs/ext4/crypto_key.c   | 15 ++++++++++++---
 fs/ext4/ext4.h         |  1 +
 fs/ext4/super.c        |  3 +++
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/crypto_fname.c b/fs/ext4/crypto_fname.c
index fded02f72299..b7a39a185d01 100644
--- a/fs/ext4/crypto_fname.c
+++ b/fs/ext4/crypto_fname.c
@@ -346,8 +346,9 @@ struct ext4_fname_crypto_ctx *ext4_get_fname_crypto_ctx(
 	if (res == 0)
 		return NULL;
 
-	if (!ext4_has_encryption_key(inode))
-		ext4_generate_encryption_key(inode);
+	res = ext4_generate_encryption_key(inode);
+	if (res)
+		return ERR_PTR(res);
 
 	/* Get a crypto context based on the key.
 	 * A new context is allocated if no context matches the requested key.
diff --git a/fs/ext4/crypto_key.c b/fs/ext4/crypto_key.c
index 52170d0b7c40..4f9818719d61 100644
--- a/fs/ext4/crypto_key.c
+++ b/fs/ext4/crypto_key.c
@@ -99,9 +99,17 @@ int ext4_generate_encryption_key(struct inode *inode)
 	struct ext4_encryption_context ctx;
 	struct user_key_payload *ukp;
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
-	int res = ext4_xattr_get(inode, EXT4_XATTR_INDEX_ENCRYPTION,
-				 EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
-				 &ctx, sizeof(ctx));
+	int res;
+
+	mutex_lock(&ei->i_encryption_lock);
+	if (ext4_has_encryption_key(inode)) {
+		mutex_unlock(&ei->i_encryption_lock);
+		return 0;
+	}
+
+	res = ext4_xattr_get(inode, EXT4_XATTR_INDEX_ENCRYPTION,
+			     EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
+			     &ctx, sizeof(ctx));
 
 	if (res != sizeof(ctx)) {
 		if (res > 0)
@@ -154,6 +162,7 @@ out:
 		key_put(keyring_key);
 	if (res < 0)
 		crypt_key->mode = EXT4_ENCRYPTION_MODE_INVALID;
+	mutex_unlock(&ei->i_encryption_lock);
 	return res;
 }
 
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index df67a6f8582a..01771ed4529d 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -989,6 +989,7 @@ struct ext4_inode_info {
 #ifdef CONFIG_EXT4_FS_ENCRYPTION
 	/* Encryption params */
 	struct ext4_encryption_key i_encryption_key;
+	struct mutex i_encryption_lock;
 #endif
 };
 
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index b29a7ef4953e..0787cb5d6e9b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -948,6 +948,9 @@ static void init_once(void *foo)
 	init_rwsem(&ei->xattr_sem);
 	init_rwsem(&ei->i_data_sem);
 	init_rwsem(&ei->i_mmap_sem);
+#ifdef CONFIG_EXT4_FS_ENCRYPTION
+	mutex_init(&ei->i_encryption_lock);
+#endif
 	inode_init_once(&ei->vfs_inode);
 }
 
-- 
2.16.2.395.g2e18187dfd-goog

Re: [PATCH 4.1 0/3] ext4 encryption fixes for 4.1-stable

[Sasha Levin]

On Fri, Mar 02, 2018 at 02:21:10PM -0800, Eric Biggers wrote:
>From: Eric Biggers <ebiggers@google.com>
>
>Hi Sasha, can you please apply these backports of ext4 encryption fixes
>to 4.1-stable?  They all have equivalent fixes in 4.4-stable.  Most
>important is patch 1 which prevents unprivileged users from using (or
>abusing) ext4 encryption when it hasn't been enabled on the filesystem
>by a system administrator.  Patch 2 adds a missing permission check
>(CVE-2016-10318), and patch 3 is a backport that Ted sent out some
>months ago that seems to have been missed, for a bug in 4.1 that is very
>similar to the bug in 4.2+ that was assigned CVE-2017-7374.
>
>Note that ext4 encryption in 4.1 is still pretty broken and should not
>be used (even just 4.4-stable is much better); these are just the most
>important fixes that really ought to be in 4.1-stable.
>
>Eric Biggers (1):
>  fscrypto: add authorization check for setting encryption policy
>
>Richard Weinberger (1):
>  ext4: require encryption feature for EXT4_IOC_SET_ENCRYPTION_POLICY
>
>Theodore Ts'o (1):
>  ext4 crypto: don't regenerate the per-inode encryption key
>    unnecessarily
>
> fs/ext4/crypto_fname.c  |  5 +++--
> fs/ext4/crypto_key.c    | 15 ++++++++++++---
> fs/ext4/crypto_policy.c |  3 +++
> fs/ext4/ext4.h          |  1 +
> fs/ext4/ioctl.c         |  3 +++
> fs/ext4/super.c         |  3 +++
> 6 files changed, 25 insertions(+), 5 deletions(-)
>
>-- 
>2.16.2.395.g2e18187dfd-goog
>

Applied, thank you!

-- 

Thanks,
Sasha