sd-device: refuse properties with invalid characters (#41001)

src/libsystemd/sd-device/sd-device.c

@@ -30,6 +30,7 @@
 #include "string-util.h"
 #include "strv.h"
 #include "time-util.h"
+#include "utf8.h"
 
 int device_new_aux(sd_device **ret) {
         sd_device *device;
@@ -81,18 +82,40 @@ static sd_device* device_free(sd_device *device) {
 
 DEFINE_PUBLIC_TRIVIAL_REF_UNREF_FUNC(sd_device, sd_device, device_free);
 
+static bool property_is_valid(const char *key, const char *value) {
+        /* Device properties may be saved to database file, then may be parsed from the file. When if a
+         * property contains spurious characters, then the parser may be confused. Let's refuse spurious
+         * properties, even if it is internal, which will not be saved to database file, for consistency. */
+
+        if (isempty(key) || !in_charset(key, ALPHANUMERICAL "_."))
+                return false;
+
+        /* an empty value means unset the property, hence that's fine. */
+        if (isempty(value))
+                return true;
+
+        /* refuse invalid UTF8 and control characters */
+        if (!utf8_is_valid(value) || string_has_cc(value, /* ok= */ NULL))
+                return false;
+
+        return true;
+}
+
 int device_add_property_aux(sd_device *device, const char *key, const char *value, bool db) {
         OrderedHashmap **properties;
 
         assert(device);
         assert(key);
 
+        if (!property_is_valid(key, value))
+                return -EINVAL;
+
         if (db)
                 properties = &device->properties_db;
         else
                 properties = &device->properties;
 
-        if (value) {
+        if (!isempty(value)) {
                 _unused_ _cleanup_free_ char *old_value = NULL;
                 _cleanup_free_ char *new_key = NULL, *new_value = NULL, *old_key = NULL;
                 int r;
@@ -1628,7 +1651,6 @@ bool device_has_devlink(sd_device *device, const char *devlink) {
 static int device_add_property_internal_from_string(sd_device *device, const char *str) {
         _cleanup_free_ char *key = NULL;
         char *value;
-        int r;
 
         assert(device);
         assert(str);
@@ -1648,11 +1670,7 @@ static int device_add_property_internal_from_string(sd_device *device, const cha
 
         /* Add the property to both sd_device::properties and sd_device::properties_db,
          * as this is called by only handle_db_line(). */
-        r = device_add_property_aux(device, key, value, false);
-        if (r < 0)
-                return r;
-
-        return device_add_property_aux(device, key, value, true);
+        return device_add_property(device, key, value);
 }
 
 int device_set_usec_initialized(sd_device *device, usec_t when) {

src/libsystemd/sd-device/test-sd-device.c

@@ -841,6 +841,54 @@ TEST(devname_from_devnum) {
         }
 }
 
+TEST(device_add_property) {
+        _cleanup_(sd_device_unrefp) sd_device *dev = NULL;
+        const char *val;
+
+        ASSERT_OK(sd_device_new_from_syspath(&dev, "/sys/class/net/lo"));
+
+        /* add a property */
+        ASSERT_OK(device_add_property(dev, "hoge", "foo"));
+        ASSERT_OK(sd_device_get_property_value(dev, "hoge", &val));
+        ASSERT_STREQ(val, "foo");
+
+        /* update an existing property */
+        ASSERT_OK(device_add_property(dev, "hoge", "bar"));
+        ASSERT_OK(sd_device_get_property_value(dev, "hoge", &val));
+        ASSERT_STREQ(val, "bar");
+
+        /* remove an existing property */
+        ASSERT_OK(device_add_property(dev, "hoge", NULL));
+        ASSERT_ERROR(sd_device_get_property_value(dev, "hoge", &val), ENOENT);
+
+        /* add a property again */
+        ASSERT_OK(device_add_property(dev, "hoge", "foo"));
+        ASSERT_OK(sd_device_get_property_value(dev, "hoge", &val));
+        ASSERT_STREQ(val, "foo");
+
+        /* remove it with an empty string */
+        ASSERT_OK(device_add_property(dev, "hoge", ""));
+        ASSERT_ERROR(sd_device_get_property_value(dev, "hoge", &val), ENOENT);
+
+        /* check internal property (starting with dot) */
+        ASSERT_OK(device_add_property(dev, ".hoge", "baz"));
+        ASSERT_OK(sd_device_get_property_value(dev, ".hoge", &val));
+        ASSERT_STREQ(val, "baz");
+
+        /* refuse invalid property names */
+        ASSERT_ERROR(device_add_property(dev, "hoge-hoge", "aaa"), EINVAL);
+        ASSERT_ERROR(device_add_property(dev, "hoge=hoge", "aaa"), EINVAL);
+        ASSERT_ERROR(device_add_property(dev, "hoge hoge", "aaa"), EINVAL);
+        ASSERT_ERROR(device_add_property(dev, "hoge\nhoge", "aaa"), EINVAL);
+        ASSERT_ERROR(device_add_property(dev, "hoge\rhoge", "aaa"), EINVAL);
+        ASSERT_ERROR(device_add_property(dev, "hoge\thoge", "aaa"), EINVAL);
+
+        /* refuse invalid property values */
+        ASSERT_ERROR(device_add_property(dev, "hoge", "aaa\naaa"), EINVAL);
+        ASSERT_ERROR(device_add_property(dev, "hoge", "aaa\raaa"), EINVAL);
+        ASSERT_ERROR(device_add_property(dev, "hoge", "aaa\taaa"), EINVAL);
+}
+
 static int intro(void) {
         int r;
 

src/udev/udev-builtin.c

@@ -139,13 +139,20 @@ int udev_builtin_add_property(UdevEvent *event, const char *key, const char *val
 
         assert(key);
 
+        val = empty_to_null(val);
+
         r = device_add_property(dev, key, val);
         if (r < 0)
-                return log_device_debug_errno(dev, r, "Failed to add property '%s%s%s'",
+                return log_device_debug_errno(dev, r, "Failed to %s property '%s%s%s'",
+                                              val ? "add" : "remove",
                                               key, val ? "=" : "", strempty(val));
 
-        if (event->event_mode == EVENT_UDEVADM_TEST_BUILTIN)
-                printf("%s=%s\n", key, strempty(val));
+        if (event->event_mode == EVENT_UDEVADM_TEST_BUILTIN) {
+                if (val)
+                        printf("%s=%s\n", key, val);
+                else
+                        printf("%s (removed)\n", key);
+        }
 
         return 0;
 }