linux-kernel - Re: [PATCH v2] firmware_loader: Block path traversal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87seurd89w.fsf@prevas.dk>
Date: Mon, 26 Aug 2024 09:59:23 +0200
From: Rasmus Villemoes <linux@...musvillemoes.dk>
To: Jann Horn <jannh@...gle.com>
Cc: Danilo Krummrich <dakr@...nel.org>,  Luis Chamberlain
 <mcgrof@...nel.org>,  Russ Weight <russ.weight@...ux.dev>,  Danilo
 Krummrich <dakr@...hat.com>,  Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>,  "Rafael J. Wysocki" <rafael@...nel.org>,
  linux-kernel@...r.kernel.org,  stable@...r.kernel.org
Subject: Re: [PATCH v2] firmware_loader: Block path traversal

Jann Horn <jannh@...gle.com> writes:

> On Sat, Aug 24, 2024 at 2:31 AM Danilo Krummrich <dakr@...nel.org> 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