evdev: strip the device name of format directives
authorPeter Hutterer <peter.hutterer@who-t.net>
Tue, 29 Mar 2022 23:25:22 +0000 (09:25 +1000)
committerPeter Hutterer <peter.hutterer@who-t.net>
Wed, 20 Apr 2022 03:32:31 +0000 (13:32 +1000)
This fixes a format string vulnerabilty.

evdev_log_message() composes a format string consisting of a fixed
prefix (including the rendered device name) and the passed-in format
buffer. This format string is then passed with the arguments to the
actual log handler, which usually and eventually ends up being printf.

If the device name contains a printf-style format directive, these ended
up in the format string and thus get interpreted correctly, e.g. for a
device "Foo%sBar" the log message vs printf invocation ends up being:
  evdev_log_message(device, "some message %s", "some argument");
  printf("event9 - Foo%sBar: some message %s", "some argument");

This can enable an attacker to execute malicious code with the
privileges of the process using libinput.

To exploit this, an attacker needs to be able to create a kernel device
with a malicious name, e.g. through /dev/uinput or a Bluetooth device.

To fix this, convert any potential format directives in the device name
by duplicating percentages.

Pre-rendering the device to avoid the issue altogether would be nicer
but the current log level hooks do not easily allow for this. The device
name is the only user-controlled part of the format string.

A second potential issue is the sysname of the device which is also
sanitized.

This issue was found by Albin Eldstål-Ahrens and Benjamin Svensson from
Assured AB, and independently by Lukas Lamster.

Fixes #752

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
meson.build
src/evdev.c
src/evdev.h
src/util-strings.h
test/litest-device-format-string.c [new file with mode: 0644]
test/litest.h
test/test-utils.c

index 97649e5..1407aac 100644 (file)
@@ -734,6 +734,7 @@ if get_option('tests')
                'test/litest-device-dell-canvas-totem-touch.c',
                'test/litest-device-elantech-touchpad.c',
                'test/litest-device-elan-tablet.c',
+               'test/litest-device-format-string.c',
                'test/litest-device-generic-pressurepad.c',
                'test/litest-device-generic-singletouch.c',
                'test/litest-device-gpio-keys.c',
