| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <277bf08e-2547-f29a-ec91-86647ab455d4@redhat.com>
Date: Mon, 28 Feb 2022 08:42:05 +0800
From: Xiubo Li <xiubli@...hat.com>
To: Luís Henriques <lhenriques@...e.de>
Cc: Jeff Layton <jlayton@...nel.org>,
Ilya Dryomov <idryomov@...il.com>, ceph-devel@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] ceph: add support for encrypted snapshot names
On 2/26/22 10:58 PM, Luís Henriques wrote:
> Xiubo Li <xiubli@...hat.com> writes:
>
>> On 2/25/22 5:48 PM, Luís Henriques wrote:
>>> Xiubo Li <xiubli@...hat.com> writes:
>>>
>>>> On 2/24/22 7:21 PM, Luís Henriques wrote:
>>>>> Since filenames in encrypted directories are already encrypted and shown
>>>>> as a base64-encoded string when the directory is locked, snapshot names
>>>>> should show a similar behaviour.
>>>>>
>>>>> Signed-off-by: Luís Henriques <lhenriques@...e.de>
>>>>> ---
>>>>> fs/ceph/dir.c | 15 +++++++++++++++
>>>>> fs/ceph/inode.c | 10 +++++++++-
>>>>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>>>>
>>>>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
>>>>> still TBD. I thought it would be something easy to do, but snapshots
>>>>> don't seem to make use of the CDir/CDentry (which is where alternate_name
>>>>> is stored on the MDS). I'm still looking into this, but I may need some
>>>>> help there :-(
>>>>>
>>>>> Cheers,
>>>>> --
>>>>> Luís
>>>>>
>>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>>> index a449f4a07c07..20ae600ee7cd 100644
>>>>> --- a/fs/ceph/dir.c
>>>>> +++ b/fs/ceph/dir.c
>>>>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>>>> op = CEPH_MDS_OP_MKSNAP;
>>>>> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>>>>> dentry, dentry);
>>>>> + /* XXX missing support for alternate_name in snapshots */
>>>>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
>>>>> + dout("encrypted snapshot name too long: %pd len: %d\n",
>>>>> + dentry, dentry->d_name.len);
>>>>> + err = -ENAMETOOLONG;
>>>>> + goto out;
>>>>> + }
>>>>> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
>>>>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>>>>> op = CEPH_MDS_OP_MKDIR;
>>>>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>>>> !req->r_reply_info.head->is_target &&
>>>>> !req->r_reply_info.head->is_dentry)
>>>>> err = ceph_handle_notrace_create(dir, dentry);
>>>>> +
>>>>> + /*
>>>>> + * If we have created a snapshot we need to clear the cache, otherwise
>>>>> + * snapshot will show encrypted filenames in readdir.
>>>>> + */
>>>> Do you mean dencrypted filenames ?
>>> What I see without this d_drop() is that, if I run an 'ls' in a snapshot
>>> directory immediately after creating it, the filenames in that snapshot
>>> will be encrypted. Maybe there's a bug somewhere else and this d_drop()
>>> isn't the right fix...?
>> BTW, how did you reproduce this ?
>>
>> The snapshot name hasn't encrypted yet ? Did you add one patch to do this ?
> I don't think I understand what you're referring to. I haven't looked
> into you patch (probably won't be able to do in before Wednesday) but if
> you remove the d_drop() in ceph_mkdir() in this patch, here's what I use
> to reproduce the issue:
>
> # mkdir mydir
> # fscrypt encrypt mydir
> # cd mydir
> # create a few files
> # mkdir .snap/snapshot-01
> # ls -l .snap/snapshot-01
This is my test by using the branch 'wip-fscrypt' branch:
[root@...ceph1 kcephfs]# cd mydir/
[root@...ceph1 mydir]# mkdir a b cd
[root@...ceph1 mydir]# ls
a b c
[root@...ceph1 mydir]# ls .snap
[root@...ceph1 mydir]# mkdir .snap/mydir_snap1
[root@...ceph1 mydir]# ls .snap/mydir_snap1
a b c
[root@...ceph1 mydir]# ls .snap/
mydir_snap1
[root@...ceph1 mydir]# touch file1 file2 file3
[root@...ceph1 mydir]# mkdir .snap/mydir_snap2
[root@...ceph1 mydir]# ls .snap/mydir_snap2
a b c file1 file2 file3
[root@...ceph1 mydir]# ls .snap/
mydir_snap1 mydir_snap2
[root@...ceph1 mydir]# ls
a b c file1 file2 file3
[root@...ceph1 mydir]# cd ..
[root@...ceph1 kcephfs]# fscrypt lock mydir/
"mydir/" is now locked.
[root@...ceph1 kcephfs]# cd mydir/
[root@...ceph1 mydir]# ls
5D7Q8T0rdSRiJZnvbpCWRFQnLb3BxO4-zyVHsFCH98o
LWbsdS2rvuCALUXE0TrTqbuElw4zU6cZHg62-LY5GMA
u2nEDclQZAdtetYqQ7aCWNFwu8-1FDH9vI6pM4o6ZN8
-fKqUSM9DGlT1KNE-pkygEXAdjwf9fTDROA_6ZkDEio
m42jW1zJs75o2dx0bEHNmEWx9GmYXxHveSmBFagwPOw
ZLYwBnb7WT78Saz5RFEOdrKn3OLb6AfHk-IElAmEVps
[root@...ceph1 mydir]# ls .snap/
mydir_snap1 mydir_snap2
[root@...ceph1 mydir]# ls .snap/mydir_snap1
LWbsdS2rvuCALUXE0TrTqbuElw4zU6cZHg62-LY5GMA
m42jW1zJs75o2dx0bEHNmEWx9GmYXxHveSmBFagwPOw
ZLYwBnb7WT78Saz5RFEOdrKn3OLb6AfHk-IElAmEVps
[root@...ceph1 mydir]# ls .snap/mydir_snap2
5D7Q8T0rdSRiJZnvbpCWRFQnLb3BxO4-zyVHsFCH98o
LWbsdS2rvuCALUXE0TrTqbuElw4zU6cZHg62-LY5GMA
u2nEDclQZAdtetYqQ7aCWNFwu8-1FDH9vI6pM4o6ZN8
-fKqUSM9DGlT1KNE-pkygEXAdjwf9fTDROA_6ZkDEio
m42jW1zJs75o2dx0bEHNmEWx9GmYXxHveSmBFagwPOw
ZLYwBnb7WT78Saz5RFEOdrKn3OLb6AfHk-IElAmEVps
[root@...ceph1 mydir]# cd ..
[root@...ceph1 kcephfs]# fscrypt unlock mydir/
Enter custom passphrase for protector "l":
"mydir/" is now unlocked and ready for use.
[root@...ceph1 kcephfs]# cd mydir/
[root@...ceph1 mydir]# ls
a b c file1 file2 file3
[root@...ceph1 mydir]# ls .snap/mydir_snap1
a b c
[root@...ceph1 mydir]# ls .snap/mydir_snap2
a b c file1 file2 file3
[root@...ceph1 mydir]#
We can see that only the "mydir_snap1" and "mydir_snap2" snapshot names
are not encrypted when the 'mydir' is locked, my patch above is fixing
this issue. All the others work as expected.
>
> And this would show the contents 'snapshot-01' but with the filenames
> encrypted, even with 'mydir' isn't locked.
>
> With this d_drop(), this behaviour will go away, i.e. you'll see the
> correct (unencrypted) filenames.
The tests above without this changes in your patch.
> Also, after locking 'mydir' (fscrypt lock mydir), an 'ls' in the snapshot
> directory ('ls mydir/.snap') will show the _encrypted_ snapshot names and
> an 'ls' in the snapshot itself ('ls mydir/.snap/<ENCRYPTED NAME>') will
> show the encrypted filenames as in an 'ls mydir'.
Not sure whether I missed something here, so strange I couldn't
reproduce it.
BTW, which branch were you using to test this ?
I will post my patch to fix the issue I mentioned.
- Xiubo
> Cheers,
Powered by blists - more mailing lists