[PATCH] remove untrusted chars read from sysfs-values or returned by PROGRAM

Better remove characters that are useless in a device node name.
It may be a security risk to pass any character read from e.g. a
sysfs attribute to a shell script we execute later.

Prevent the modification of the libsysfs attribute value
cache.

Clear PROGRAM result if the execution encountered an error.

diff --git a/test/udev-test.pl b/test/udev-test.pl
index 50ea5858a408c496c04a20e40b3aca7bf8ccb5ed..5d7c5e5b8169c2910a451785b38de694196398d8 100644 (file)
--- a/test/udev-test.pl
+++ b/test/udev-test.pl
@@ -1213,6 +1213,15 @@ BUS=="scsi", KERNEL=="sda1", ENV{ENV_KEY_TEST}=="go", NAME="wrong"
 BUS=="scsi", KERNEL=="sda1", ENV{ENV_KEY_TEST}=="yes", ENV{ACTION}=="add", ENV{DEVPATH}=="/block/sda/sdax1", NAME="no"
 BUS=="scsi", KERNEL=="sda1", ENV{ENV_KEY_TEST}=="test", ENV{ACTION}=="add", ENV{DEVPATH}=="/block/sda/sda1", NAME="true"
 BUS=="scsi", KERNEL=="sda1", ENV{ENV_KEY_TEST}=="bad", NAME="bad"
+EOF
+       },
+       {
+               desc            => "untrusted string sanitize",
+               subsys          => "block",
+               devpath         => "/block/sda/sda1",
+               exp_name        => "sane",
+               rules           => <<EOF
+BUS=="scsi", KERNEL=="sda1", PROGRAM=="/bin/echo -e name; (/sbin/badprogram)", RESULT="name_ _/sbin/badprogram_", NAME="sane"
 EOF
        },
 );

diff --git a/udev_config.c b/udev_config.c
index 54eedb980a19b1db017954529511247ce1e8a8fc..7d6bb77e5bd7a1b28cbd09c9c5fe92615388b3f0 100644 (file)
--- a/udev_config.c
+++ b/udev_config.c
@@ -182,19 +182,19 @@ static int parse_config_file(void)
 
                if (strcasecmp(variable, "udev_root") == 0) {
                        strlcpy(udev_root, value, sizeof(udev_root));
-                       no_trailing_slash(udev_root);
+                       remove_trailing_char(udev_root, '/');
                        continue;
                }
 
                if (strcasecmp(variable, "udev_db") == 0) {
                        strlcpy(udev_db_path, value, sizeof(udev_db_path));
-                       no_trailing_slash(udev_db_path);
+                       remove_trailing_char(udev_db_path, '/');
                        continue;
                }
 
                if (strcasecmp(variable, "udev_rules") == 0) {
                        strlcpy(udev_rules_filename, value, sizeof(udev_rules_filename));
-                       no_trailing_slash(udev_rules_filename);
+                       remove_trailing_char(udev_rules_filename, '/');
                        continue;
                }
 
@@ -232,7 +232,7 @@ void udev_init_config(void)
        env = getenv("UDEV_CONFIG_FILE");
        if (env) {
                strlcpy(udev_config_filename, env, sizeof(udev_config_filename));
-               no_trailing_slash(udev_config_filename);
+               remove_trailing_char(udev_config_filename, '/');
        }
 
        parse_config_file();

diff --git a/udev_rules.c b/udev_rules.c
index 4eb8fd44118bd52896a02011659c4752e2730fe1..2581dc2a78decf91901399653a29ea76d967aa5a 100644 (file)
--- a/udev_rules.c
+++ b/udev_rules.c
@@ -43,7 +43,6 @@
 #include "udev_rules.h"
 #include "udev_db.h"
 
-static struct sysfs_attribute *find_sysfs_attribute(struct sysfs_class_device *class_dev, struct sysfs_device *sysfs_device, char *attr);
 
 /* compare string with pattern (supports * ? [0-9] [!A-Z]) */
 static int strcmp_pattern(const char *p, const char *s)
@@ -167,6 +166,33 @@ static int find_free_number(struct udevice *udev, const char *name)
        }
 }
 
