[PATCH 0/3] fscrypto: Return -EXDEV for link, rename, and cross-rename between incompat contexts

[PATCH 0/3] fscrypto: Return -EXDEV for link, rename, and cross-rename between incompat contexts

[Michael Halcrow]

Currently file systems support fscrypto will return -EPERM when the
user attempts to link, rename, or cross-rename between two directories
that have incompatible encryption policy contexts.  User space tools
will fail the operation when receiving this errno.  With -EXDEV, user
space tools will typically fall back to copy-and-delete instead.

Our original motivation for returning -EPERM was to force users to try
harder when doing these operations, hopefully making them think more
carefully about whether what they're doing is secure.  One security
concern is that when moving files between unencrypted locations into
encrypted locations, the data in the unencrypted location will remain
in the clear on the storage device until the freed blocks are
overwritten at some arbitrary point in the future (if ever).  Moving
files from encrypted locations into unencrypted locations is also
(perhaps more obviously) problematic.

Whether making things fail will have the intended effect on users is
up for debate.  Meanwhile I've had at least one person tell me their
userspace tools are failing and that they would prefer seeing the same
sort of behavior that they see when (for example) moving files from
one project quota hierarchy to another (ext4 returns -EXDEV).

Note that xfstests generic/398 will require an update with this
change.

Michael Halcrow (3):
  ext4 crypto: Return -EXDEV for link, rename, and cross-rename between
    incompat contexts
  F2FS crypto: Return -EXDEV for link, rename, and cross-rename between
    incompat contexts
  UBIFS crypto: Return -EXDEV for link, rename, and cross-rename between
    incompat contexts

 fs/ext4/namei.c | 6 +++---
 fs/f2fs/namei.c | 6 +++---
 fs/ubifs/dir.c  | 6 +++---
 3 files changed, 9 insertions(+), 9 deletions(-)

-- 
2.14.1.581.gf28d330327-goog

[PATCH 1/3] ext4 crypto: Return -EXDEV for link, rename, and cross-rename between incompat contexts

[Michael Halcrow]

Gives user space the opportunity to fall back to copy-and-delete.

