Message-ID: <Zskl9-0VKnixHA9X@pollux>
Date: Sat, 24 Aug 2024 02:14:47 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Luis Chamberlain <mcgrof@...nel.org>
Cc: Jann Horn <jannh@...gle.com>,
	Linus Torvalds <torvalds@...ux-foundation.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 Fri, Aug 23, 2024 at 02:13:13PM -0700, Luis Chamberlain wrote:
> 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

I don't think we can fully validate it, there are lots of cases, where path
names are passed through large call stacks, concatenated with other strings,
selected from arrays, etc.

So, if we want to be extra careful, we should indeed just warn for now.

> suggest for now we leave the warning in place for one kernel release,
> and for the one after we enforce this.

I'd expect it to take a bit longer until someone recognizes when drivers for
embedded stuff hit this, but that's probably fine, hopefully some vendor
discovers it before it goes to end users. :)

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

Powered by blists - more mailing lists