Jann Horn <jannh@xxxxxxxxxx> writes: > 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> > /../ > > 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; > > 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. Maybe for (p = s; (q = strstr(p, "..")) != NULL; p = q+2) { if ((q == s || q[-1] == '/') && (q[2] == '\0' || q[2] == '/')) return true; } return false; ? Rasmus