Signed-off-by: Michael Halcrow <mhalcrow@google.com>
---
 fs/ext4/namei.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index c1cf020d1889..3c493bc4273d 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3223,7 +3223,7 @@ static int ext4_link(struct dentry *old_dentry,
 		return -EMLINK;
 	if (ext4_encrypted_inode(dir) &&
 			!fscrypt_has_permitted_context(dir, inode))
-		return -EPERM;
+		return -EXDEV;
 
        if ((ext4_test_inode_flag(dir, EXT4_INODE_PROJINHERIT)) &&
 	   (!projid_eq(EXT4_I(dir)->i_projid,
@@ -3552,7 +3552,7 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if ((old.dir != new.dir) &&
 	    ext4_encrypted_inode(new.dir) &&
 	    !fscrypt_has_permitted_context(new.dir, old.inode)) {
-		retval = -EPERM;
+		retval = -EXDEV;
 		goto end_rename;
 	}
 
@@ -3732,7 +3732,7 @@ static int ext4_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 	    (old_dir != new_dir) &&
 	    (!fscrypt_has_permitted_context(new_dir, old.inode) ||
 	     !fscrypt_has_permitted_context(old_dir, new.inode)))
-		return -EPERM;
+		return -EXDEV;
 
 	if ((ext4_test_inode_flag(new_dir, EXT4_INODE_PROJINHERIT) &&
 	     !projid_eq(EXT4_I(new_dir)->i_projid,
-- 
2.14.1.581.gf28d330327-goog

[PATCH 2/3] F2FS crypto: Return -EXDEV for link, rename, and cross-rename between incompat contexts

[Michael Halcrow]

Gives user space the opportunity to fall back to copy-and-delete.

Signed-off-by: Michael Halcrow <mhalcrow@google.com>
---
 fs/f2fs/namei.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 760d85223c81..689c671cf8b8 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -202,7 +202,7 @@ static int f2fs_link(struct dentry *old_dentry, struct inode *dir,
 
 	if (f2fs_encrypted_inode(dir) &&
 			!fscrypt_has_permitted_context(dir, inode))
-		return -EPERM;
+		return -EXDEV;
 
 	err = dquot_initialize(dir);
 	if (err)
@@ -720,7 +720,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	if ((old_dir != new_dir) && f2fs_encrypted_inode(new_dir) &&
 			!fscrypt_has_permitted_context(new_dir, old_inode)) {
-		err = -EPERM;
+		err = -EXDEV;
 		goto out;
 	}
 
@@ -910,7 +910,7 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 			(old_dir != new_dir) &&
 			(!fscrypt_has_permitted_context(new_dir, old_inode) ||
 			 !fscrypt_has_permitted_context(old_dir, new_inode)))
-		return -EPERM;
+		return -EXDEV;
 
 	err = dquot_initialize(old_dir);
 	if (err)
-- 
2.14.1.581.gf28d330327-goog

[PATCH 3/3] UBIFS crypto: Return -EXDEV for link, rename, and cross-rename between incompat contexts

[Michael Halcrow]

Gives user space the opportunity to fall back to copy-and-delete.

Signed-off-by: Michael Halcrow <mhalcrow@google.com>
---
 fs/ubifs/dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 417fe0b29f23..6c5753a2f79f 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -745,7 +745,7 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
 
 	if (ubifs_crypt_is_encrypted(dir) &&
 	    !fscrypt_has_permitted_context(dir, inode))
-		return -EPERM;
+		return -EXDEV;
 
 	err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &nm);
 	if (err)
@@ -1356,7 +1356,7 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (old_dir != new_dir) {
 		if (ubifs_crypt_is_encrypted(new_dir) &&
 		    !fscrypt_has_permitted_context(new_dir, old_inode))
-			return -EPERM;
+			return -EXDEV;
 	}
 
 	if (unlink && is_dir) {
@@ -1578,7 +1578,7 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
 	    (old_dir != new_dir) &&
 	    (!fscrypt_has_permitted_context(new_dir, fst_inode) ||
 	     !fscrypt_has_permitted_context(old_dir, snd_inode)))
-		return -EPERM;
+		return -EXDEV;
 
 	err = fscrypt_setup_filename(old_dir, &old_dentry->d_name, 0, &fst_nm);
 	if (err)
-- 
2.14.1.581.gf28d330327-goog

Re: [PATCH 0/3] fscrypto: Return -EXDEV for link, rename, and cross-rename between incompat contexts

[Eric Biggers]

Hi Michael,

On Thu, Sep 07, 2017 at 05:12:01PM -0700, Michael Halcrow wrote:
> Currently file systems support fscrypto will return -EPERM when the
> user attempts to link, rename, or cross-rename between two directories
> that have incompatible encryption policy contexts.  User space tools
> will fail the operation when receiving this errno.  With -EXDEV, user
> space tools will typically fall back to copy-and-delete instead.

Mention 'mv' as an example to make it more concrete?

> Our original motivation for returning -EPERM was to force users to try
> harder when doing these operations, hopefully making them think more
> carefully about whether what they're doing is secure.  One security
> concern is that when moving files between unencrypted locations into
> encrypted locations, the data in the unencrypted location will remain
> in the clear on the storage device until the freed blocks are
> overwritten at some arbitrary point in the future (if ever).  Moving
> files from encrypted locations into unencrypted locations is also
> (perhaps more obviously) problematic.
> 
> Whether making things fail will have the intended effect on users is
> up for debate.  Meanwhile I've had at least one person tell me their
> userspace tools are failing and that they would prefer seeing the same
> sort of behavior that they see when (for example) moving files from
> one project quota hierarchy to another (ext4 returns -EXDEV).

There are arguments in favor of this, but I'm worried that people will think
they can encrypt their files by "moving" them into an encrypted directory.  In
fact, the unencrypted data will still be on-disk in the free blocks.  When
people say they want 'mv' to work, do they actually understand that it usually
defeats the purpose of encryption?  Or are they naively assuming that deleting a
file means its data is gone forever?

In any case, if we actually do this we'd need to document that mv-ing files into
an encrypted directory is a bad idea, although it would be of limited use since
users rarely read documentation.

> Michael Halcrow (3):
>   ext4 crypto: Return -EXDEV for link, rename, and cross-rename between
>     incompat contexts
>   F2FS crypto: Return -EXDEV for link, rename, and cross-rename between
>     incompat contexts
>   UBIFS crypto: Return -EXDEV for link, rename, and cross-rename between
>     incompat contexts

Nits for if we do end up doing this: remove "cross-rename" from the subjects to
shorten them a bit (cross-rename is a type of rename), then add more detail to
the patch descriptions, even if that requires duplicating some text.  The cover
letter doesn't get recorded in the commit history, only the actual commits do.

Note: I'm also working on a patchset which adds helper functions
fscrypt_prepare_link() and fscrypt_prepare_rename().  After that, this error
will be chosen by the shared code, rather than individual filesystems.

Eric