Re: [PATCH 0/3] ceph: minor fixes and encrypted snapshot names

[Xiubo Li <xiubli@redhat.com>]

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,