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

[Luís Henriques]

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.

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

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

Cheers,
-- 
Luís