On Sat, Aug 24, 2024 at 03:34:20AM +0200, Jann Horn wrote:
> On Sat, Aug 24, 2024 at 2:31 AM Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
> > On Fri, Aug 23, 2024 at 08:38:55PM +0200, Jann Horn wrote:
> > > Fix it by rejecting any firmware names containing ".." path components.
> [...]
> > > +/*
> > > + * Reject firmware file names with ".." path components.
> > > + * There are drivers that construct firmware file names from device-supplied
> > > + * strings, and we don't want some device to be able to tell us "I would like to
> > > + * be sent my firmware from ../../../etc/shadow, please".
> > > + *
> > > + * Search for ".." surrounded by either '/' or start/end of string.
> > > + *
> > > + * This intentionally only looks at the firmware name, not at the firmware base
> > > + * directory or at symlink contents.
> > > + */
> > > +static bool name_contains_dotdot(const char *name)
> > > +{
> > > + size_t name_len = strlen(name);
> > > + size_t i;
> > > +
> > > + if (name_len < 2)
> > > + return false;
> > > + for (i = 0; i < name_len - 1; i++) {
> > > + /* do we see a ".." sequence? */
> > > + if (name[i] != '.' || name[i+1] != '.')
> > > + continue;
> > > +
> > > + /* is it a path component? */
> > > + if ((i == 0 || name[i-1] == '/') &&
> > > + (i == name_len - 2 || name[i+2] == '/'))
> > > + return true;
> > > + }
> > > + return false;
> > > +}
> >
> > Why do you open code it, instead of using strstr() and strncmp() like you did
> > in v1? I think your approach from v1 read way better.
>
> The code in v1 was kinda sloppy - it was probably good enough for this
> check, but not good enough to put in a function called
> name_contains_dotdot() that is documented to exactly search for any
> ".." components.
>
> Basically, the precise regex we have to search for is something like
> /(^|/)\.\.($|/)/
>
> To implement that by searching for substrings like in v1, we'd have to
> search for each possible combination of the capture groups in the
> regex, which gives the following four (pow(2,2)) patterns:
>
> <start>..<end>
> <start>../
> /..<end>
> /../
I see.
>
> So written like in v1, that'd look something like:
>
> if (strcmp(name, "..") == 0 || strncmp(name, "../", 3) == 0 ||
> strstr(name, "/../") != NULL || (name_len >= 3 &&
> strcmp(name+name_len-3, "/..") == 0)))
> return true;
I think I still slightly prefer this variant, but I think either one is fine.
With one or the other and dev_warn() fixed,
Reviewed-by: Danilo Krummrich <dakr@xxxxxxxxxx>
>
> Compared to that, I prefer the code I wrote in v2, since it is less
> repetitive. But if you want, I can change it to the expression I wrote
> just now.
>
