* [PATCH] ecryptfs: Restore support for both encrypted and unencrypted file names @ 2018-02-13 22:36 Guenter Roeck 2018-02-28 19:27 ` Guenter Roeck 2018-03-27 15:58 ` [PATCH] " Tyler Hicks 0 siblings, 2 replies; 6+ messages in thread From: Guenter Roeck @ 2018-02-13 22:36 UTC (permalink / raw) To: Tyler Hicks; +Cc: ecryptfs, linux-kernel, Guenter Roeck, Al Viro Commit 88ae4ab9802e ("ecryptfs_lookup(): try either only encrypted or plaintext name") was supposed to fix a situation where two files with the same name and same inode could be created in ecryptfs. One of those files had an encrypted file name, the other file name was unencrypted. After commit 88ae4ab9802e, having a mix of encrypted and unencrypted file names is no longer supposed to be possible. However, that is not the case. The only difference is that it is now even easier to create a situation where two files with the same name coexist (one encrypted and the other not encrypted). In practice, this looks like the following (files created with v4.14.12). ecryptfs mounted with file name encryption enabled: $ ls -li total 48 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 $ grep . * myfile:encrypted myfile:encrypted myfile2:encrypted myfile2:encrypted $ ls -li total 48 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVun.BU8Zu5-njbcIPoApxk7-E-- 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVunt0fda7t9YCtJ70cm911yZ--- 5252817 -rw-rw-r-- 1 groeck groeck 12 Jan 20 12:52 myfile 5252827 -rw-rw-r-- 1 groeck groeck 12 Jan 20 15:37 myfile2 $ grep . * ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVun.BU8Zu5-njbcIPoApxk7-E--:encrypted ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVunt0fda7t9YCtJ70cm911yZ---:encrypted myfile:unencrypted myfile2:unencrypted Creating a file with file name encryption disabled and remounting with file name encryption enabled results in the following. $ ls -li ls: cannot access 'myfile3': No such file or directory total 48 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 ? -????????? ? ? ? ? ? myfile3 Prior to commit 88ae4ab9802e, the file system had to be mounted with encrypted file names first to create a file, then the same had to be repeated after mounting with unencrypted file names. Now the duplicate files can be created both ways (unencrypted _or_ encrypted first). The only real difference is that it is no longer possible to have a _working_ combination of encrypted and unencrypted file names. In other words, commit 88ae4ab9802e results in reduced functionality with no benefit whatsoever. Restore ability to have a mix of unencrypted and encrypted files. This effectively reverts commit 88ae4ab9802e, but the code is now better readable since it avoids a number of goto statements. Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- fs/ecryptfs/inode.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c index 847904aa63a9..14a5c096ead6 100644 --- a/fs/ecryptfs/inode.c +++ b/fs/ecryptfs/inode.c @@ -392,11 +392,11 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode, int rc = 0; lower_dir_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry->d_parent); - + lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len); mount_crypt_stat = &ecryptfs_superblock_to_private( ecryptfs_dentry->d_sb)->mount_crypt_stat; - if (mount_crypt_stat - && (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)) { + if (IS_ERR(lower_dentry) && + (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)) { rc = ecryptfs_encrypt_and_encode_filename( &encrypted_and_encoded_name, &len, mount_crypt_stat, name, len); @@ -405,10 +405,10 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode, "filename; rc = [%d]\n", __func__, rc); return ERR_PTR(rc); } - name = encrypted_and_encoded_name; + lower_dentry = lookup_one_len_unlocked( + encrypted_and_encoded_name, lower_dir_dentry, len); } - lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len); if (IS_ERR(lower_dentry)) { ecryptfs_printk(KERN_DEBUG, "%s: lookup_one_len() returned " "[%ld] on lower_dentry = [%s]\n", __func__, -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: ecryptfs: Restore support for both encrypted and unencrypted file names 2018-02-13 22:36 [PATCH] ecryptfs: Restore support for both encrypted and unencrypted file names Guenter Roeck @ 2018-02-28 19:27 ` Guenter Roeck 2018-03-27 15:58 ` [PATCH] " Tyler Hicks 1 sibling, 0 replies; 6+ messages in thread From: Guenter Roeck @ 2018-02-28 19:27 UTC (permalink / raw) To: Tyler Hicks; +Cc: ecryptfs, linux-kernel, Al Viro ping On Tue, Feb 13, 2018 at 02:36:08PM -0800, Guenter Roeck wrote: > Commit 88ae4ab9802e ("ecryptfs_lookup(): try either only encrypted or > plaintext name") was supposed to fix a situation where two files with > the same name and same inode could be created in ecryptfs. One of those > files had an encrypted file name, the other file name was unencrypted. > > After commit 88ae4ab9802e, having a mix of encrypted and unencrypted file > names is no longer supposed to be possible. However, that is not the case. > The only difference is that it is now even easier to create a situation > where two files with the same name coexist (one encrypted and the other > not encrypted). In practice, this looks like the following (files > created with v4.14.12). > > ecryptfs mounted with file name encryption enabled: > > $ ls -li > total 48 > 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile > 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile > 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 > 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 > $ grep . * > myfile:encrypted > myfile:encrypted > myfile2:encrypted > myfile2:encrypted > > $ ls -li > total 48 > 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 > ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVun.BU8Zu5-njbcIPoApxk7-E-- > 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 > ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVunt0fda7t9YCtJ70cm911yZ--- > 5252817 -rw-rw-r-- 1 groeck groeck 12 Jan 20 12:52 myfile > 5252827 -rw-rw-r-- 1 groeck groeck 12 Jan 20 15:37 myfile2 > > $ grep . * > ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVun.BU8Zu5-njbcIPoApxk7-E--:encrypted > ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVunt0fda7t9YCtJ70cm911yZ---:encrypted > myfile:unencrypted > myfile2:unencrypted > > Creating a file with file name encryption disabled and remounting with > file name encryption enabled results in the following. > > $ ls -li > ls: cannot access 'myfile3': No such file or directory > total 48 > 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile > 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile > 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 > 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 > ? -????????? ? ? ? ? ? myfile3 > > Prior to commit 88ae4ab9802e, the file system had to be mounted with > encrypted file names first to create a file, then the same had to be > repeated after mounting with unencrypted file names. Now the duplicate > files can be created both ways (unencrypted _or_ encrypted first). > > The only real difference is that it is no longer possible to have a > _working_ combination of encrypted and unencrypted file names. In other > words, commit 88ae4ab9802e results in reduced functionality with no > benefit whatsoever. > > Restore ability to have a mix of unencrypted and encrypted files. > This effectively reverts commit 88ae4ab9802e, but the code is now > better readable since it avoids a number of goto statements. > > Cc: Al Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > fs/ecryptfs/inode.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c > index 847904aa63a9..14a5c096ead6 100644 > --- a/fs/ecryptfs/inode.c > +++ b/fs/ecryptfs/inode.c > @@ -392,11 +392,11 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode, > int rc = 0; > > lower_dir_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry->d_parent); > - > + lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len); > mount_crypt_stat = &ecryptfs_superblock_to_private( > ecryptfs_dentry->d_sb)->mount_crypt_stat; > - if (mount_crypt_stat > - && (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)) { > + if (IS_ERR(lower_dentry) && > + (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)) { > rc = ecryptfs_encrypt_and_encode_filename( > &encrypted_and_encoded_name, &len, > mount_crypt_stat, name, len); > @@ -405,10 +405,10 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode, > "filename; rc = [%d]\n", __func__, rc); > return ERR_PTR(rc); > } > - name = encrypted_and_encoded_name; > + lower_dentry = lookup_one_len_unlocked( > + encrypted_and_encoded_name, lower_dir_dentry, len); > } > > - lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len); > if (IS_ERR(lower_dentry)) { > ecryptfs_printk(KERN_DEBUG, "%s: lookup_one_len() returned " > "[%ld] on lower_dentry = [%s]\n", __func__, ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ecryptfs: Restore support for both encrypted and unencrypted file names 2018-02-13 22:36 [PATCH] ecryptfs: Restore support for both encrypted and unencrypted file names Guenter Roeck 2018-02-28 19:27 ` Guenter Roeck @ 2018-03-27 15:58 ` Tyler Hicks 2018-03-28 13:33 ` Guenter Roeck 1 sibling, 1 reply; 6+ messages in thread From: Tyler Hicks @ 2018-03-27 15:58 UTC (permalink / raw) To: Guenter Roeck; +Cc: ecryptfs, linux-kernel, Al Viro [-- Attachment #1.1: Type: text/plain, Size: 5468 bytes --] Hello Guenter On 02/13/2018 04:36 PM, Guenter Roeck wrote: > Commit 88ae4ab9802e ("ecryptfs_lookup(): try either only encrypted or > plaintext name") was supposed to fix a situation where two files with > the same name and same inode could be created in ecryptfs. One of those > files had an encrypted file name, the other file name was unencrypted. That's correct. Al was concerned about possible deadlocks with aliased dentries and I thought it would be best to only support encrypted and unencrypted but not both. > > After commit 88ae4ab9802e, having a mix of encrypted and unencrypted file > names is no longer supposed to be possible. However, that is not the case. > The only difference is that it is now even easier to create a situation > where two files with the same name coexist (one encrypted and the other > not encrypted). In practice, this looks like the following (files > created with v4.14.12). > > ecryptfs mounted with file name encryption enabled: > > $ ls -li > total 48 > 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile > 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile > 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 > 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 > $ grep . * > myfile:encrypted > myfile:encrypted > myfile2:encrypted > myfile2:encrypted > > $ ls -li > total 48 > 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 > ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVun.BU8Zu5-njbcIPoApxk7-E-- > 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 > ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVunt0fda7t9YCtJ70cm911yZ--- > 5252817 -rw-rw-r-- 1 groeck groeck 12 Jan 20 12:52 myfile > 5252827 -rw-rw-r-- 1 groeck groeck 12 Jan 20 15:37 myfile2 > > $ grep . * > ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVun.BU8Zu5-njbcIPoApxk7-E--:encrypted > ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVunt0fda7t9YCtJ70cm911yZ---:encrypted > myfile:unencrypted > myfile2:unencrypted > > Creating a file with file name encryption disabled and remounting with > file name encryption enabled results in the following. > > $ ls -li > ls: cannot access 'myfile3': No such file or directory > total 48 > 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile > 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile > 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 > 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 > ? -????????? ? ? ? ? ? myfile3 > > Prior to commit 88ae4ab9802e, the file system had to be mounted with > encrypted file names first to create a file, then the same had to be > repeated after mounting with unencrypted file names. Now the duplicate > files can be created both ways (unencrypted _or_ encrypted first). > > The only real difference is that it is no longer possible to have a > _working_ combination of encrypted and unencrypted file names. In other > words, commit 88ae4ab9802e results in reduced functionality with no > benefit whatsoever. > > Restore ability to have a mix of unencrypted and encrypted files. > This effectively reverts commit 88ae4ab9802e, but the code is now > better readable since it avoids a number of goto statements. I'd like for us to correctly fix 88ae4ab9802e rather than try to support both filename types under a single mount since that's complex and there are unknown corner cases to consider. I think this can be done by not copying up the lower filename when an error is encountered in ecryptfs_decode_and_decrypt_filename(). If filename encryption is enabled, it should only return decrypted filenames or an error if it isn't possible to decrypt the lower filename. Tyler > > Cc: Al Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > fs/ecryptfs/inode.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c > index 847904aa63a9..14a5c096ead6 100644 > --- a/fs/ecryptfs/inode.c > +++ b/fs/ecryptfs/inode.c > @@ -392,11 +392,11 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode, > int rc = 0; > > lower_dir_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry->d_parent); > - > + lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len); > mount_crypt_stat = &ecryptfs_superblock_to_private( > ecryptfs_dentry->d_sb)->mount_crypt_stat; > - if (mount_crypt_stat > - && (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)) { > + if (IS_ERR(lower_dentry) && > + (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)) { > rc = ecryptfs_encrypt_and_encode_filename( > &encrypted_and_encoded_name, &len, > mount_crypt_stat, name, len); > @@ -405,10 +405,10 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode, > "filename; rc = [%d]\n", __func__, rc); > return ERR_PTR(rc); > } > - name = encrypted_and_encoded_name; > + lower_dentry = lookup_one_len_unlocked( > + encrypted_and_encoded_name, lower_dir_dentry, len); > } > > - lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len); > if (IS_ERR(lower_dentry)) { > ecryptfs_printk(KERN_DEBUG, "%s: lookup_one_len() returned " > "[%ld] on lower_dentry = [%s]\n", __func__, > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ecryptfs: Restore support for both encrypted and unencrypted file names 2018-03-27 15:58 ` [PATCH] " Tyler Hicks @ 2018-03-28 13:33 ` Guenter Roeck 2018-03-28 14:19 ` Tyler Hicks 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2018-03-28 13:33 UTC (permalink / raw) To: Tyler Hicks; +Cc: ecryptfs, linux-kernel, Al Viro On 03/27/2018 08:58 AM, Tyler Hicks wrote: > Hello Guenter > > On 02/13/2018 04:36 PM, Guenter Roeck wrote: >> Commit 88ae4ab9802e ("ecryptfs_lookup(): try either only encrypted or >> plaintext name") was supposed to fix a situation where two files with >> the same name and same inode could be created in ecryptfs. One of those >> files had an encrypted file name, the other file name was unencrypted. > > That's correct. Al was concerned about possible deadlocks with aliased > dentries and I thought it would be best to only support encrypted and > unencrypted but not both. > >> >> After commit 88ae4ab9802e, having a mix of encrypted and unencrypted file >> names is no longer supposed to be possible. However, that is not the case. >> The only difference is that it is now even easier to create a situation >> where two files with the same name coexist (one encrypted and the other >> not encrypted). In practice, this looks like the following (files >> created with v4.14.12). >> >> ecryptfs mounted with file name encryption enabled: >> >> $ ls -li >> total 48 >> 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile >> 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile >> 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 >> 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 >> $ grep . * >> myfile:encrypted >> myfile:encrypted >> myfile2:encrypted >> myfile2:encrypted >> >> $ ls -li >> total 48 >> 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 >> ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVun.BU8Zu5-njbcIPoApxk7-E-- >> 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 >> ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVunt0fda7t9YCtJ70cm911yZ--- >> 5252817 -rw-rw-r-- 1 groeck groeck 12 Jan 20 12:52 myfile >> 5252827 -rw-rw-r-- 1 groeck groeck 12 Jan 20 15:37 myfile2 >> >> $ grep . * >> ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVun.BU8Zu5-njbcIPoApxk7-E--:encrypted >> ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVunt0fda7t9YCtJ70cm911yZ---:encrypted >> myfile:unencrypted >> myfile2:unencrypted >> >> Creating a file with file name encryption disabled and remounting with >> file name encryption enabled results in the following. >> >> $ ls -li >> ls: cannot access 'myfile3': No such file or directory >> total 48 >> 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile >> 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile >> 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 >> 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 >> ? -????????? ? ? ? ? ? myfile3 >> >> Prior to commit 88ae4ab9802e, the file system had to be mounted with >> encrypted file names first to create a file, then the same had to be >> repeated after mounting with unencrypted file names. Now the duplicate >> files can be created both ways (unencrypted _or_ encrypted first). >> >> The only real difference is that it is no longer possible to have a >> _working_ combination of encrypted and unencrypted file names. In other >> words, commit 88ae4ab9802e results in reduced functionality with no >> benefit whatsoever. >> >> Restore ability to have a mix of unencrypted and encrypted files. >> This effectively reverts commit 88ae4ab9802e, but the code is now >> better readable since it avoids a number of goto statements. > > I'd like for us to correctly fix 88ae4ab9802e rather than try to support > both filename types under a single mount since that's complex and there > are unknown corner cases to consider. I think this can be done by not > copying up the lower filename when an error is encountered in > ecryptfs_decode_and_decrypt_filename(). If filename encryption is > enabled, it should only return decrypted filenames or an error if it > isn't possible to decrypt the lower filename. > NP. I'll leave it alone, then. Since our use case requires both encrypted and unencrypted file names, our "fix" will be to carry this patch along locally as long as needed and stop using ecryptfs otherwise. Guenter > Tyler > >> >> Cc: Al Viro <viro@zeniv.linux.org.uk> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> fs/ecryptfs/inode.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c >> index 847904aa63a9..14a5c096ead6 100644 >> --- a/fs/ecryptfs/inode.c >> +++ b/fs/ecryptfs/inode.c >> @@ -392,11 +392,11 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode, >> int rc = 0; >> >> lower_dir_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry->d_parent); >> - >> + lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len); >> mount_crypt_stat = &ecryptfs_superblock_to_private( >> ecryptfs_dentry->d_sb)->mount_crypt_stat; >> - if (mount_crypt_stat >> - && (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)) { >> + if (IS_ERR(lower_dentry) && >> + (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)) { >> rc = ecryptfs_encrypt_and_encode_filename( >> &encrypted_and_encoded_name, &len, >> mount_crypt_stat, name, len); >> @@ -405,10 +405,10 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode, >> "filename; rc = [%d]\n", __func__, rc); >> return ERR_PTR(rc); >> } >> - name = encrypted_and_encoded_name; >> + lower_dentry = lookup_one_len_unlocked( >> + encrypted_and_encoded_name, lower_dir_dentry, len); >> } >> >> - lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len); >> if (IS_ERR(lower_dentry)) { >> ecryptfs_printk(KERN_DEBUG, "%s: lookup_one_len() returned " >> "[%ld] on lower_dentry = [%s]\n", __func__, >> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ecryptfs: Restore support for both encrypted and unencrypted file names 2018-03-28 13:33 ` Guenter Roeck @ 2018-03-28 14:19 ` Tyler Hicks 2018-03-29 0:32 ` [PATCH] eCryptfs: don't pass up plaintext names when using filename encryption Tyler Hicks 0 siblings, 1 reply; 6+ messages in thread From: Tyler Hicks @ 2018-03-28 14:19 UTC (permalink / raw) To: Guenter Roeck; +Cc: ecryptfs, linux-kernel, Al Viro [-- Attachment #1.1: Type: text/plain, Size: 6879 bytes --] On 03/28/2018 08:33 AM, Guenter Roeck wrote: > On 03/27/2018 08:58 AM, Tyler Hicks wrote: >> Hello Guenter >> >> On 02/13/2018 04:36 PM, Guenter Roeck wrote: >>> Commit 88ae4ab9802e ("ecryptfs_lookup(): try either only encrypted or >>> plaintext name") was supposed to fix a situation where two files with >>> the same name and same inode could be created in ecryptfs. One of those >>> files had an encrypted file name, the other file name was unencrypted. >> >> That's correct. Al was concerned about possible deadlocks with aliased >> dentries and I thought it would be best to only support encrypted and >> unencrypted but not both. >> >>> >>> After commit 88ae4ab9802e, having a mix of encrypted and unencrypted >>> file >>> names is no longer supposed to be possible. However, that is not the >>> case. >>> The only difference is that it is now even easier to create a situation >>> where two files with the same name coexist (one encrypted and the other >>> not encrypted). In practice, this looks like the following (files >>> created with v4.14.12). >>> >>> ecryptfs mounted with file name encryption enabled: >>> >>> $ ls -li >>> total 48 >>> 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile >>> 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile >>> 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 >>> 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 >>> $ grep . * >>> myfile:encrypted >>> myfile:encrypted >>> myfile2:encrypted >>> myfile2:encrypted >>> >>> $ ls -li >>> total 48 >>> 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 >>> ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVun.BU8Zu5-njbcIPoApxk7-E-- >>> >>> 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 >>> ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVunt0fda7t9YCtJ70cm911yZ--- >>> >>> 5252817 -rw-rw-r-- 1 groeck groeck 12 Jan 20 12:52 myfile >>> 5252827 -rw-rw-r-- 1 groeck groeck 12 Jan 20 15:37 myfile2 >>> >>> $ grep . * >>> ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVun.BU8Zu5-njbcIPoApxk7-E--:encrypted >>> >>> ECRYPTFS_FNEK_ENCRYPTED.FWbF9U6H6L6ekEZYGWnkfR4wMiyeTVoCeVunt0fda7t9YCtJ70cm911yZ---:encrypted >>> >>> myfile:unencrypted >>> myfile2:unencrypted >>> >>> Creating a file with file name encryption disabled and remounting with >>> file name encryption enabled results in the following. >>> >>> $ ls -li >>> ls: cannot access 'myfile3': No such file or directory >>> total 48 >>> 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile >>> 5252822 -rw-rw-r-- 1 groeck groeck 10 Jan 20 13:02 myfile >>> 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 >>> 5252824 -rw-rw-r-- 1 groeck groeck 10 Jan 20 15:36 myfile2 >>> ? -????????? ? ? ? ? ? myfile3 >>> >>> Prior to commit 88ae4ab9802e, the file system had to be mounted with >>> encrypted file names first to create a file, then the same had to be >>> repeated after mounting with unencrypted file names. Now the duplicate >>> files can be created both ways (unencrypted _or_ encrypted first). >>> >>> The only real difference is that it is no longer possible to have a >>> _working_ combination of encrypted and unencrypted file names. In other >>> words, commit 88ae4ab9802e results in reduced functionality with no >>> benefit whatsoever. >>> >>> Restore ability to have a mix of unencrypted and encrypted files. >>> This effectively reverts commit 88ae4ab9802e, but the code is now >>> better readable since it avoids a number of goto statements. >> >> I'd like for us to correctly fix 88ae4ab9802e rather than try to support >> both filename types under a single mount since that's complex and there >> are unknown corner cases to consider. I think this can be done by not >> copying up the lower filename when an error is encountered in >> ecryptfs_decode_and_decrypt_filename(). If filename encryption is >> enabled, it should only return decrypted filenames or an error if it >> isn't possible to decrypt the lower filename. >> > > NP. I'll leave it alone, then. Since our use case requires both encrypted > and unencrypted file names, our "fix" will be to carry this patch along > locally as long as needed and stop using ecryptfs otherwise. I think that's a good plan. While eCryptfs has been fairly stable for quite some time, it is starved for maintenance attention these days as you've noticed with this thread. :/ Thanks for bringing up this bug. I'll include you on any followup patches to fix the 88ae4ab9802e change so that you'll be aware of any additional local patches that you'll need to carry. Tyler > > Guenter > >> Tyler >> >>> >>> Cc: Al Viro <viro@zeniv.linux.org.uk> >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>> --- >>> fs/ecryptfs/inode.c | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c >>> index 847904aa63a9..14a5c096ead6 100644 >>> --- a/fs/ecryptfs/inode.c >>> +++ b/fs/ecryptfs/inode.c >>> @@ -392,11 +392,11 @@ static struct dentry *ecryptfs_lookup(struct >>> inode *ecryptfs_dir_inode, >>> int rc = 0; >>> lower_dir_dentry = >>> ecryptfs_dentry_to_lower(ecryptfs_dentry->d_parent); >>> - >>> + lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, >>> len); >>> mount_crypt_stat = &ecryptfs_superblock_to_private( >>> ecryptfs_dentry->d_sb)->mount_crypt_stat; >>> - if (mount_crypt_stat >>> - && (mount_crypt_stat->flags & >>> ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)) { >>> + if (IS_ERR(lower_dentry) && >>> + (mount_crypt_stat->flags & >>> ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)) { >>> rc = ecryptfs_encrypt_and_encode_filename( >>> &encrypted_and_encoded_name, &len, >>> mount_crypt_stat, name, len); >>> @@ -405,10 +405,10 @@ static struct dentry *ecryptfs_lookup(struct >>> inode *ecryptfs_dir_inode, >>> "filename; rc = [%d]\n", __func__, rc); >>> return ERR_PTR(rc); >>> } >>> - name = encrypted_and_encoded_name; >>> + lower_dentry = lookup_one_len_unlocked( >>> + encrypted_and_encoded_name, lower_dir_dentry, len); >>> } >>> - lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, >>> len); >>> if (IS_ERR(lower_dentry)) { >>> ecryptfs_printk(KERN_DEBUG, "%s: lookup_one_len() returned " >>> "[%ld] on lower_dentry = [%s]\n", __func__, >>> >> >> > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] eCryptfs: don't pass up plaintext names when using filename encryption 2018-03-28 14:19 ` Tyler Hicks @ 2018-03-29 0:32 ` Tyler Hicks 0 siblings, 0 replies; 6+ messages in thread From: Tyler Hicks @ 2018-03-29 0:32 UTC (permalink / raw) To: ecryptfs; +Cc: linux-kernel, Guenter Roeck, Al Viro Both ecryptfs_filldir() and ecryptfs_readlink_lower() use ecryptfs_decode_and_decrypt_filename() to translate lower filenames to upper filenames. The function correctly passes up lower filenames, unchanged, when filename encryption isn't in use. However, it was also passing up lower filenames when the filename wasn't encrypted or when decryption failed. Since 88ae4ab9802e, eCryptfs refuses to lookup lower plaintext names when filename encryption is enabled so this resulted in a situation where userspace would see lower plaintext filenames in calls to getdents(2) but then not be able to lookup those filenames. An example of this can be seen when enabling filename encryption on an eCryptfs mount at the root directory of an Ext4 filesystem: $ ls -1i /lower 12 ECRYPTFS_FNEK_ENCRYPTED.FWYZD8TcW.5FV-TKTEYOHsheiHX9a-w.NURCCYIMjI8pn5BDB9-h3fXwrE-- 11 lost+found $ ls -1i /upper ls: cannot access '/upper/lost+found': No such file or directory ? lost+found 12 test With this change, the lower lost+found dentry is ignored: $ ls -1i /lower 12 ECRYPTFS_FNEK_ENCRYPTED.FWYZD8TcW.5FV-TKTEYOHsheiHX9a-w.NURCCYIMjI8pn5BDB9-h3fXwrE-- 11 lost+found $ ls -1i /upper 12 test Additionally, some potentially noisy error/info messages in the related code paths are turned into debug messages so that the logs can't be easily filled. Fixes: 88ae4ab9802e ("ecryptfs_lookup(): try either only encrypted or plaintext name") Reported-by: Guenter Roeck <linux@roeck-us.net> Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Tyler Hicks <tyhicks@canonical.com> --- fs/ecryptfs/crypto.c | 41 ++++++++++++++++++++++++++++------------- fs/ecryptfs/file.c | 21 ++++++++++++++++----- 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c index 846ca15..4dd842f 100644 --- a/fs/ecryptfs/crypto.c +++ b/fs/ecryptfs/crypto.c @@ -1997,6 +1997,16 @@ int ecryptfs_encrypt_and_encode_filename( return rc; } +static bool is_dot_dotdot(const char *name, size_t name_size) +{ + if (name_size == 1 && name[0] == '.') + return true; + else if (name_size == 2 && name[0] == '.' && name[1] == '.') + return true; + + return false; +} + /** * ecryptfs_decode_and_decrypt_filename - converts the encoded cipher text name to decoded plaintext * @plaintext_name: The plaintext name @@ -2021,13 +2031,21 @@ int ecryptfs_decode_and_decrypt_filename(char **plaintext_name, size_t packet_size; int rc = 0; - if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES) - && !(mount_crypt_stat->flags & ECRYPTFS_ENCRYPTED_VIEW_ENABLED) - && (name_size > ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE) - && (strncmp(name, ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX, - ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE) == 0)) { - const char *orig_name = name; - size_t orig_name_size = name_size; + if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES) && + !(mount_crypt_stat->flags & ECRYPTFS_ENCRYPTED_VIEW_ENABLED)) { + if (is_dot_dotdot(name, name_size)) { + rc = ecryptfs_copy_filename(plaintext_name, + plaintext_name_size, + name, name_size); + goto out; + } + + if (name_size <= ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE || + strncmp(name, ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX, + ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE)) { + rc = -EINVAL; + goto out; + } name += ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE; name_size -= ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE; @@ -2047,12 +2065,9 @@ int ecryptfs_decode_and_decrypt_filename(char **plaintext_name, decoded_name, decoded_name_size); if (rc) { - printk(KERN_INFO "%s: Could not parse tag 70 packet " - "from filename; copying through filename " - "as-is\n", __func__); - rc = ecryptfs_copy_filename(plaintext_name, - plaintext_name_size, - orig_name, orig_name_size); + ecryptfs_printk(KERN_DEBUG, + "%s: Could not parse tag 70 packet from filename\n", + __func__); goto out_free; } } else { diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c index c74ed3c..b76a985 100644 --- a/fs/ecryptfs/file.c +++ b/fs/ecryptfs/file.c @@ -82,17 +82,28 @@ ecryptfs_filldir(struct dir_context *ctx, const char *lower_name, buf->sb, lower_name, lower_namelen); if (rc) { - printk(KERN_ERR "%s: Error attempting to decode and decrypt " - "filename [%s]; rc = [%d]\n", __func__, lower_name, - rc); - goto out; + if (rc != -EINVAL) { + ecryptfs_printk(KERN_DEBUG, + "%s: Error attempting to decode and decrypt filename [%s]; rc = [%d]\n", + __func__, lower_name, rc); + return rc; + } + + /* Mask -EINVAL errors as these are most likely due a plaintext + * filename present in the lower filesystem despite filename + * encryption being enabled. One unavoidable example would be + * the "lost+found" dentry in the root directory of an Ext4 + * filesystem. + */ + return 0; } + buf->caller->pos = buf->ctx.pos; rc = !dir_emit(buf->caller, name, name_size, ino, d_type); kfree(name); if (!rc) buf->entries_written++; -out: + return rc; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-29 0:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-13 22:36 [PATCH] ecryptfs: Restore support for both encrypted and unencrypted file names Guenter Roeck 2018-02-28 19:27 ` Guenter Roeck 2018-03-27 15:58 ` [PATCH] " Tyler Hicks 2018-03-28 13:33 ` Guenter Roeck 2018-03-28 14:19 ` Tyler Hicks 2018-03-29 0:32 ` [PATCH] eCryptfs: don't pass up plaintext names when using filename encryption Tyler Hicks
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).