linux-kernel - Re: [PATCH] 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: <CAG48ez3AEU+LD-i7Qwo3kreJ0zGQEZOnthFX++QTUOMxe3e40Q@mail.gmail.com>
Date: Tue, 20 Aug 2024 02:23:05 +0200
From: Jann Horn <jannh@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Luis Chamberlain <mcgrof@...nel.org>, Russ Weight <russ.weight@...ux.dev>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] firmware_loader: Block path traversal

On Tue, Aug 20, 2024 at 2:14 AM Danilo Krummrich <dakr@...nel.org> wrote:
> On Tue, Aug 20, 2024 at 01:18:54AM +0200, Jann Horn wrote:
> > Most firmware names are hardcoded strings, or are constructed from fairly
> > constrained format strings where the dynamic parts are just some hex
> > numbers or such.
> >
> > However, there are a couple codepaths in the kernel where firmware file
> > names contain string components that are passed through from a device or
> > semi-privileged userspace; the ones I could find (not counting interfaces
> > that require root privileges) are:
> >
> >  - lpfc_sli4_request_firmware_update() seems to construct the firmware
> >    filename from "ModelName", a string that was previously parsed out of
> >    some descriptor ("Vital Product Data") in lpfc_fill_vpd()
> >  - nfp_net_fw_find() seems to construct a firmware filename from a model
> >    name coming from nfp_hwinfo_lookup(pf->hwinfo, "nffw.partno"), which I
> >    think parses some descriptor that was read from the device.
> >    (But this case likely isn't exploitable because the format string looks
> >    like "netronome/nic_%s", and there shouldn't be any *folders* starting
> >    with "netronome/nic_". The previous case was different because there,
> >    the "%s" is *at the start* of the format string.)
> >  - module_flash_fw_schedule() is reachable from the
> >    ETHTOOL_MSG_MODULE_FW_FLASH_ACT netlink command, which is marked as
> >    GENL_UNS_ADMIN_PERM (meaning CAP_NET_ADMIN inside a user namespace is
> >    enough to pass the privilege check), and takes a userspace-provided
> >    firmware name.
> >    (But I think to reach this case, you need to have CAP_NET_ADMIN over a
> >    network namespace that a special kind of ethernet device is mapped into,
> >    so I think this is not a viable attack path in practice.)
> >
> > For what it's worth, I went looking and haven't found any USB device
> > drivers that use the firmware loader dangerously.
>
> Your commit message very well describes the status quo, but only implies the
> problem, and skips how you intend to solve it.
>
> >
> > Cc: stable@...r.kernel.org
> > Fixes: abb139e75c2c ("firmware: teach the kernel to load firmware files directly from the filesystem")
> > Signed-off-by: Jann Horn <jannh@...gle.com>
> > ---
> > I wasn't sure whether to mark this one for stable or not - but I think
> > since there seems to be at least one PCI device model which could
> > trigger firmware loading with directory traversal, we should probably
> > backport the fix?
> > ---
> >  drivers/base/firmware_loader/main.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> > index a03ee4b11134..a32be64f3bf5 100644
> > --- a/drivers/base/firmware_loader/main.c
> > +++ b/drivers/base/firmware_loader/main.c
> > @@ -864,7 +864,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> >       if (!firmware_p)
> >               return -EINVAL;
> >
> > -     if (!name || name[0] == '\0') {
> > +     /*
> > +      * Reject firmware file names with "/../" sequences in them.
> > +      * 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".
> > +      */
> > +     if (!name || name[0] == '\0' ||
> > +         strstr(name, "/../") != NULL || strncmp(name, "../", 3) == 0) {
>
> Seems reasonable, but are there any API users that rely on that?

I tried grepping for in-kernel users and didn't find any, though I
guess I could have missed something.
I suppose slightly more likely than in-kernel users, there could be
userspace code out there that intentionally uses netlink or sysfs
interfaces to tell the kernel to load from firmware paths outside the
firmware directory, though that would be kinda weird?

> I guess we can't just check for strstr(name, "../"), because "foo.." is a valid
> file name?

Yeah, that's the intent.

> Maybe it would be worth adding a comment and / or a small
> helper function for that.

Yeah, I guess that might make it clearer.

> I also suggest to update the documentation of the firmware loader API to let
> people know that going back the path isn't tolerated by this API.

In Documentation/driver-api/firmware/request_firmware.rst, correct?

> >               ret = -EINVAL;
> >               goto out;
> >       }
> >
> > ---
> > base-commit: b0da640826ba3b6506b4996a6b23a429235e6923
> > change-id: 20240820-firmware-traversal-6df8501b0fe4
> > --
> > Jann Horn <jannh@...gle.com>
> >