firmware: make sure paths remain relative
PATCH v2 1/1 firmware: make sure paths remain relative
Some devices have configurable firmware locations. If these configuration mechanisms are exposed to unprivileged userspace, it may be possible to load firmware from an unexpected location. To minimize the risk of this, make sure the string "../" does not appear in the firmware name. This means that neither the users of request_firmware, nor the uevent handler have to do this filtering themselves.
---
v2:
- add dev_err on failure, suggested by Ming Lei.
--- drivers/base/firmware_class.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/base/firmware_class.c +6 -0
@@ -814,6 +814,12 @@ _request_firmware_prepare(const struct firmware **firmware_p, const char *name,if (!firmware_p)return ERR_PTR (-EINVAL ); +if (strstr (name,"../" )) { +dev_err (device,"%s: parent directory in firmware name (%s)\n" , + __func__, name); +return ERR_PTR (-EINVAL ); + } + *firmware_p = firmware =kzalloc (sizeof (*firmware),GFP_KERNEL );if (!firmware) { dev_err(device, "%s: kmalloc(struct firmware) failed\n",
Signature · 6 lines
1.7.9.5 -- Kees Cook Chrome OS Security
Quoted 6 lines · On Tue, 18 Dec 2012 13:04:49 -0800 Kees Cook <[email protected]> wrote:
Some devices have configurable firmware locations. If these configuration mechanisms are exposed to unprivileged userspace, it may be possible to load firmware from an unexpected location. To minimize the risk of this, make sure the string "../" does not appear in the firmware name. This means that neither the users of
request_firmware, nor the uevent handler have to do this filtering themselves.
Have you verified no existing distribution ever does this ?
drivers/base/firmware_class.c _request_firmware_prepare(const struct firmware **firmware_p, const char *name, · line 817
Quoted 1 line
drivers/base/firmware_class.c +1
-0+if (strstr (name,"../" )) {
and this is obviously buggy and hasn't been tested
Hint foo/bar../baz
You need to check for ^../ /../ and /..$
Alan
Quoted 11 lines · Alan Cox
Quoted 6 lines · On Tue, 18 Dec 2012 13:04:49 -0800 Kees Cook <[email protected]> wrote:
Some devices have configurable firmware locations. If these configuration mechanisms are exposed to unprivileged userspace, it may be possible to load firmware from an unexpected location. To minimize the risk of this, make sure the string "../" does not appear in the firmware name. This means that neither the users of
request_firmware, nor the uevent handler have to do this filtering themselves.Have you verified no existing distribution ever does this ?
As far as I've found, the only non-static firmware loader I've found is on Chrome OS.
Quoted 7 lines
Quoted 1 line
+ if (
strstr(name, "../")) {and this is obviously buggy and hasn't been tested
Hint foo/bar../baz
You need to check for ^../ /../ and /..$
The distros that handle the firmware uevent all prepend the string "/lib/firmware/".
/lib/firmware/ + "../anything" will leave the directory. /lib/firmware/ + "anything/../anything" can potentially leave the directory. /lib/firmware/ + "anything/.." can potentially leave the directory, but does not refer to a file.
Only "../" needs to be filtered. Given what a tiny corner-case this is (non-root-control over the firmware filename path), I feel it is sufficient.
The alternative is for the uevent handler to do this filtering, but it seemed more sensible to have the kernel not feed it a crazy string in the first place.
-Kees
--
Kees Cook
Chrome OS Security