Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   =?utf-8?Q?Lu=C3=ADs_Henriques?= <lhenriques@suse.de>
To:     Jeff Layton <jlayton@kernel.org>
Cc:     Xiubo Li <xiubli@redhat.com>, Ilya Dryomov <idryomov@gmail.com>,
        ceph-devel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/3] ceph: minor fixes and encrypted snapshot names
References: <20220304161403.19295-1-lhenriques@suse.de>
        <87fsnx4rb3.fsf@brahms.olymp>
        <e7f91d0be0f41320e5a4f38ded1bde166626a17f.camel@kernel.org>
Date:   Sat, 05 Mar 2022 14:56:49 +0000
In-Reply-To: <e7f91d0be0f41320e5a4f38ded1bde166626a17f.camel@kernel.org> (Jeff
        Layton's message of "Fri, 04 Mar 2022 13:30:50 -0500")
Message-ID: <878rtoo3bi.fsf@brahms.olymp>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Jeff Layton <jlayton@kernel.org> writes:

> On Fri, 2022-03-04 at 16:26 +0000, Lu=C3=ADs Henriques wrote:
>> Lu=C3=ADs Henriques <lhenriques@suse.de> writes:
>>=20
>> > Hi!
>> >=20
>> > 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.
>> >=20
>> > Two notes:
>> >=20
>> > 1. Patch 0001 is just a small fix from another fscrypt patch.  It's
>> >    probably better to simply squash it.
>> >=20
>> > 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.
>>=20
>> Obviously, immediately after sending this patchset I realized I failed to
>> mention a very (*VERY*) important note:
>>=20
>> 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.
>>=20
>> 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.
>>=20
>
>
> 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,
--=20
Lu=C3=ADs