index d0090c8..3c31b59 100644 (file)
@@ -2384,19 +2384,19 @@ evdev_device_create(struct libinput_seat *seat,
        struct libinput *libinput = seat->libinput;
        struct evdev_device *device = NULL;
        int rc;
-       int fd;
+       int fd = -1;
        int unhandled_device = 0;
        const char *devnode = udev_device_get_devnode(udev_device);
-       const char *sysname = udev_device_get_sysname(udev_device);
+       char *sysname = str_sanitize(udev_device_get_sysname(udev_device));
 
        if (!devnode) {
                log_info(libinput, "%s: no device node associated\n", sysname);
-               return NULL;
+               goto err;
        }
 
        if (udev_device_should_be_ignored(udev_device)) {
                log_debug(libinput, "%s: device is ignored\n", sysname);
-               return NULL;
+               goto err;
        }
 
        /* Use non-blocking mode so that we can loop on read on
@@ -2410,13 +2410,15 @@ evdev_device_create(struct libinput_seat *seat,
                         sysname,
                         devnode,
                         strerror(-fd));
-               return NULL;
+               goto err;
        }
 
        if (!evdev_device_have_same_syspath(udev_device, fd))
                goto err;
 
        device = zalloc(sizeof *device);
+       device->sysname = sysname;
+       sysname = NULL;
 
        libinput_device_init(&device->base, seat);
        libinput_seat_ref(seat);
@@ -2439,6 +2441,9 @@ evdev_device_create(struct libinput_seat *seat,
        device->dispatch = NULL;
        device->fd = fd;
        device->devname = libevdev_get_name(device->evdev);
+       /* the log_prefix_name is used as part of a printf format string and
+        * must not contain % directives, see evdev_log_msg */
+       device->log_prefix_name = str_sanitize(device->devname);
        device->scroll.threshold = 5.0; /* Default may be overridden */
        device->scroll.direction_lock_threshold = 5.0; /* Default may be overridden */
        device->scroll.direction = 0;
@@ -2479,12 +2484,16 @@ evdev_device_create(struct libinput_seat *seat,
        return device;
 
 err:
-       close_restricted(libinput, fd);
-       if (device) {
-               unhandled_device = device->seat_caps == 0;
-               evdev_device_destroy(device);
+       if (fd >= 0) {
+               close_restricted(libinput, fd);
+               if (device) {
+                       unhandled_device = device->seat_caps == 0;
+                       evdev_device_destroy(device);
+               }
        }
 
+       free(sysname);
+
        return unhandled_device ? EVDEV_UNHANDLED_DEVICE :  NULL;
 }
 
@@ -2497,7 +2506,7 @@ evdev_device_get_output(struct evdev_device *device)
 const char *
 evdev_device_get_sysname(struct evdev_device *device)
 {
-       return udev_device_get_sysname(device->udev_device);
+       return device->sysname;
 }
 
 const char *
@@ -3094,6 +3103,8 @@ evdev_device_destroy(struct evdev_device *device)
        if (device->base.group)
                libinput_device_group_unref(device->base.group);
 
+       free(device->log_prefix_name);
+       free(device->sysname);
        free(device->output_name);
        filter_destroy(device->pointer.filter);
        libinput_timer_destroy(&device->scroll.timer);
index c7d130f..980c594 100644 (file)
@@ -169,6 +169,8 @@ struct evdev_device {
        struct udev_device *udev_device;
        char *output_name;
        const char *devname;
+       char *log_prefix_name;
+       char *sysname;
        bool was_removed;
        int fd;
        enum evdev_device_seat_capability seat_caps;
@@ -786,7 +788,7 @@ evdev_log_msg(struct evdev_device *device,
                 sizeof(buf),
                 "%-7s - %s%s%s",
                 evdev_device_get_sysname(device),
-                (priority > LIBINPUT_LOG_PRIORITY_DEBUG) ?  device->devname : "",
+                (priority > LIBINPUT_LOG_PRIORITY_DEBUG) ?  device->log_prefix_name : "",
                 (priority > LIBINPUT_LOG_PRIORITY_DEBUG) ?  ": " : "",
                 format);
 
@@ -824,7 +826,7 @@ evdev_log_msg_ratelimit(struct evdev_device *device,
                 sizeof(buf),
                 "%-7s - %s%s%s",
                 evdev_device_get_sysname(device),
-                (priority > LIBINPUT_LOG_PRIORITY_DEBUG) ?  device->devname : "",
+                (priority > LIBINPUT_LOG_PRIORITY_DEBUG) ?  device->log_prefix_name : "",
                 (priority > LIBINPUT_LOG_PRIORITY_DEBUG) ?  ": " : "",
                 format);
 
index 2a15fab..d5a8414 100644 (file)
@@ -43,6 +43,8 @@
 #include <xlocale.h>
 #endif
 
+#include "util-macros.h"
+
 static inline bool
 streq(const char *str1, const char *str2)
 {
@@ -398,3 +400,31 @@ safe_basename(const char *filename);
 
 char *
 trunkname(const char *filename);
+
+/**
+ * Return a copy of str with all % converted to %% to make the string
+ * acceptable as printf format.
+ */
+static inline char *
+str_sanitize(const char *str)
+{
+       if (!str)
+               return NULL;
+
+       if (!strchr(str, '%'))
+               return strdup(str);
+
+       size_t slen = min(strlen(str), 512);
+       char *sanitized = zalloc(2 * slen + 1);
+       const char *src = str;
+       char *dst = sanitized;
+
+       for (size_t i = 0; i < slen; i++) {
+               if (*src == '%')
+                       *dst++ = '%';
+               *dst++ = *src++;
+       }
+       *dst = '\0';
+
+       return sanitized;
+}
diff --git a/test/litest-device-format-string.c b/test/litest-device-format-string.c
new file mode 100644 (file)
index 0000000..aed15db
--- /dev/null
@@ -0,0 +1,56 @@
+
+/*
+ * Copyright © 2013 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include "config.h"
+
+#include "litest.h"
+#include "litest-int.h"
+
+static struct input_id input_id = {
+       .bustype = 0x3,
+       .vendor = 0x0123,
+       .product = 0x0456,
+};
+
+static int events[] = {
+       EV_KEY, BTN_LEFT,
+       EV_KEY, BTN_RIGHT,
+       EV_KEY, BTN_MIDDLE,
+       EV_REL, REL_X,
+       EV_REL, REL_Y,
+       EV_REL, REL_WHEEL,
+       EV_REL, REL_WHEEL_HI_RES,
+       -1 , -1,
+};
+
+TEST_DEVICE("mouse-format-string",
+       .type = LITEST_MOUSE_FORMAT_STRING,
+       .features = LITEST_RELATIVE | LITEST_BUTTON | LITEST_WHEEL,
+       .interface = NULL,
+
+       .name = "Evil %s %d %x Mouse %p %",
+       .id = &input_id,
+       .absinfo = NULL,
+       .events = events,
+)
index 4982e51..1b1daa9 100644 (file)
@@ -321,6 +321,7 @@ enum litest_device_type {
        LITEST_SYNAPTICS_PRESSUREPAD,
        LITEST_GENERIC_PRESSUREPAD,
        LITEST_WACOM_ISDV4_524C_PEN,
+       LITEST_MOUSE_FORMAT_STRING,
 };
 
 #define LITEST_DEVICELESS      -2
index ca804a9..46444d8 100644 (file)
@@ -1316,6 +1316,31 @@ START_TEST(strstartswith_test)
 }
 END_TEST
 
+START_TEST(strsanitize_test)
+{
+       struct strsanitize_test {
+               const char *string;
+               const char *expected;
+       } tests[] = {
+               { "foobar", "foobar" },
+               { "", "" },
+               { "%", "%%" },
+               { "%%%%", "%%%%%%%%" },
+               { "x %s", "x %%s" },
+               { "x %", "x %%" },
+               { "%sx", "%%sx" },
+               { "%s%s", "%%s%%s" },
+               { NULL, NULL },
+       };
+
+       for (struct strsanitize_test *t = tests; t->string; t++) {
+               char *sanitized = str_sanitize(t->string);
+               ck_assert_str_eq(sanitized, t->expected);
+               free(sanitized);
+       }
+}
+END_TEST
+
 START_TEST(list_test_insert)
 {
        struct list_test {
@@ -1538,6 +1563,7 @@ litest_utils_suite(void)
        tcase_add_test(tc, strstrip_test);
        tcase_add_test(tc, strendswith_test);
        tcase_add_test(tc, strstartswith_test);
+       tcase_add_test(tc, strsanitize_test);
        tcase_add_test(tc, time_conversion);
        tcase_add_test(tc, human_time);