linux-kernel - [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-next>] [day] [month] [year] [list]

Message-Id: <20240820-firmware-traversal-v1-1-8699ffaa9276@google.com>
Date: Tue, 20 Aug 2024 01:18:54 +0200
From: Jann Horn <jannh@...gle.com>
To: Luis Chamberlain <mcgrof@...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>
Cc: linux-kernel@...r.kernel.org, stable@...r.kernel.org, 
 Jann Horn <jannh@...gle.com>
Subject: [PATCH] firmware_loader: Block path traversal

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.

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) {
 		ret = -EINVAL;
 		goto out;
 	}

---
base-commit: b0da640826ba3b6506b4996a6b23a429235e6923
change-id: 20240820-firmware-traversal-6df8501b0fe4
-- 
Jann Horn <jannh@...gle.com>