From: kay.sievers@vrfy.org Date: Sat, 26 Mar 2005 23:15:07 +0000 (+0100) Subject: [PATCH] remove untrusted chars read from sysfs-values or returned by PROGRAM X-Git-Tag: 057~26 X-Git-Url: https://err.no/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=18614ab25d4208749a3d85ced33acc6679c60fce;p=systemd [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 50ea5858..5d7c5e5b 100644 --- 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 => <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 2b5683fd..37607492 100644 --- 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 b45500dc..5ebc9e5b 100644 --- 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);