[PATCH v2] firmware: make sure paths remain relative

[PATCH v2] firmware: make sure paths remain relative

[Kees Cook]

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.

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Ming Lei <ming.lei@canonical.com>
---
v2:
 - add dev_err on failure, suggested by Ming Lei.
---
 drivers/base/firmware_class.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d814603..b7e095b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -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",
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2] firmware: make sure paths remain relative

[Alan Cox]

On Tue, 18 Dec 2012 13:04:49 -0800
Kees Cook <keescook@chromium.org> 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 ?


> +	if (strstr(name, "../")) {

and this is obviously buggy and hasn't been tested

Hint foo/bar../baz

You need to check for ^../ /../ and /..$

Alan

Re: [PATCH v2] firmware: make sure paths remain relative

[Kees Cook]

On Tue, Dec 18, 2012 at 2:02 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Tue, 18 Dec 2012 13:04:49 -0800
> Kees Cook <keescook@chromium.org> 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.

>> +     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