[PATCH] ext4: don't allow encrypted operations without keys

[PATCH] ext4: don't allow encrypted operations without keys

[Theodore Ts'o]

While we allow deletes without the key, the following should not be
permitted:

# cd /vdc/encrypted-dir-without-key
# 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
# mv uRJ5vJh9gE7vcomYMqTAyD  6,LKNRJsp209FbXoSvJWzB

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/namei.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index eadba919f26b..45a5ba558074 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3525,6 +3525,12 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry,
 			EXT4_I(old_dentry->d_inode)->i_projid)))
 		return -EXDEV;
 
+	if ((ext4_encrypted_inode(old_dir) &&
+	     !fscrypt_has_encryption_key(old_dir)) ||
+	    (ext4_encrypted_inode(new_dir) &&
+	     !fscrypt_has_encryption_key(new_dir)))
+		return -ENOKEY;
+
 	retval = dquot_initialize(old.dir);
 	if (retval)
 		return retval;
@@ -3725,6 +3731,12 @@ static int ext4_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 	int retval;
 	struct timespec ctime;
 
+	if ((ext4_encrypted_inode(old_dir) &&
+	     !fscrypt_has_encryption_key(old_dir)) ||
+	    (ext4_encrypted_inode(new_dir) &&
+	     !fscrypt_has_encryption_key(new_dir)))
+		return -ENOKEY;
+
 	if ((ext4_encrypted_inode(old_dir) ||
 	     ext4_encrypted_inode(new_dir)) &&
 	    (old_dir != new_dir) &&
-- 
2.11.0.rc0.7.gbe5a750

Re: [PATCH] ext4: don't allow encrypted operations without keys

[Eric Biggers]

Hi Ted,

On Wed, Dec 28, 2016 at 12:22:52AM -0500, Theodore Ts'o wrote:
> While we allow deletes without the key, the following should not be
> permitted:
> 
> # cd /vdc/encrypted-dir-without-key
> # 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
> # mv uRJ5vJh9gE7vcomYMqTAyD  6,LKNRJsp209FbXoSvJWzB
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  fs/ext4/namei.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index eadba919f26b..45a5ba558074 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3525,6 +3525,12 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry,
>  			EXT4_I(old_dentry->d_inode)->i_projid)))
>  		return -EXDEV;
>  
> +	if ((ext4_encrypted_inode(old_dir) &&
> +	     !fscrypt_has_encryption_key(old_dir)) ||
> +	    (ext4_encrypted_inode(new_dir) &&
> +	     !fscrypt_has_encryption_key(new_dir)))
> +		return -ENOKEY;
> +
>  	retval = dquot_initialize(old.dir);
>  	if (retval)
>  		return retval;
> @@ -3725,6 +3731,12 @@ static int ext4_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	int retval;
>  	struct timespec ctime;
>  
> +	if ((ext4_encrypted_inode(old_dir) &&
> +	     !fscrypt_has_encryption_key(old_dir)) ||
> +	    (ext4_encrypted_inode(new_dir) &&
> +	     !fscrypt_has_encryption_key(new_dir)))
> +		return -ENOKEY;
> +
>  	if ((ext4_encrypted_inode(old_dir) ||
>  	     ext4_encrypted_inode(new_dir)) &&
>  	    (old_dir != new_dir) &&

I'm fine with this, with the understanding that it relies on ext4_lookup()
calling fscrypt_get_encryption_info() (via fscrypt_has_permitted_context()) when
looking up the directory.  I also suggest moving the fscrypt_permitted_context()
check in ext4_rename() up to be next to the new check, so that the fscrypt hooks
are grouped together and are consistent with ext4_cross_rename().

I can also write/update an xfstest to test this.

Something I'm thinking about is making things easier for filesystems by having
functions like "fscrypt_rename_hook()" which would handle all these needed
checks.  It would be easy to do with out-of-line functions in fs/crypto/, but we
don't want to be making ->is_encrypted() calls through the fscrypt_operations
all the time, when an inlined call to ext4_encrypted_inode() (or f2fs or
ubifs_encrypted_inode()) is much faster.  I think it could be implemented as
efficiently as now if the hooks were defined in a header and called a macro like
"fs_encrypted_inode()" which filesystems would have to #define first.  It would
be a little ugly, but at least it would be less error-prone than having multiple
filesystems replicate these increasingly complex checks.

Eric

Re: [PATCH] ext4: don't allow encrypted operations without keys

[Theodore Ts'o]

On Thu, Jan 05, 2017 at 11:26:19AM -0800, Eric Biggers wrote:
> Something I'm thinking about is making things easier for filesystems by having
> functions like "fscrypt_rename_hook()" which would handle all these needed
> checks.  It would be easy to do with out-of-line functions in fs/crypto/, but we
> don't want to be making ->is_encrypted() calls through the fscrypt_operations
> all the time, when an inlined call to ext4_encrypted_inode() (or f2fs or
> ubifs_encrypted_inode()) is much faster.  I think it could be implemented as
> efficiently as now if the hooks were defined in a header and called a macro like
> "fs_encrypted_inode()" which filesystems would have to #define first.  It would
> be a little ugly, but at least it would be less error-prone than having multiple
> filesystems replicate these increasingly complex checks.

We could make things a lot simpler and a lot more efficient by adding
make flags at the VFS level in struct super and struct inode meaning
"file systems has fscrypt enabled", and "the inode is encrypted using
fscrypt".  That way, we wouldn't need to go through through
fscrypt_operations to do these tests.  We could just set the flags
appropriately when the struct inode or struct super is instantiated.

   	      	    	    		       	  - Ted

Re: [PATCH] ext4: don't allow encrypted operations without keys

[Eric Biggers]

On Wed, Dec 28, 2016 at 12:22:52AM -0500, Theodore Ts'o wrote:
> While we allow deletes without the key, the following should not be
> permitted:
> 
> # cd /vdc/encrypted-dir-without-key
> # 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
> # mv uRJ5vJh9gE7vcomYMqTAyD  6,LKNRJsp209FbXoSvJWzB
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Hi Ted, this commit shows up twice in the ext4 tree, as 173b8439e1ba3 and
a7ede371cb821, but the second actually adds the check two *more* times to
ext4_cross_rename(), such that there are now a total of three checks in that
function, all the same.  Do you want to revert the second, unnecessary, commit?

Thanks,

Eric

Re: [PATCH] ext4: don't allow encrypted operations without keys

[Theodore Ts'o]

On Sat, Feb 04, 2017 at 01:44:28PM -0800, Eric Biggers wrote:
> 
> Hi Ted, this commit shows up twice in the ext4 tree, as 173b8439e1ba3 and
> a7ede371cb821, but the second actually adds the check two *more* times to
> ext4_cross_rename(), such that there are now a total of three checks in that
> function, all the same.  Do you want to revert the second, unnecessary, commit?

Yeah, it (and a few other fixes) was on the dev branch, and then when
they were cherry picked to the stable branch, and then I rebased the
dev branch on top of 4.10-rc3 to pick up a few other commits, I forgot
drop it.

Thanks for pointing it out; I've dropped it.

						- Ted