diff mbox

[v2,1/5] fscrypt: fix loophole in one-encryption-policy-per-tree enforcement

Message ID 1482186016-107643-1-git-send-email-ebiggers3@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Biggers Dec. 19, 2016, 10:20 p.m. UTC 
From: Eric Biggers <ebiggers@google.com>

Filesystem encryption is designed to enforce that all files in an
encrypted directory tree use the same encryption policy.  Operations
that violate this constraint are supposed to fail with EPERM.  There was
one case that was missed, however: the cross-rename operation (i.e.
renameat2 with RENAME_EXCHANGE) allowed two files with different
encryption policies to be exchanged, provided that neither encryption
key was available.

To fix this, when we can't compare the fscrypt_info structs because the
key is unavailable, compare the fscrypt_context structs instead.

This will be covered by a test in my encryption xfstests patchset.

Fixes: b7236e21d55f ("ext4 crypto: reorganize how we store keys in the inode")
Signed-off-by: Eric Biggers <ebiggers@google.com>
Reviewed-by: Richard Weinberger <richard@nod.at>
Cc: stable@vger.kernel.org
---
 fs/crypto/policy.c | 76 ++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 19 deletions(-)

Comments

Theodore Ts'o Dec. 28, 2016, 3:48 a.m. UTC | #1 
On Mon, Dec 19, 2016 at 02:20:12PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Filesystem encryption is designed to enforce that all files in an
> encrypted directory tree use the same encryption policy.  Operations
> that violate this constraint are supposed to fail with EPERM.  There was
> one case that was missed, however: the cross-rename operation (i.e.
> renameat2 with RENAME_EXCHANGE) allowed two files with different
> encryption policies to be exchanged, provided that neither encryption
> key was available.

I'm actually not sure this is the best way to address this issue.

What I think is better would be to forbid any renames if we are
missing the encryption key for the directory.  I had actually noticed
a problem here early when this worked:

root@kvm-xfstests:/# mount -o test_dummy_encryption /dev/vdc /vdc
root@kvm-xfstests:/# mkdir /vdc/test
root@kvm-xfstests:/# cp /etc/motd /vdc/test
root@kvm-xfstests:/# touch /vdc/test/empty
root@kvm-xfstests:/# umount /vdc
root@kvm-xfstests:/# mount /dev/vdc /vdc
root@kvm-xfstests:/# cd /vdc/test
root@kvm-xfstests:/vdc/test# ls -l
total 4
-rw-r--r-- 1 root root   0 Dec 27 22:35 6,LKNRJsp209FbXoSvJWzB
-rw-r--r-- 1 root root 286 Dec 27 22:35 uRJ5vJh9gE7vcomYMqTAyD
root@kvm-xfstests:/vdc/test# mv uRJ5vJh9gE7vcomYMqTAyD  6,LKNRJsp209FbXoSvJWzB
root@kvm-xfstests:/vdc/test# ls -l
total 4
-rw-r--r-- 1 root root 286 Dec 27 22:35 6,LKNRJsp209FbXoSvJWzB
root@kvm-xfstests:/vdc/test#

While there's no cryptographic reason why this can't be done
(obviously), and while a bad guy can always do something like this via
debugfs, I can't see any legitimate reason why we should allow this to
work.

It's one thing to allow a process without the encryption key
(generally with root privileges) to delete a file, but to rename two
files in an encrypted directory?  Or as you pointed out, allowing an
exchange of two files via RENAME_EXCHANGE?  Even if encryption
policies match, I don't think we should be allowing this at all.

	 	 	  	      	 - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 6ed7c2e..5de0633 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -169,22 +169,46 @@  int fscrypt_ioctl_get_policy(struct file *filp, void __user *arg)
 }
 EXPORT_SYMBOL(fscrypt_ioctl_get_policy);
 
