| From f0e5311aa8022107d63c54e2f03684ec097d1394 Mon Sep 17 00:00:00 2001 |
| From: Jann Horn <jannh@google.com> |
| Date: Wed, 28 Aug 2024 01:45:48 +0200 |
| Subject: firmware_loader: Block path traversal |
| |
| From: Jann Horn <jannh@google.com> |
| |
| commit f0e5311aa8022107d63c54e2f03684ec097d1394 upstream. |
| |
| 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@vger.kernel.org |
| Reviewed-by: Danilo Krummrich <dakr@kernel.org> |
| Fixes: abb139e75c2c ("firmware: teach the kernel to load firmware files directly from the filesystem") |
| Signed-off-by: Jann Horn <jannh@google.com> |
| Acked-by: Luis Chamberlain <mcgrof@kernel.org> |
| Link: https://lore.kernel.org/r/20240828-firmware-traversal-v3-1-c76529c63b5f@google.com |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/base/firmware_loader/main.c | 30 ++++++++++++++++++++++++++++++ |
| 1 file changed, 30 insertions(+) |
| |
| --- a/drivers/base/firmware_loader/main.c |
| +++ b/drivers/base/firmware_loader/main.c |
| @@ -565,6 +565,26 @@ static void fw_abort_batch_reqs(struct f |
| mutex_unlock(&fw_lock); |
| } |
| |
| +/* |
| + * Reject firmware file names with ".." path components. |
| + * 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". |
| + * |
| + * Search for ".." surrounded by either '/' or start/end of string. |
| + * |
| + * This intentionally only looks at the firmware name, not at the firmware base |
| + * directory or at symlink contents. |
| + */ |
| +static bool name_contains_dotdot(const char *name) |
| +{ |
| + size_t name_len = strlen(name); |
| + |
| + return strcmp(name, "..") == 0 || strncmp(name, "../", 3) == 0 || |
| + strstr(name, "/../") != NULL || |
| + (name_len >= 3 && strcmp(name+name_len-3, "/..") == 0); |
| +} |
| + |
| /* called from request_firmware() and request_firmware_work_func() */ |
| static int |
| _request_firmware(const struct firmware **firmware_p, const char *name, |
| @@ -582,6 +602,14 @@ _request_firmware(const struct firmware |
| goto out; |
| } |
| |
| + if (name_contains_dotdot(name)) { |
| + dev_warn(device, |
| + "Firmware load for '%s' refused, path contains '..' component\n", |
| + name); |
| + ret = -EINVAL; |
| + goto out; |
| + } |
| + |
| ret = _request_firmware_prepare(&fw, name, device, buf, size, |
| opt_flags); |
| if (ret <= 0) /* error or already assigned */ |
| @@ -622,6 +650,8 @@ _request_firmware(const struct firmware |
| * @name will be used as $FIRMWARE in the uevent environment and |
| * should be distinctive enough not to be confused with any other |
| * firmware image for this or any other device. |
| + * It must not contain any ".." path components - "foo/bar..bin" is |
| + * allowed, but "foo/../bar.bin" is not. |
| * |
| * Caller must hold the reference count of @device. |
| * |