fscrypt: prevent creating duplicate encrypted filenames

PATCH In review Eric Biggers 1 participant 7 messages Nov 18, 2020 – Nov 24, 2020

Subsystems (7)

Stable branch VFS Memory technology devices (MTD) ext4 file system +3 more subsystems

lore

Share & tools

lore

Eric Biggers

author maintainer

2020-11-18 07:57 UTC #0 lore raw

cover fscrypt: prevent creating duplicate encrypted filenames

This series fixes a longstanding race condition where a duplicate filename can be created in an encrypted directory if a syscall that creates a new filename (e.g. open() or mkdir()) races with the directory's encryption key being added.

To close this race, we need to prevent creating files if the dentry is still marked as a no-key name. I.e. we need to fail the ->create() (or other operation that creates a new filename) if the key wasn't available when doing the dentry lookup earlier in the syscall, even if the key was concurrently added between the dentry lookup and ->create().

See patch 1 for a more detailed explanation.

Patch 1 introduces a helper function required for the fix. Patches 2-4 fix the bug on ext4, f2fs, and ubifs. Patch 5 is a cleanup.

This fixes xfstest generic/595 on ubifs, but that test was hitting this bug only accidentally. I've also written a new xfstest which reproduces this bug on both ext4 and ubifs.

Eric Biggers (5):
fscrypt: add fscrypt_is_nokey_name()
ext4: prevent creating duplicate encrypted filenames
f2fs: prevent creating duplicate encrypted filenames
ubifs: prevent creating duplicate encrypted filenames
fscrypt: remove unnecessary calls to fscrypt_require_key()

 fs/crypto/hooks.c       | 31 +++++++++++--------------------
 fs/ext4/namei.c         |  3 +++
 fs/f2fs/f2fs.h          |  2 ++
 fs/ubifs/dir.c          | 17 +++++++++++++----
 include/linux/fscrypt.h | 37 +++++++++++++++++++++++++++++++++++--
 5 files changed, 64 insertions(+), 26 deletions(-)

base-commit: 3ceb6543e9cf6ed87cc1fbc6f23ca2db903564cd

Signature · 1 line

2.29.2

Eric Biggers

author maintainer

2020-11-18 07:57 UTC #1 lore raw

PATCH 1/5 fscrypt: add fscrypt_is_nokey_name()

From: Eric Biggers <[email protected]>

It's possible to create a duplicate filename in an encrypted directory by creating a file concurrently with adding the encryption key.

Specifically, sys_open(O_CREAT) (or sys_mkdir(), sys_mknod(), or sys_symlink()) can lookup the target filename while the directory's encryption key hasn't been added yet, resulting in a negative no-key dentry. The VFS then calls ->create() (or ->mkdir(), ->mknod(), or ->symlink()) because the dentry is negative. Normally, ->create() would return -ENOKEY due to the directory's key being unavailable. However, if the key was added between the dentry lookup and ->create(), then the filesystem will go ahead and try to create the file.

If the target filename happens to already exist as a normal name (not a no-key name), a duplicate filename may be added to the directory.

In order to fix this, we need to fix the filesystems to prevent ->create(), ->mkdir(), ->mknod(), and ->symlink() on no-key names. (->rename() and ->link() need it too, but those are already handled correctly by fscrypt_prepare_rename() and fscrypt_prepare_link().)

In preparation for this, add a helper function fscrypt_is_nokey_name() that filesystems can use to do this check. Use this helper function for the existing checks that fs/crypto/ does for rename and link.

Cc [email protected]

Signed-off-by Eric Biggers <[email protected]>

---
 fs/crypto/hooks.c       |  5 +++--
 include/linux/fscrypt.h | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 2 deletions(-)

fs/crypto/hooks.c +3 -2

