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

[Luís Henriques]

Xiubo Li <xiubli@xxxxxxxxxx> writes:

> On 3/5/22 10:56 PM, Luís Henriques wrote:
>> Jeff Layton <jlayton@xxxxxxxxxx> writes:
>>
>>> On Fri, 2022-03-04 at 16:26 +0000, Luís Henriques wrote:
>>>> Luís Henriques <lhenriques@xxxxxxx> 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 ?

I think this will only make the encrypted snapshot creation less likely to
fail, but it's still possible that the base64 encoded of the encrypted
snapshot name to start with '_' and end with '_${DIGIT}'.  Or have I
misunderstood your suggestion?

> 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.

Yeah, that PR doesn't look too bad, but I'm still looking into your
kernel-side patch.  Which adds a *lot* of complexity to this.  This whole
fscrypt thing is really getting on my nerves -- every single step that
seems simple ends up being a huge pain :-/

Xiubo, have you tested your PR (and corresponding kernel changes) with my
patch?  Do they work correctly?  (I'm about to start looking into doing
that myself, so no worries if you didn't tried that.)

Cheers,
-- 
Luís

>
> [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,
>