| Message ID | 20161228052252.10314-1-tytso@mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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
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
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) &&
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(+)