index 20b0df47fe6a..061418be4b08 100644
@@ -61,7 +61,7 @@ int __fscrypt_prepare_link(struct inode *inode, struct inode *dir,
 		return err;
 
 	/* ... in case we looked up no-key name before key was added */
-	if (dentry->d_flags & DCACHE_NOKEY_NAME)
+	if (fscrypt_is_nokey_name(dentry))
 		return -ENOKEY;
 
 	if (!fscrypt_has_permitted_context(dir, inode))

@@ -86,7 +86,8 @@ int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry,
 		return err;
 
 	/* ... in case we looked up no-key name(s) before key was added */
-	if ((old_dentry->d_flags | new_dentry->d_flags) & DCACHE_NOKEY_NAME)
+	if (fscrypt_is_nokey_name(old_dentry) ||
+	    fscrypt_is_nokey_name(new_dentry))
 		return -ENOKEY;
 
 	if (old_dir != new_dir) {

include/linux/fscrypt.h +34 -0

index a8f7a43f031b..8e1d31c959bf 100644
@@ -111,6 +111,35 @@ static inline void fscrypt_handle_d_move(struct dentry *dentry)
 	dentry->d_flags &= ~DCACHE_NOKEY_NAME;
 }
 
+/**
+ * fscrypt_is_nokey_name() - test whether a dentry is a no-key name
+ * @dentry: the dentry to check
+ *
+ * This returns true if the dentry is a no-key dentry.  A no-key dentry is a
+ * dentry that was created in an encrypted directory that hasn't had its
+ * encryption key added yet.  Such dentries may be either positive or negative.
+ *
+ * When a filesystem is asked to create a new filename in an encrypted directory
+ * and the new filename's dentry is a no-key dentry, it must fail the operation
+ * with ENOKEY.  This includes ->create(), ->mkdir(), ->mknod(), ->symlink(),
+ * ->rename(), and ->link().  (However, ->rename() and ->link() are already
+ * handled by fscrypt_prepare_rename() and fscrypt_prepare_link().)
+ *
+ * This is necessary because creating a filename requires the directory's
+ * encryption key, but just checking for the key on the directory inode during
+ * the final filesystem operation doesn't guarantee that the key was available
+ * during the preceding dentry lookup.  And the key must have already been
+ * available during the dentry lookup in order for it to have been checked
+ * whether the filename already exists in the directory and for the new file's
+ * dentry not to be invalidated due to it incorrectly having the no-key flag.
+ *
+ * Return: %true if the dentry is a no-key name
+ */
+static inline bool fscrypt_is_nokey_name(const struct dentry *dentry)
+{
+	return dentry->d_flags & DCACHE_NOKEY_NAME;
+}
+
 /* crypto.c */
 void fscrypt_enqueue_decrypt_work(struct work_struct *);

@@ -244,6 +273,11 @@ static inline void fscrypt_handle_d_move(struct dentry *dentry)
 {
 }
 
+static inline bool fscrypt_is_nokey_name(const struct dentry *dentry)
+{
+	return false;
+}
+
 /* crypto.c */
 static inline void fscrypt_enqueue_decrypt_work(struct work_struct *work)
 {

Signature · 1 line

2.29.2

Eric Biggers

author maintainer

2020-11-18 07:57 UTC #2 lore raw

PATCH 2/5 ext4: prevent creating duplicate encrypted filenames

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

As described in "fscrypt: add fscrypt_is_nokey_name()", it's possible to create a duplicate filename in an encrypted directory by creating a file concurrently with adding the directory's encryption key.

Fix this bug on ext4 by rejecting no-key dentries in ext4_add_entry().

Note that the duplicate check in ext4_find_dest_de() sometimes prevented this bug. However in many cases it didn't, since ext4_find_dest_de() doesn't examine every dentry.

Fixes 4461471107b7 ("ext4 crypto: enable filename encryption")

Cc [email protected]

Signed-off-by Eric Biggers <[email protected]>

---
 fs/ext4/namei.c | 3 +++
 1 file changed, 3 insertions(+)

fs/ext4/namei.c +3 -0

index 33509266f5a0..793fc7db9d28 100644
@@ -2195,6 +2195,9 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
 	if (!dentry->d_name.len)
 		return -EINVAL;
 
+	if (fscrypt_is_nokey_name(dentry))
+		return -ENOKEY;
+
 #ifdef CONFIG_UNICODE
 	if (sb_has_strict_encoding(sb) && IS_CASEFOLDED(dir) &&
 	    sb->s_encoding && utf8_validate(sb->s_encoding, &dentry->d_name))

Signature · 1 line

2.29.2

Eric Biggers

author maintainer

2020-11-18 07:57 UTC #3 lore raw

PATCH 3/5 f2fs: prevent creating duplicate encrypted filenames

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Fix this bug on f2fs by rejecting no-key dentries in f2fs_add_link().

Note that the weird check for the current task in f2fs_do_add_link() seems to make this bug difficult to reproduce on f2fs.

Fixes 9ea97163c6da ("f2fs crypto: add filename encryption for f2fs_add_link")

Cc [email protected]

Signed-off-by Eric Biggers <[email protected]>

---
 fs/f2fs/f2fs.h | 2 ++
 1 file changed, 2 insertions(+)

fs/f2fs/f2fs.h +2 -0

index cb700d797296..9a321c52face 100644
@@ -3251,6 +3251,8 @@ bool f2fs_empty_dir(struct inode *dir);
 
 static inline int f2fs_add_link(struct dentry *dentry, struct inode *inode)
 {
+	if (fscrypt_is_nokey_name(dentry))
+		return -ENOKEY;
 	return f2fs_do_add_link(d_inode(dentry->d_parent), &dentry->d_name,
 				inode, inode->i_ino, inode->i_mode);
 }

Signature · 1 line

2.29.2

Eric Biggers

author maintainer

2020-11-18 07:57 UTC #4 lore raw

PATCH 4/5 ubifs: prevent creating duplicate encrypted filenames

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Fix this bug on ubifs by rejecting no-key dentries in ubifs_create(), ubifs_mkdir(), ubifs_mknod(), and ubifs_symlink().

Note that ubifs doesn't actually report the duplicate filenames from readdir, but rather it seems to replace the original dentry with a new one (which is still wrong, just a different effect from ext4).

On ubifs, this fixes xfstest generic/595 as well as the new xfstest I wrote specifically for this bug.

Fixes f4f61d2cc6d8 ("ubifs: Implement encrypted filenames")

Cc [email protected]

Signed-off-by Eric Biggers <[email protected]>

---
 fs/ubifs/dir.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

fs/ubifs/dir.c +13 -4

index 155521e51ac5..08fde777c324 100644
@@ -270,6 +270,15 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 	return d_splice_alias(inode, dentry);
 }
 
+static int ubifs_prepare_create(struct inode *dir, struct dentry *dentry,
+				struct fscrypt_name *nm)
+{
+	if (fscrypt_is_nokey_name(dentry))
+		return -ENOKEY;
+
+	return fscrypt_setup_filename(dir, &dentry->d_name, 0, nm);
+}
+
 static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 			bool excl)
 {

@@ -293,7 +302,7 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 	if (err)
 		return err;
 
-	err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &nm);
+	err = ubifs_prepare_create(dir, dentry, &nm);
 	if (err)
 		goto out_budg;

