On Tue, Aug 20, 2024 at 2:23 AM Jann Horn <jannh@xxxxxxxxxx> wrote:
> On Tue, Aug 20, 2024 at 2:14 AM Danilo Krummrich <dakr@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > > Fixes: abb139e75c2c ("firmware: teach the kernel to load firmware files directly from the filesystem")
> > > Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> > > ---
> > > 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 if we are seriously concerned that someone might rely on that,
there are several things we could do to mitigate it, ordered by
increasing level of how annoying it would be to implement and how much
it would nerf the check:
1. add a pr_warn() specifically for this case, so if it does break,
users know what's wrong and can complain - I think I should probably
do that in v2 anyway
2. add a module parameter to disable the check, so if it does break,
users can immediately work around the issue
3. make the whole thing just a warning for now, and revisit it in a
year or so to enable enforcement
My preference would be to implement number 1 but not 2/3, but if you
think that's not enough to merge it, I could implement 2 or 3...