+/**
+ * fscrypt_has_permitted_context() - is a file's encryption policy permitted
+ *				     within its parent directory?
+ *
+ * @parent: inode for parent directory
+ * @child: inode for file being looked up or linked into the directory
+ *
+ * Filesystems must call this function during lookup in an encrypted directory
+ * and before any operation that involves linking a file into an encrypted
+ * directory, including link, rename, and cross rename.  It enforces the
+ * constraint that within a given encrypted directory tree, all files use the
+ * same encryption policy.  The lookup check is needed to detect offline
+ * violations of this constraint, whereas the other checks are needed to prevent
+ * online violations of this constraint.
+ *
+ * Return: 1 if permitted, 0 if forbidden.  If forbidden, the caller must fail
+ * the filesystem operation with EPERM.
+ */
 int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 {
-	struct fscrypt_info *parent_ci, *child_ci;
+	const struct fscrypt_operations *cops = parent->i_sb->s_cop;
+	const struct fscrypt_info *parent_ci, *child_ci;
+	struct fscrypt_context parent_ctx, child_ctx;
 	int res;
 
-	if ((parent == NULL) || (child == NULL)) {
-		printk(KERN_ERR	"parent %p child %p\n", parent, child);
-		BUG_ON(1);
-	}
-
-	/* no restrictions if the parent directory is not encrypted */
-	if (!parent->i_sb->s_cop->is_encrypted(parent))
+	/* No restrictions if the parent directory is unencrypted */
+	if (!cops->is_encrypted(parent))
 		return 1;
-	/* if the child directory is not encrypted, this is always a problem */
-	if (!parent->i_sb->s_cop->is_encrypted(child))
+
+	/* Encrypted directories must not contain unencrypted files */
+	if (!cops->is_encrypted(child))
 		return 0;
+
+	/*
+	 * Both parent and child are encrypted, so verify they use the same
+	 * encryption policy.  Compare the fscrypt_info structs if the keys are
+	 * available, otherwise compare the fscrypt_context structs directly.
+	 * In any case, if an unexpected error occurs, fall back to "forbidden".
+	 */
+
 	res = fscrypt_get_encryption_info(parent);
 	if (res)
 		return 0;
@@ -193,17 +217,31 @@  int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 		return 0;
 	parent_ci = parent->i_crypt_info;
 	child_ci = child->i_crypt_info;
-	if (!parent_ci && !child_ci)
-		return 1;
-	if (!parent_ci || !child_ci)
+	if (parent_ci && child_ci) {
+		return memcmp(parent_ci->ci_master_key, child_ci->ci_master_key,
+			      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
+			(parent_ci->ci_data_mode == child_ci->ci_data_mode) &&
+			(parent_ci->ci_filename_mode ==
+			 child_ci->ci_filename_mode) &&
+			(parent_ci->ci_flags == child_ci->ci_flags);
+	}
+
+	res = cops->get_context(parent, &parent_ctx, sizeof(parent_ctx));
+	if (res != sizeof(parent_ctx))
 		return 0;
 
-	return (memcmp(parent_ci->ci_master_key,
-			child_ci->ci_master_key,
-			FS_KEY_DESCRIPTOR_SIZE) == 0 &&
-		(parent_ci->ci_data_mode == child_ci->ci_data_mode) &&
-		(parent_ci->ci_filename_mode == child_ci->ci_filename_mode) &&
-		(parent_ci->ci_flags == child_ci->ci_flags));
+	res = cops->get_context(child, &child_ctx, sizeof(child_ctx));
+	if (res != sizeof(child_ctx))
+		return 0;
+
+	return memcmp(parent_ctx.master_key_descriptor,
+		      child_ctx.master_key_descriptor,
+		      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
+		(parent_ctx.contents_encryption_mode ==
+		 child_ctx.contents_encryption_mode) &&
+		(parent_ctx.filenames_encryption_mode ==
+		 child_ctx.filenames_encryption_mode) &&
+		(parent_ctx.flags == child_ctx.flags);
 }
 EXPORT_SYMBOL(fscrypt_has_permitted_context);