[PATCH 0/4] fscrypt context consistency check fixes

[PATCH 0/4] fscrypt context consistency check fixes

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

This patchset fixes fscrypt_has_permitted_context() to correctly handle cases
where one or both encryption keys are missing, then updates the ->lookup()
methods for ext4, f2fs, and ubifs to be consistent in how they handle the
encryption context consistency check.

Eric Biggers (4):
  fscrypt: fix context consistency check when key(s) unavailable
  ext4: remove "nokey" check from ext4_lookup()
  f2fs: sync f2fs_lookup() with ext4_lookup()
  ubifs: check for consistent encryption contexts in ubifs_lookup()

 fs/crypto/policy.c | 87 ++++++++++++++++++++++++++++++++++++++++++------------
 fs/ext4/namei.c    |  9 +-----
 fs/f2fs/namei.c    |  7 +++--
 fs/ubifs/dir.c     | 11 +++++++
 4 files changed, 84 insertions(+), 30 deletions(-)

-- 
2.12.2.715.g7642488e1d-goog

[PATCH 1/4] fscrypt: fix context consistency check when key(s) unavailable

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

To mitigate some types of offline attacks, filesystem encryption is
designed to enforce that all files in an encrypted directory tree use
the same encryption policy (i.e. the same encryption context excluding
the nonce).  However, the fscrypt_has_permitted_context() function which
enforces this relies on comparing struct fscrypt_info's, which are only
available when we have the encryption keys.  This can cause two
incorrect behaviors:

1. If we have the parent directory's key but not the child's key, or
   vice versa, then fscrypt_has_permitted_context() returned false,
   causing applications to see EPERM or ENOKEY.  This is incorrect if
   the encryption contexts are in fact consistent.  Although we'd
   normally have either both keys or neither key in that case since the
   master_key_descriptors would be the same, this is not guaranteed
   because keys can be added or removed from keyrings at any time.

2. If we have neither the parent's key nor the child's key, then
   fscrypt_has_permitted_context() returned true, causing applications
   to see no error (or else an error for some other reason).  This is
   incorrect if the encryption contexts are in fact inconsistent, since
   in that case we should deny access.

To fix this, retrieve and compare the fscrypt_contexts if we are unable
to set up both fscrypt_infos.

While this slightly hurts performance when accessing an encrypted
directory tree without the key, this isn't a case we really need to be
optimizing for; access *with* the key is much more important.
Furthermore, the performance hit is barely noticeable given that we are
already retrieving the fscrypt_context and doing two keyring searches in
fscrypt_get_encryption_info().  If we ever actually wanted to optimize
this case we might start by caching the fscrypt_contexts.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/policy.c | 87 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 68 insertions(+), 19 deletions(-)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 4908906d54d5..fc3660d82a7f 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -143,27 +143,61 @@ 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 directory?
+ *
+ * @parent: inode for parent directory
+ * @child: inode for file being looked up, opened, or linked into @parent
+ *
+ * Filesystems must call this before permitting access to an inode in a
+ * situation where the parent directory is encrypted (either before allowing
+ * ->lookup() to succeed, or for a regular file before allowing it to be opened)
+ * and before any operation that involves linking an inode 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 pre-access check is needed to detect potentially
+ * malicious offline violations of this constraint, while the link and rename
+ * 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 on file types which are never encrypted */
 	if (!S_ISREG(child->i_mode) && !S_ISDIR(child->i_mode) &&
 	    !S_ISLNK(child->i_mode))
 		return 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 retrieve and compare the fscrypt_contexts.
+	 *
+	 * Note that the fscrypt_context retrieval will be required frequently
+	 * when accessing an encrypted directory tree without the key.
+	 * Performance-wise this is not a big deal because we already don't
+	 * really optimize for file access without the key (to the extent that
+	 * such access is even possible), given that any attempted access
+	 * already causes a fscrypt_context retrieval and keyring search.
+	 *
+	 * In any case, if an unexpected error occurs, fall back to "forbidden".
+	 */
+
 	res = fscrypt_get_encryption_info(parent);
 	if (res)
 		return 0;
@@ -172,17 +206,32 @@ 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;
+
+	res = cops->get_context(child, &child_ctx, sizeof(child_ctx));
+	if (res != sizeof(child_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));
+	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);
 
-- 
2.12.2.715.g7642488e1d-goog

Re: [1/4] fscrypt: fix context consistency check when key(s) unavailable

