Avoid buffer overflow in uevent_sender_send()

action, devpath, and other values are unbounded, so ensure that we don't
overflow the property buffer. This does not cross a security boundary
and is not typically used with untrusted data, so not really a
vulnerability. But show a proper error message instead of crashing if it
happens.

Thanks to Simon Schmitt <[email protected]> for pointing this out!

Author: martinpitt

src/uevent_sender.c

@@ -194,6 +194,22 @@ string_hash32(const char *str)
     return h;
 }
 
+static ssize_t
+append_property(char *array, size_t size, ssize_t offset, const char *name, const char *value)
+{
+    int r;
+    assert(offset < size);
+    r = snprintf(array + offset, size - offset, "%s%s", name, value);
+    // include the NUL terminator in the string length, as we need to keep it as a separator between keys
+    ++r;
+    if (r + offset >= size) {
+        fprintf(stderr, "ERROR: uevent_sender_send: Property buffer overflow\n");
+        abort();
+     }
+
+    return r;
+}
+
 /* this mirrors the code from systemd/src/libudev/libudev-monitor.c,
  * udev_monitor_send_device() */
 void
@@ -206,6 +222,7 @@ uevent_sender_send(uevent_sender * sender, const char *devpath, const char *acti
     const char *subsystem;
     const char *devname;
     const char *devtype;
+    char seqnumstr[20];
     struct udev_device *device;
     struct udev_monitor_netlink_header nlh;
     static unsigned long long seqnum = 1;
@@ -224,27 +241,15 @@ uevent_sender_send(uevent_sender * sender, const char *devpath, const char *acti
 
     /* build NUL-terminated property array */
     count = 0;
-    strcpy(props, "ACTION=");
-    strcat(props, action);
-    count += strlen(props) + 1;
-    strcpy(props + count, "DEVPATH=");
-    strcat(props + count, udev_device_get_devpath(device));
-    count += strlen(props + count) + 1;
-    strcpy(props + count, "SUBSYSTEM=");
-    strcat(props + count, subsystem);
-    count += strlen(props + count) + 1;
-    snprintf(props + count, sizeof(props) - count, "SEQNUM=%llu", seqnum++);
-    count += strlen(props + count) + 1;
-    if (devname) {
-        strcpy(props + count, "DEVNAME=");
-        strcat(props + count, devname);
-        count += strlen(props + count) + 1;
-    }
-    if (devtype) {
-        strcpy(props + count, "DEVTYPE=");
-        strcat(props + count, devtype);
-        count += strlen(props + count) + 1;
-    }
+    count += append_property(props, sizeof (props), count, "ACTION=", action);
+    count += append_property(props, sizeof (props), count, "DEVPATH=", udev_device_get_devpath(device));
+    count += append_property(props, sizeof (props), count, "SUBSYSTEM=", subsystem);
+    snprintf(seqnumstr, sizeof(seqnumstr), "%llu", seqnum++);
+    count += append_property(props, sizeof (props), count, "SEQNUM=", seqnumstr);
+    if (devname)
+        count += append_property(props, sizeof (props), count, "DEVNAME=", devname);
+    if (devtype)
+        count += append_property(props, sizeof (props), count, "DEVTYPE=", devtype);
 
     /* add versioned header */
     memset(&nlh, 0x00, sizeof(struct udev_monitor_netlink_header));

tests/test-umockdev.c

@@ -771,6 +771,46 @@ t_testbed_uevent_error(UMockdevTestbedFixture * fixture, gconstpointer data)
     udev_unref(udev);
 }
 
+static void
+t_testbed_uevent_null_action(UMockdevTestbedFixture * fixture, gconstpointer data)
+{
+    gchar *syspath;
+
+    syspath = umockdev_testbed_add_device(fixture->testbed, "pci", "mydev", NULL, NULL, NULL);
+    g_assert(syspath);
+
+    if (g_test_subprocess()) {
+        umockdev_testbed_uevent(fixture->testbed, syspath, NULL);
+    }
+    g_test_trap_subprocess(NULL, 0, 0);
+    g_test_trap_assert_failed();
+    g_test_trap_assert_stderr ("*action != NULL*");
+
+    g_free(syspath);
+}
+
+static void
+t_testbed_uevent_action_overflow(UMockdevTestbedFixture * fixture, gconstpointer data)
+{
+    gchar *syspath;
+
+    syspath = umockdev_testbed_add_device(fixture->testbed, "pci", "mydev", NULL, NULL, NULL);
+    g_assert(syspath);
+
+    /* overly long action */
+    if (g_test_subprocess()) {
+        char long_action[4096];
+        memset(long_action, 'a', sizeof(long_action));
+        long_action[sizeof(long_action)-1] = '\0';
+        umockdev_testbed_uevent(fixture->testbed, syspath, long_action);
+    }
+    g_test_trap_subprocess(NULL, 0, 0);
+    g_test_trap_assert_failed();
+    g_test_trap_assert_stderr ("*uevent_sender_send*Property buffer overflow*");
+
+    g_free(syspath);
+}
+
 static void
 t_testbed_add_from_string(UMockdevTestbedFixture * fixture, gconstpointer data)
 {
@@ -2056,6 +2096,10 @@ main(int argc, char **argv)
 	       t_testbed_uevent_no_listener, t_testbed_fixture_teardown);
     g_test_add("/umockdev-testbed/uevent/error", UMockdevTestbedFixture, NULL, t_testbed_fixture_setup,
 	       t_testbed_uevent_error, t_testbed_fixture_teardown);
+    g_test_add("/umockdev-testbed/uevent/null_action", UMockdevTestbedFixture, NULL, t_testbed_fixture_setup,
+	       t_testbed_uevent_null_action, t_testbed_fixture_teardown);
+    g_test_add("/umockdev-testbed/uevent/action_overflow", UMockdevTestbedFixture, NULL, t_testbed_fixture_setup,
+	       t_testbed_uevent_action_overflow, t_testbed_fixture_teardown);
 
     /* tests for mocking USB devices */
     g_test_add("/umockdev-testbed-usb/lsusb", UMockdevTestbedFixture, NULL, t_testbed_fixture_setup,