lkml.org 
[lkml]   [2022]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 0/3] ceph: minor fixes and encrypted snapshot names
From
Date

On 3/5/22 10:56 PM, Luís Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:
>
>> On Fri, 2022-03-04 at 16:26 +0000, Luís Henriques wrote:
>>> Luís Henriques <lhenriques@suse.de> writes:
>>>
>>>> Hi!
>>>>
>>>> I'm sending another iteration of the encrypted snapshot names patch. This
>>>> patch assumes PR#45224 [1] to be merged as it adds support for the
>>>> alternate names.
>>>>
>>>> Two notes:
>>>>
>>>> 1. Patch 0001 is just a small fix from another fscrypt patch. It's
>>>> probably better to simply squash it.
>>>>
>>>> 2. I'm not sure how easy it is to hit the UAF fixed by patch 0002. I can
>>>> reproduce it easily by commenting the code that adds the
>>>> DCACHE_NOKEY_NAME flag in patch 0003.
>>> Obviously, immediately after sending this patchset I realized I failed to
>>> mention a very (*VERY*) important note:
>>>
>>> Snapshot names can not start with a '_'. I think the reason is related
>>> with the 'long snapshot names', but I can't really remember the details
>>> anymore. The point is that an encrypted snapshot name base64-encoded
>>> *may* end-up starting with an '_' as we're using the base64-url variant.
>>>
>>> I really don't know if it's possible to fix that. I guess that in that
>>> case the user will get an error and fail to create the snapshot but he'll
>>> be clueless because the reason. Probably a warning can be added to the
>>> kernel logs, but maybe there are other ideas.
>>>
>>
>> Ouch. Is that imposed by the MDS? It'd be best if we could remove that
>> limitation from it altogether if we can.
> I do remember hitting this limitation in the past, but a quick grep didn't
> show anything in the documentation about it. This seems to have been
> added to the MDS a *long* time ago, with commit 068553473c82 ("mds: adjust
> trace encoding, clean up snap naming") but (as usual) there aren't a lot
> of details.

When making a snapshot and in MDS code:

10458   if (snapname.length() == 0 ||
10459       snapname[0] == '_') {
10460     respond_to_request(mdr, -CEPHFS_EINVAL);
10461     return;
10462   }


>> If we can't, then we might be able to get away with prepending all the
>> encrypted names with some legal characte. Then when we go to decrypt it
>> we just strip that off.
> This is probably the best way to fix it, but it's worth trying to find
> out the origins of this limitation. I do seem to remember some obscure
> reasons, related with the long snap names (for which Xiubo has a patch),
> which will start with '_'. But yeah I'll have to go dig deeper.

It will recognize the encrypted "_XYZ_${DIGIT}" snapshot name as the
long snapshot name inherited from its parent snap realm, and will parse
the "${DIGIT}" as an ino in other places.

Maybe in MDS we should fail the request only when snapshot name is in
type of "_XYZ_${DIGIT}" instead of only "_XYZ", and in client side
should also print one error or warn log about this ?

This why added the ceph PR[1] to tell the kclient current snapshot name
is a long snap name in lssnap. So if we can forbid the snap shot name
begin with '_' it will simple in kclient code to handle the long snap
name, or it will be complex in both MDS and kclient.

[1] https://github.com/ceph/ceph/pull/45208

-- Xiubo


>> We could also consider changing the base64 routine to use something else
>> in lieu of '_' but that's more of a hassle.
> Cheers,

\
 
 \ /
  Last update: 2022-03-07 01:50    [W:0.214 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site