[Theodore Ts'o]

On Fri, Apr 07, 2017 at 10:58:37AM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> To mitigate some types of offline attacks, filesystem encryption is
> designed to enforce that all files in an encrypted directory tree use
> the same encryption policy (i.e. the same encryption context excluding
> the nonce).  However, the fscrypt_has_permitted_context() function which
> enforces this relies on comparing struct fscrypt_info's, which are only
> available when we have the encryption keys.  This can cause two
> incorrect behaviors:
> 
> 1. If we have the parent directory's key but not the child's key, or
>    vice versa, then fscrypt_has_permitted_context() returned false,
>    causing applications to see EPERM or ENOKEY.  This is incorrect if
>    the encryption contexts are in fact consistent.  Although we'd
>    normally have either both keys or neither key in that case since the
>    master_key_descriptors would be the same, this is not guaranteed
>    because keys can be added or removed from keyrings at any time.
> 
> 2. If we have neither the parent's key nor the child's key, then
>    fscrypt_has_permitted_context() returned true, causing applications
>    to see no error (or else an error for some other reason).  This is
>    incorrect if the encryption contexts are in fact inconsistent, since
>    in that case we should deny access.
> 
> To fix this, retrieve and compare the fscrypt_contexts if we are unable
> to set up both fscrypt_infos.
> 
> While this slightly hurts performance when accessing an encrypted
> directory tree without the key, this isn't a case we really need to be
> optimizing for; access *with* the key is much more important.
> Furthermore, the performance hit is barely noticeable given that we are
> already retrieving the fscrypt_context and doing two keyring searches in
> fscrypt_get_encryption_info().  If we ever actually wanted to optimize
> this case we might start by caching the fscrypt_contexts.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

					- Ted

Re: [1/4] fscrypt: fix context consistency check when key(s) unavailable

[Eric Biggers]

On Sun, Apr 30, 2017 at 02:17:25AM -0400, Theodore Ts'o wrote:
> On Fri, Apr 07, 2017 at 10:58:37AM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > To mitigate some types of offline attacks, filesystem encryption is
> > designed to enforce that all files in an encrypted directory tree use
> > the same encryption policy (i.e. the same encryption context excluding
> > the nonce).  However, the fscrypt_has_permitted_context() function which
> > enforces this relies on comparing struct fscrypt_info's, which are only
> > available when we have the encryption keys.  This can cause two
> > incorrect behaviors:
> > 
> > 1. If we have the parent directory's key but not the child's key, or
> >    vice versa, then fscrypt_has_permitted_context() returned false,
> >    causing applications to see EPERM or ENOKEY.  This is incorrect if
> >    the encryption contexts are in fact consistent.  Although we'd
> >    normally have either both keys or neither key in that case since the
> >    master_key_descriptors would be the same, this is not guaranteed
> >    because keys can be added or removed from keyrings at any time.
> > 
> > 2. If we have neither the parent's key nor the child's key, then
> >    fscrypt_has_permitted_context() returned true, causing applications
> >    to see no error (or else an error for some other reason).  This is
> >    incorrect if the encryption contexts are in fact inconsistent, since
> >    in that case we should deny access.
> > 
> > To fix this, retrieve and compare the fscrypt_contexts if we are unable
> > to set up both fscrypt_infos.
> > 
> > While this slightly hurts performance when accessing an encrypted
> > directory tree without the key, this isn't a case we really need to be
> > optimizing for; access *with* the key is much more important.
> > Furthermore, the performance hit is barely noticeable given that we are
> > already retrieving the fscrypt_context and doing two keyring searches in
> > fscrypt_get_encryption_info().  If we ever actually wanted to optimize
> > this case we might start by caching the fscrypt_contexts.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> 
> Thanks, applied.
> 
> 					- Ted

Ted, can you add Cc stable to this commit?  (Just this one; the others in the
series aren't as important.)  I think this fix is more important than I
originally thought, because it's actually pretty easy to end up in a situation
where a directory has its key but a file in it does not, due to the file's inode
having been evicted from the inode cache but not the directory's inode.  This
makes the file undeletable.

- Eric

Re: [1/4] fscrypt: fix context consistency check when key(s) unavailable

[Theodore Ts'o]

On Tue, May 02, 2017 at 10:36:01AM -0700, Eric Biggers wrote:
> Ted, can you add Cc stable to this commit?  (Just this one; the others in the
> series aren't as important.)  I think this fix is more important than I
> originally thought, because it's actually pretty easy to end up in a situation
> where a directory has its key but a file in it does not, due to the file's inode
> having been evicted from the inode cache but not the directory's inode.  This
> makes the file undeletable.

Done.  This meant I had to do a rewind on the fscrypt tree, so if
someone has built something on top of fscrypt since Sunday you may
need to take special precautions.  This was just a change to the
commit description so git rebase should take of the problem relatively
easily.

					- Ted

[PATCH 2/4] ext4: remove "nokey" check from ext4_lookup()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Now that fscrypt_has_permitted_context() correctly handles the case
where we have the key for the parent directory but not the child, we
don't need to try to work around this in ext4_lookup().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/namei.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 6ad612c576fc..600b37874038 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1616,16 +1616,9 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsi
 		if (!IS_ERR(inode) && ext4_encrypted_inode(dir) &&
 		    (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) &&
 		    !fscrypt_has_permitted_context(dir, inode)) {
-			int nokey = ext4_encrypted_inode(inode) &&
-				!fscrypt_has_encryption_key(inode);
-			if (nokey) {
-				iput(inode);
-				return ERR_PTR(-ENOKEY);
-			}
 			ext4_warning(inode->i_sb,
 				     "Inconsistent encryption contexts: %lu/%lu",
-				     (unsigned long) dir->i_ino,
-				     (unsigned long) inode->i_ino);
+				     dir->i_ino, inode->i_ino);
 			iput(inode);
 			return ERR_PTR(-EPERM);
 		}
-- 
2.12.2.715.g7642488e1d-goog

Re: [2/4] ext4: remove "nokey" check from ext4_lookup()

[Theodore Ts'o]

On Fri, Apr 07, 2017 at 10:58:38AM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Now that fscrypt_has_permitted_context() correctly handles the case
> where we have the key for the parent directory but not the child, we
> don't need to try to work around this in ext4_lookup().
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

					- Ted

[PATCH 3/4] f2fs: sync f2fs_lookup() with ext4_lookup()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

As for ext4, now that fscrypt_has_permitted_context() correctly handles
the case where we have the key for the parent directory but not the
child, f2fs_lookup() no longer has to work around it.  Also add the same
warning message that ext4 uses.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/namei.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 98f00a3a7f50..9a5b9fa55318 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -324,9 +324,10 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 	if (f2fs_encrypted_inode(dir) &&
 	    (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) &&
 	    !fscrypt_has_permitted_context(dir, inode)) {
-		bool nokey = f2fs_encrypted_inode(inode) &&
-			!fscrypt_has_encryption_key(inode);
-		err = nokey ? -ENOKEY : -EPERM;
+		f2fs_msg(inode->i_sb, KERN_WARNING,
+			 "Inconsistent encryption contexts: %lu/%lu",
+			 dir->i_ino, inode->i_ino);
+		err = -EPERM;
 		goto err_out;
 	}
 	return d_splice_alias(inode, dentry);
-- 
2.12.2.715.g7642488e1d-goog

Re: [3/4] f2fs: sync f2fs_lookup() with ext4_lookup()

[Theodore Ts'o]

On Fri, Apr 07, 2017 at 10:58:39AM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> As for ext4, now that fscrypt_has_permitted_context() correctly handles
> the case where we have the key for the parent directory but not the
> child, f2fs_lookup() no longer has to work around it.  Also add the same
> warning message that ext4 uses.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

					- Ted

[PATCH 4/4] ubifs: check for consistent encryption contexts in ubifs_lookup()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

As ext4 and f2fs do, ubifs should check for consistent encryption
contexts during ->lookup() in an encrypted directory.  This protects
certain users of filesystem encryption against certain types of offline
attacks.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ubifs/dir.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 30825d882aa9..bbe2b346a94f 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -285,6 +285,15 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 		goto out_dent;
 	}
 
+	if (ubifs_crypt_is_encrypted(dir) &&
+	    (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) &&
+	    !fscrypt_has_permitted_context(dir, inode)) {
+		ubifs_warn(c, "Inconsistent encryption contexts: %lu/%lu",
+			   dir->i_ino, inode->i_ino);
+		err = -EPERM;
+		goto out_inode;
+	}
+
 done:
 	kfree(dent);
 	fscrypt_free_filename(&nm);
@@ -295,6 +304,8 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 	d_add(dentry, inode);
 	return NULL;
 
+out_inode:
+	iput(inode);
 out_dent:
 	kfree(dent);
 out_fname:
-- 
2.12.2.715.g7642488e1d-goog

Re: [4/4] ubifs: check for consistent encryption contexts in ubifs_lookup()

[Theodore Ts'o]

On Fri, Apr 07, 2017 at 10:58:40AM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> As ext4 and f2fs do, ubifs should check for consistent encryption
> contexts during ->lookup() in an encrypted directory.  This protects
> certain users of filesystem encryption against certain types of offline
> attacks.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

						- Ted