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: <CAG48ez2_Gs=fuG5vwML-gCzvZcVDJJy=Tr8p+ANxW4h2dKBAjQ@mail.gmail.com>
Date: Sat, 24 Aug 2024 03:48:21 +0200
From: Jann Horn <jannh@...gle.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Luis Chamberlain <mcgrof@...nel.org>, Christian Brauner <brauner@...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

On Sat, Aug 24, 2024 at 3:14 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Sat, Aug 24, 2024 at 5:13 AM Luis Chamberlain <mcgrof@...nel.org> wrote:
> >
> > I'm all for this, however a strong rejection outright for the first
> > kernel release is bound to end up with some angry user with some oddball
> > driver that had this for whatever stupid reason.
>
> I can't actually see a reason why a firmware file would have a ".."
> component in it, so I think the immediate rejection is fine -
> particularly since it has a warning printout, so you see what happened
> and why.
>
> I do wonder if we should just have a LOOKUP_NO_DOTDOT flag, and just use that.
>
> [ Christian - the issue is the firmware loading path not wanting to
> have ".." in the pathname so that you can't load outside the normal
> firmware tree. We could also use LOOKUP_BENEATH, except
> kernel_read_file_from_path_initns() just takes one long path rather
> than "here's the base, and here's the path". ]

One other difference between the semantics we need here and
LOOKUP_BENEATH is that we need to allow *symlinks* that contain ".."
components or absolute paths; just the original path string must not
contain them. If root decides to put symlinks to other places on the
disk into /lib/firmware, I think that's reasonable, and it's root's
decision to make, and we shouldn't break that. (And as an example, on
my Debian machine, I see that /lib/firmware/regulatory.db is a symlink
to /etc/alternatives/regulatory.db, which in turn is a symlink to
/lib/firmware/regulatory.db-debian. I also see a bunch of symlinks in
subdirectories of /lib/firmware with ".." in the link destinations,
though those don't escape from /lib/firmware.)

So if we do this with a lookup flag, it'd have to be something that
only takes effect when nd->depth is 0, or something vaguely along
those lines? IDK how exactly that part of the path walking code works.

> There might be other people who want LOOKUP_NO_DOTDOT for similar
> reasons. In fact, some people might want an even stronger "normalized
> path" validation, where empty components or just "." is invalid, just
> because that makes pathnames ambiguous.

(For what it's worth, I think I have seen many copies of this kind of
string-based checking for ".." components in various pieces of
userspace code. I don't think I've seen many places in the kernel that
would benefit from that.)