+static int find_sysfs_attribute(struct sysfs_class_device *class_dev, struct sysfs_device *sysfs_device,
+                               const char *name, char *value, size_t len)
+{
+       struct sysfs_attribute *tmpattr;
+
+       dbg("look for device attribute '%s'", name);
+       if (class_dev) {
+               tmpattr = sysfs_get_classdev_attr(class_dev, name);
+               if (tmpattr)
+                       goto attr_found;
+       }
+       if (sysfs_device) {
+               tmpattr = sysfs_get_device_attr(sysfs_device, name);
+               if (tmpattr)
+                       goto attr_found;
+       }
+
+       return -1;
+
+attr_found:
+       strlcpy(value, tmpattr->value, len);
+       remove_trailing_char(value, '\n');
+
+       dbg("found attribute '%s'", tmpattr->path);
+       return 0;
+}
+
 static void apply_format(struct udevice *udev, char *string, size_t maxsize,
                         struct sysfs_class_device *class_dev, struct sysfs_device *sysfs_device)
 {
@@ -176,7 +202,6 @@ static void apply_format(struct udevice *udev, char *string, size_t maxsize,
        int len;
        int i;
        char c;
-       struct sysfs_attribute *tmpattr;
        unsigned int next_free_number;
        struct sysfs_class_device *class_dev_parent;
 
@@ -257,30 +282,21 @@ static void apply_format(struct udevice *udev, char *string, size_t maxsize,
                        }
                        break;
                case 's':
-                       if (!class_dev)
-                               break;
                        if (attr == NULL) {
                                dbg("missing attribute");
                                break;
                        }
-                       tmpattr = find_sysfs_attribute(class_dev, sysfs_device, attr);
-                       if (tmpattr == NULL) {
+                       if (find_sysfs_attribute(class_dev, sysfs_device, attr, temp2, sizeof(temp2)) != 0) {
                                dbg("sysfs attribute '%s' not found", attr);
                                break;
                        }
-                       /* strip trailing whitespace of matching value */
-                       if (isspace(tmpattr->value[strlen(tmpattr->value)-1])) {
-                               i = len = strlen(tmpattr->value);
-                               while (i > 0 &&  isspace(tmpattr->value[i-1]))
-                                       i--;
-                               if (i < len) {
-                                       tmpattr->value[i] = '\0';
-                                       dbg("remove %i trailing whitespace chars from '%s'",
-                                                len - i, tmpattr->value);
-                               }
-                       }
-                       strlcat(string, tmpattr->value, maxsize);
-                       dbg("substitute sysfs value '%s'", tmpattr->value);
+                       /* strip trailing whitespace of sysfs value */
+                       i = strlen(temp2);
+                       while (i > 0 && isspace(temp2[i-1]))
+                               temp2[--i] = '\0';
+                       replace_untrusted_chars(temp2);
+                       strlcat(string, temp2, maxsize);
+                       dbg("substitute sysfs value '%s'", temp2);
                        break;
                case '%':
                        strlcat(string, "%", maxsize);
@@ -385,8 +401,7 @@ static int execute_program(struct udevice *udev, const char *path, char *value,
        pid = fork();
        switch(pid) {
        case 0:
-               /* child */
-               /* dup2 write side of pipe to STDOUT */
+               /* child dup2 write side of pipe to STDOUT */
                dup2(fds[1], STDOUT_FILENO);
                retval = execv(arg, argv);
 
@@ -402,7 +417,12 @@ static int execute_program(struct udevice *udev, const char *path, char *value,
                i = 0;
                while (1) {
                        count = read(fds[0], value + i, len - i-1);
-                       if (count <= 0)
+                       if (count < 0) {
+                               err("read failed with '%s'", strerror(errno));
+                               retval = -1;
+                       }
+
+                       if (count == 0)
                                break;
 
                        i += count;
@@ -412,16 +432,7 @@ static int execute_program(struct udevice *udev, const char *path, char *value,
                                break;
                        }
                }
-
-               if (count < 0) {
-                       err("read failed with '%s'", strerror(errno));
-                       retval = -1;
-               }
-
-               if (i > 0 && value[i-1] == '\n')
-                       i--;
                value[i] = '\0';
-               dbg("result is '%s'", value);
 
                close(fds[0]);
                waitpid(pid, &status, 0);
@@ -431,70 +442,38 @@ static int execute_program(struct udevice *udev, const char *path, char *value,
                        retval = -1;
                }
        }
-       return retval;
-}
-
-static struct sysfs_attribute *find_sysfs_attribute(struct sysfs_class_device *class_dev, struct sysfs_device *sysfs_device, char *attr)
-{
-       struct sysfs_attribute *tmpattr = NULL;
-       char *c;
-
-       dbg("look for device attribute '%s'", attr);
-       /* try to find the attribute in the class device directory */
-       tmpattr = sysfs_get_classdev_attr(class_dev, attr);
-       if (tmpattr)
-               goto attr_found;
 
-       /* look in the class device directory if present */
-       if (sysfs_device) {
-               tmpattr = sysfs_get_device_attr(sysfs_device, attr);
-               if (tmpattr)
-                       goto attr_found;
-       }
-
-       return NULL;
-
-attr_found:
-       c = strchr(tmpattr->value, '\n');
-       if (c != NULL)
-               c[0] = '\0';
+       if (!retval) {
+               remove_trailing_char(value, '\n');
+               dbg("result is '%s'", value);
+               replace_untrusted_chars(value);
+       } else
+               value[0] = '\0';
 
-       dbg("found attribute '%s'", tmpattr->path);
-       return tmpattr;
+       return retval;
 }
 
 static int compare_sysfs_attribute(struct sysfs_class_device *class_dev, struct sysfs_device *sysfs_device, struct key_pair *pair)
 {
-       struct sysfs_attribute *tmpattr;
+       char value[VALUE_SIZE];
        int i;
-       int len;
-
-       if ((pair == NULL) || (pair->name[0] == '\0') || (pair->value == '\0'))
-               return -ENODEV;
 
-       tmpattr = find_sysfs_attribute(class_dev, sysfs_device, pair->name);
-       if (tmpattr == NULL)
-               return -ENODEV;
+       if (find_sysfs_attribute(class_dev, sysfs_device, pair->name, value, sizeof(value)) != 0)
+               return -1;
 
        /* strip trailing whitespace of value, if not asked to match for it */
-       if (! isspace(pair->value[strlen(pair->value)-1])) {
-               i = len = strlen(tmpattr->value);
-               while (i > 0 &&  isspace(tmpattr->value[i-1]))
-                       i--;
-               if (i < len) {
-                       tmpattr->value[i] = '\0';
-                       dbg("remove %i trailing whitespace chars from '%s'",
-                           len - i, tmpattr->value);
-               }
+       if (!isspace(pair->value[strlen(pair->value)-1])) {
+               i = strlen(value);
+               while (i > 0 && isspace(value[i-1]))
+                       value[--i] = '\0';
+               dbg("removed %i trailing whitespace chars from '%s'", strlen(value)-i, value);
        }
 
-       dbg("compare attribute '%s' value '%s' with '%s'",
-                 pair->name, tmpattr->value, pair->value);
-       if (strcmp_pattern(pair->value, tmpattr->value) != 0)
-               return -ENODEV;
+       dbg("compare attribute '%s' value '%s' with '%s'", pair->name, value, pair->value);
+       if (strcmp_pattern(pair->value, value) != 0)
+               return -1;
 
-       dbg("found matching attribute '%s' with value '%s'",
-           pair->name, pair->value);
+       dbg("found matching attribute '%s' with value '%s'", pair->name, pair->value);
        return 0;
 }
 

diff --git a/udev_utils.c b/udev_utils.c
index 2b5683fda65b5801207fb8a878cac167fe32a758..37607492acf906267469b7741c228b2fa5d56a55 100644 (file)
--- a/udev_utils.c
+++ b/udev_utils.c
@@ -50,7 +50,7 @@ int udev_init_device(struct udevice *udev, const char* devpath, const char *subs
 
        if (devpath) {
                strlcpy(udev->devpath, devpath, sizeof(udev->devpath));
-               no_trailing_slash(udev->devpath);
+               remove_trailing_char(udev->devpath, '/');
 
                if (strncmp(udev->devpath, "/block/", 7) == 0)
                        udev->type = DEV_BLOCK;
@@ -228,12 +228,24 @@ size_t buf_get_line(const char *buf, size_t buflen, size_t cur)
        return count - cur;
 }
 
-void no_trailing_slash(char *path)
+void replace_untrusted_chars(char *string)
+{
+       size_t len;
+
+       for (len = 0; string[len] != '\0'; len++) {
+               if (strchr(";,~\\()\'", string[len])) {
+                       info("replace '%c' in '%s'", string[len], string);
+                       string[len] = '_';
+               }
+       }
+}
+
+void remove_trailing_char(char *path, char c)
 {
        size_t len;
 
        len = strlen(path);
-       while (len > 0 && path[len-1] == '/')
+       while (len > 0 && path[len-1] == c)
                path[--len] = '\0';
 }
 

diff --git a/udev_utils.h b/udev_utils.h
index b45500dcff61b63f6d6ff76572b9c672720b41f8..5ebc9e5bb6f830cdeb243cf9c35dccc9d513b0f3 100644 (file)
--- a/udev_utils.h
+++ b/udev_utils.h
@@ -39,7 +39,8 @@ extern int unlink_secure(const char *filename);
 extern int file_map(const char *filename, char **buf, size_t *bufsize);
 extern void file_unmap(char *buf, size_t bufsize);
 extern size_t buf_get_line(const char *buf, size_t buflen, size_t cur);
-extern void no_trailing_slash(char *path);
+extern void remove_trailing_char(char *path, char c);
+extern void replace_untrusted_chars(char *string);
 extern int name_list_add(struct list_head *name_list, const char *name, int sort);
 extern int add_matching_files(struct list_head *name_list, const char *dirname, const char *suffix);