On Tue, 2020-09-15 at 13:40 -0700, Eric Biggers wrote:
> On Tue, Sep 15, 2020 at 09:27:49AM -0400, Jeff Layton wrote:
> > > > + }
> > > > +
> > > > + declen = fscrypt_base64_decode(name, len, tname->name);
> > > > + if (declen < 0 || declen > NAME_MAX) {
> > > > + ret = -EIO;
> > > > + goto out;
> > > > + }
> > >
> > > declen <= 0, to cover the empty name case.
> > >
> > > Also, is there a point in checking for > NAME_MAX?
> > >
> >
> > IDK. We're getting these strings from the MDS and they could end up
> > being corrupt if there are bugs there (or if the MDS is compromised).
> > Of course, if we get a name longer than NAME_MAX then we've overrun the
> > buffer.
> >
> > Maybe we should add a maxlen parameter to fscrypt_base64_encode/decode ?
> > Or maybe I should just have fscrypt_fname_alloc_buffer allocate a buffer
> > the same size as "len"? It might be a little larger than necessary, but
> > that would be safer.
>
> How about checking that the base64-encoded filename is <= BASE64_CHARS(NAME_MAX)
> in length? Then decoding it can't give more than NAME_MAX bytes.
>
> fscrypt_setup_filename() does a similar check when decoding a no-key name.
>
Good idea. I'll roll that in.
Thanks,
--
Jeff Layton <jlayton@xxxxxxxxxx>
|