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: <Zsj7afivXqOL1FXG@bombadil.infradead.org>
Date: Fri, 23 Aug 2024 14:13:13 -0700
From: Luis Chamberlain <mcgrof@...nel.org>
To: Jann Horn <jannh@...gle.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Cc: 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 Fri, Aug 23, 2024 at 08:38:55PM +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.)
> 
> Fix it by rejecting any firmware names containing ".." path components.
> 
> For what it's worth, I went looking and haven't found any USB device
> drivers that use the firmware loader dangerously.
> 
> 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'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. Without a semantic
patch assessment to do this (I think its possible with coccinelle) I'd
suggest for now we leave the warning in place for one kernel release,
and for the one after we enforce this.

Linus might feel differently over it, and may want it right away. I'll
let him chime in.

  Luis