@@ -953,7 +962,7 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 	if (err)
 		return err;
 
-	err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &nm);
+	err = ubifs_prepare_create(dir, dentry, &nm);
 	if (err)
 		goto out_budg;

@@ -1038,7 +1047,7 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
 		return err;
 	}
 
-	err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &nm);
+	err = ubifs_prepare_create(dir, dentry, &nm);
 	if (err) {
 		kfree(dev);
 		goto out_budg;

@@ -1122,7 +1131,7 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry,
 	if (err)
 		return err;
 
-	err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &nm);
+	err = ubifs_prepare_create(dir, dentry, &nm);
 	if (err)
 		goto out_budg;

Signature · 1 line

2.29.2

Eric Biggers

author maintainer

2020-11-18 07:57 UTC #5 lore raw

PATCH 5/5 fscrypt: remove unnecessary calls to fscrypt_require_key()

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

In an encrypted directory, a regular dentry (one that doesn't have the no-key name flag) can only be created if the directory's encryption key is available.

Therefore the calls to fscrypt_require_key() in __fscrypt_prepare_link() and __fscrypt_prepare_rename() are unnecessary, as these functions already check that the dentries they're given aren't no-key names.

Remove these unnecessary calls to fscrypt_require_key().

Signed-off-by Eric Biggers <[email protected]>

---
 fs/crypto/hooks.c       | 26 ++++++++------------------
 include/linux/fscrypt.h |  3 +--
 2 files changed, 9 insertions(+), 20 deletions(-)

fs/crypto/hooks.c +8 ~~-18~~

index 061418be4b08..c582e2ddb39c 100644
@@ -54,15 +54,12 @@ EXPORT_SYMBOL_GPL(fscrypt_file_open);
 int __fscrypt_prepare_link(struct inode *inode, struct inode *dir,
 			   struct dentry *dentry)
 {
-	int err;
-
-	err = fscrypt_require_key(dir);
-	if (err)
-		return err;
-
-	/* ... in case we looked up no-key name before key was added */
 	if (fscrypt_is_nokey_name(dentry))
 		return -ENOKEY;
+	/*
+	 * We don't need to separately check that the directory inode's key is
+	 * available, as it's implied by the dentry not being a no-key name.
+	 */
 
 	if (!fscrypt_has_permitted_context(dir, inode))
 		return -EXDEV;

@@ -75,20 +72,13 @@ int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry,
 			     struct inode *new_dir, struct dentry *new_dentry,
 			     unsigned int flags)
 {
-	int err;
-
-	err = fscrypt_require_key(old_dir);
-	if (err)
-		return err;
-
-	err = fscrypt_require_key(new_dir);
-	if (err)
-		return err;
-
-	/* ... in case we looked up no-key name(s) before key was added */
 	if (fscrypt_is_nokey_name(old_dentry) ||
 	    fscrypt_is_nokey_name(new_dentry))
 		return -ENOKEY;
+	/*
+	 * We don't need to separately check that the directory inodes' keys are
+	 * available, as it's implied by the dentries not being no-key names.
+	 */
 
 	if (old_dir != new_dir) {
 		if (IS_ENCRYPTED(new_dir) &&

include/linux/fscrypt.h +1 -2

index 8e1d31c959bf..0c9e64969b73 100644
@@ -710,8 +710,7 @@ static inline int fscrypt_require_key(struct inode *inode)
  *
  * A new link can only be added to an encrypted directory if the directory's
  * encryption key is available --- since otherwise we'd have no way to encrypt
- * the filename.  Therefore, we first set up the directory's encryption key (if
- * not already done) and return an error if it's unavailable.
+ * the filename.
  *
  * We also verify that the link will not violate the constraint that all files
  * in an encrypted directory tree use the same encryption policy.

Signature · 1 line

2.29.2

Eric Biggers

author maintainer

2020-11-24 23:28 UTC #6 lore raw

In reply to Eric Biggers

Quoted 35 lines · Eric Biggers

This series fixes a longstanding race condition where a duplicate filename can be created in an encrypted directory if a syscall that creates a new filename (e.g. open() or mkdir()) races with the directory's encryption key being added.
To close this race, we need to prevent creating files if the dentry is still marked as a no-key name. I.e. we need to fail the ->create() (or other operation that creates a new filename) if the key wasn't available when doing the dentry lookup earlier in the syscall, even if the key was concurrently added between the dentry lookup and ->create().
See patch 1 for a more detailed explanation.
Patch 1 introduces a helper function required for the fix. Patches 2-4 fix the bug on ext4, f2fs, and ubifs. Patch 5 is a cleanup.
This fixes xfstest generic/595 on ubifs, but that test was hitting this bug only accidentally. I've also written a new xfstest which reproduces this bug on both ext4 and ubifs.
Eric Biggers (5):
fscrypt: add fscrypt_is_nokey_name()
ext4: prevent creating duplicate encrypted filenames
f2fs: prevent creating duplicate encrypted filenames
ubifs: prevent creating duplicate encrypted filenames
fscrypt: remove unnecessary calls to fscrypt_require_key()
 fs/crypto/hooks.c       | 31 +++++++++++--------------------
 fs/ext4/namei.c         |  3 +++
 fs/f2fs/f2fs.h          |  2 ++
 fs/ubifs/dir.c          | 17 +++++++++++++----
 include/linux/fscrypt.h | 37 +++++++++++++++++++++++++++++++++++--
 5 files changed, 64 insertions(+), 26 deletions(-)

Quoted 1 line

base-commit: 3ceb6543e9cf6ed87cc1fbc6f23ca2db903564cd

All applied to fscrypt.git#master for 5.11.

I'd still appreciate acks for ext4, f2fs, and ubifs though.

- Eric