From: Yu Watanabe Date: Sat, 11 Jan 2025 22:03:49 +0000 (+0900) Subject: sd-device: chase sysattr and refuse to read/write files outside of sysfs X-Git-Tag: v258-rc1~1510^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8d89667abaa8b569c2ac9e37752c404cebdfaaaa;p=thirdparty%2Fsystemd.git sd-device: chase sysattr and refuse to read/write files outside of sysfs This makes sd_device_get_sysattr_value()/sd_device_set_sysattr_value() refuse to read/write files outside of sysfs for safety. Also this makes - use chase() to resolve and open the symlink in path to sysfs attribute, - use delete_trailing_chars(), - include error code in cache entry, so we can cache more error cases, - refuse caching value written to uevent file of any devices, i.e. sd_device_set_sysattr_value(dev, "../uevent", "add") will also not cache the value "add". --- diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index eab54203f0d..11a27b01d6f 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -9,6 +9,7 @@ #include "sd-device.h" +#include "chase.h" #include "macro.h" int device_new_from_mode_and_devnum(sd_device **ret, mode_t mode, dev_t devnum); @@ -31,8 +32,9 @@ int device_get_devnode_mode(sd_device *device, mode_t *ret); int device_get_devnode_uid(sd_device *device, uid_t *ret); int device_get_devnode_gid(sd_device *device, gid_t *ret); +int device_chase(sd_device *device, const char *path, ChaseFlags flags, char **ret_resolved, int *ret_fd); void device_clear_sysattr_cache(sd_device *device); -int device_cache_sysattr_value(sd_device *device, const char *key, char *value); +int device_cache_sysattr_value(sd_device *device, char *key, char *value, int error); int device_get_cached_sysattr_value(sd_device *device, const char *key, const char **ret_value); void device_seal(sd_device *device); diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index a608ae326f4..2ca5a677867 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -2326,134 +2326,215 @@ void device_clear_sysattr_cache(sd_device *device) { device->sysattr_values = hashmap_free(device->sysattr_values); } -int device_cache_sysattr_value(sd_device *device, const char *key, char *value) { - _unused_ _cleanup_free_ char *old_value = NULL; - _cleanup_free_ char *new_key = NULL; +typedef struct SysAttrCacheEntry { + char *key; + char *value; + int error; +} SysAttrCacheEntry; + +static SysAttrCacheEntry* sysattr_cache_entry_free(SysAttrCacheEntry *p) { + if (!p) + return NULL; + + free(p->key); + free(p->value); + return mfree(p); +} + +DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR( + sysattr_cache_hash_ops, + char, path_hash_func, path_compare, + SysAttrCacheEntry, sysattr_cache_entry_free); + +int device_cache_sysattr_value(sd_device *device, char *key, char *value, int error) { int r; assert(device); assert(key); + assert(value || error > 0); - /* This takes the reference of the input value. The input value may be NULL. - * This replaces the value if it already exists. */ + /* This takes the reference of the input arguments on success. The input value may be NULL. + * This replaces an already existing entry. */ - /* First, remove the old cache entry. So, we do not need to clear cache on error. */ - old_value = hashmap_remove2(device->sysattr_values, key, (void **) &new_key); - if (!new_key) { - new_key = strdup(key); - if (!new_key) - return -ENOMEM; - } + /* Remove the old cache entry. So, we do not need to clear cache on error. */ + sysattr_cache_entry_free(hashmap_remove(device->sysattr_values, key)); + + /* We use ENOANO as a recognizable error code when we have not read the attribute. */ + if (error == ENOANO) + error = ESTALE; + + _cleanup_free_ SysAttrCacheEntry *entry = new(SysAttrCacheEntry, 1); + if (!entry) + return -ENOMEM; - r = hashmap_ensure_put(&device->sysattr_values, &path_hash_ops_free_free, new_key, value); + *entry = (SysAttrCacheEntry) { + .key = key, + .value = value, + .error = error, + }; + + r = hashmap_ensure_put(&device->sysattr_values, &sysattr_cache_hash_ops, entry->key, entry); if (r < 0) return r; - TAKE_PTR(new_key); - + TAKE_PTR(entry); return 0; } int device_get_cached_sysattr_value(sd_device *device, const char *key, const char **ret_value) { - const char *k = NULL, *value; + SysAttrCacheEntry *entry; assert(device); assert(key); - value = hashmap_get2(device->sysattr_values, key, (void **) &k); - if (!k) - return -ESTALE; /* We have not read the attribute. */ - if (!value) - return -ENOENT; /* We have looked up the attribute before and it did not exist. */ + entry = hashmap_get(device->sysattr_values, key); + if (!entry) + return -ENOANO; /* We have not read the attribute. */ + if (!entry->value) { + /* We have looked up the attribute before and failed. Return the cached error code. */ + assert(entry->error > 0); + return -entry->error; + } if (ret_value) - *ret_value = value; + *ret_value = entry->value; return 0; } -/* We cache all sysattr lookups. If an attribute does not exist, it is stored - * with a NULL value in the cache, otherwise the returned string is stored */ -_public_ int sd_device_get_sysattr_value(sd_device *device, const char *sysattr, const char **ret_value) { - _cleanup_free_ char *value = NULL, *path = NULL; - const char *syspath; - struct stat statbuf; +int device_chase(sd_device *device, const char *path, ChaseFlags flags, char **ret_resolved, int *ret_fd) { int r; - assert_return(device, -EINVAL); - assert_return(sysattr, -EINVAL); + assert(device); + assert(path); - /* look for possibly already cached result */ - r = device_get_cached_sysattr_value(device, sysattr, ret_value); - if (r != -ESTALE) + const char *syspath; + r = sd_device_get_syspath(device, &syspath); + if (r < 0) return r; - r = sd_device_get_syspath(device, &syspath); + /* Here, CHASE_PREFIX_ROOT is borrowed. If the flag is set or the specified path is relative, then + * the path will be prefixed with the syspath. Note, we do not pass CHASE_PREFIX_ROOT flag with + * syspath as root to chase(), but we manually concatenate the specified path with syspath before + * calling chase(). Otherwise, we cannot set/get attributes of parent or sibling devices. */ + _cleanup_free_ char *prefixed = NULL; + if (FLAGS_SET(flags, CHASE_PREFIX_ROOT) || !path_is_absolute(path)) { + prefixed = path_join(syspath, path); + if (!prefixed) + return -ENOMEM; + path = prefixed; + flags &= ~CHASE_PREFIX_ROOT; + } + + _cleanup_free_ char *resolved = NULL; + _cleanup_close_ int fd = -EBADF; + r = chase(path, /* root = */ NULL, CHASE_NO_AUTOFS | flags, &resolved, ret_fd ? &fd : NULL); if (r < 0) return r; - path = path_join(syspath, sysattr); - if (!path) - return -ENOMEM; + /* Refuse to reading/writing files outside of sysfs. */ + if (!path_startswith(resolved, "/sys/")) + return -EINVAL; - if (lstat(path, &statbuf) < 0) { - int k; + if (ret_resolved) { + /* Always return relative path. */ + r = path_make_relative(syspath, resolved, ret_resolved); + if (r < 0) + return r; + } - r = -errno; + if (ret_fd) + *ret_fd = TAKE_FD(fd); - /* remember that we could not access the sysattr */ - k = device_cache_sysattr_value(device, sysattr, NULL); - if (k < 0) - log_device_debug_errno(device, k, - "sd-device: failed to cache attribute '%s' with NULL, ignoring: %m", - sysattr); + return 0; +} + +_public_ int sd_device_get_sysattr_value(sd_device *device, const char *sysattr, const char **ret_value) { + _cleanup_free_ char *resolved = NULL, *value = NULL; + _cleanup_close_ int fd = -EBADF; + int r; + + assert_return(device, -EINVAL); + assert_return(sysattr, -EINVAL); + /* Look for possibly already cached result. */ + r = device_get_cached_sysattr_value(device, sysattr, ret_value); + if (r != -ENOANO) return r; - } else if (S_ISLNK(statbuf.st_mode)) { - /* Some core links return only the last element of the target path, - * these are just values, the paths should not be exposed. */ - if (STR_IN_SET(sysattr, "driver", "subsystem", "module")) { - r = readlink_value(path, &value); - if (r < 0) - return r; - } else - return -EINVAL; - } else if (S_ISDIR(statbuf.st_mode)) - /* skip directories */ - return -EISDIR; - else if (!(statbuf.st_mode & S_IRUSR)) - /* skip non-readable files */ - return -EPERM; - else { - size_t size; - /* Read attribute value, Some attributes contain embedded '\0'. So, it is necessary to - * also get the size of the result. See issue #20025. */ - r = read_full_virtual_file(path, &value, &size); + /* Special cases: read the symlink and return the last component of the value. Some core links return + * only the last element of the target path, these are just values, the paths should not be exposed. */ + if (STR_IN_SET(sysattr, "driver", "subsystem", "module")) { + _cleanup_free_ char *prefixed = NULL; + const char *syspath; + + r = sd_device_get_syspath(device, &syspath); if (r < 0) return r; - /* drop trailing newlines */ - while (size > 0 && strchr(NEWLINE, value[--size])) - value[size] = '\0'; + prefixed = path_join(syspath, sysattr); + if (!prefixed) + return -ENOMEM; + + r = readlink_value(prefixed, &value); + if (r != -EINVAL) /* -EINVAL means the path is not a symlink. */ + goto cache_result; } - /* Unfortunately, we need to return 'const char*' instead of 'char*'. Hence, failure in caching - * sysattr value is critical unlike the other places. */ - r = device_cache_sysattr_value(device, sysattr, value); - if (r < 0) { - log_device_debug_errno(device, r, - "sd-device: failed to cache attribute '%s' with '%s'%s: %m", - sysattr, value, ret_value ? "" : ", ignoring"); - if (ret_value) - return r; + r = device_chase(device, sysattr, CHASE_PREFIX_ROOT, &resolved, &fd); + if (r < 0) + goto cache_result; - return 0; + /* Look for cacheed result again with the resolved path. */ + r = device_get_cached_sysattr_value(device, resolved, ret_value); + if (r != -ENOANO) + return r; + + /* Read attribute value, Some attributes contain embedded '\0'. So, it is necessary to also get the + * size of the result. See issue #20025. */ + size_t size; + r = read_virtual_file_fd(fd, SIZE_MAX, &value, &size); + if (r < 0) + goto cache_result; + + delete_trailing_chars(value, NEWLINE); + r = 0; + +cache_result: + if (r == -ENOMEM) + return r; /* Do not cache -ENOMEM, as the failure may be transient. */ + + if (!resolved) { + /* If we have not or could not chase the path, assume 'sysattr' is normalized. */ + resolved = strdup(sysattr); + if (!resolved) + return RET_GATHER(r, -ENOMEM); } - if (ret_value) + int k = device_cache_sysattr_value(device, resolved, value, -r); + if (k < 0) { + if (r < 0) + log_device_debug_errno(device, k, + "sd-device: failed to cache error code (%i) in reading attribute '%s', ignoring: %m", + -r, resolved); + else { + /* Unfortunately, we need to return 'const char*' instead of 'char*'. Hence, failure in caching + * sysattr value is critical unlike the other places. */ + log_device_debug_errno(device, k, + "sd-device: failed to cache attribute '%s' with '%s'%s: %m", + resolved, value, ret_value ? "" : ", ignoring"); + if (ret_value) + return k; + } + + return r; + } + + if (ret_value && r >= 0) *ret_value = value; + /* device_cache_sysattr_value() takes 'resolved' and 'value' on success. */ + TAKE_PTR(resolved); TAKE_PTR(value); - return 0; + return r; } int device_get_sysattr_int(sd_device *device, const char *sysattr, int *ret_value) { @@ -2527,19 +2608,22 @@ int device_get_sysattr_bool(sd_device *device, const char *sysattr) { return parse_boolean(value); } -static void device_remove_cached_sysattr_value(sd_device *device, const char *_key) { - _cleanup_free_ char *key = NULL; +static int device_remove_cached_sysattr_value(sd_device *device, const char *sysattr) { + int r; assert(device); - assert(_key); + assert(sysattr); - free(hashmap_remove2(device->sysattr_values, _key, (void **) &key)); + _cleanup_free_ char *resolved = NULL; + r = device_chase(device, sysattr, CHASE_PREFIX_ROOT | CHASE_NONEXISTENT, &resolved, /* ret_fd = */ NULL); + if (r < 0) + return r; + + sysattr_cache_entry_free(hashmap_remove(device->sysattr_values, resolved)); + return 0; } -_public_ int sd_device_set_sysattr_value(sd_device *device, const char *sysattr, const char *_value) { - _cleanup_free_ char *value = NULL, *path = NULL; - const char *syspath; - size_t len; +_public_ int sd_device_set_sysattr_value(sd_device *device, const char *sysattr, const char *value) { int r; assert_return(device, -EINVAL); @@ -2547,52 +2631,47 @@ _public_ int sd_device_set_sysattr_value(sd_device *device, const char *sysattr, /* Set the attribute and save it in the cache. */ - if (!_value) { + if (!value) /* If input value is NULL, then clear cache and not write anything. */ - device_remove_cached_sysattr_value(device, sysattr); - return 0; - } + return device_remove_cached_sysattr_value(device, sysattr); - r = sd_device_get_syspath(device, &syspath); - if (r < 0) + _cleanup_free_ char *resolved = NULL; + _cleanup_close_ int fd = -EBADF; + r = device_chase(device, sysattr, CHASE_PREFIX_ROOT, &resolved, &fd); + if (r < 0) { + /* On failure, clear cache entry, hopefully, 'sysattr' is normalized. */ + sysattr_cache_entry_free(hashmap_remove(device->sysattr_values, sysattr)); return r; + } - path = path_join(syspath, sysattr); - if (!path) + /* value length is limited to 4k */ + _cleanup_free_ char *copied = strndup(value, 4096); + if (!copied) return -ENOMEM; - len = strlen(_value); - /* drop trailing newlines */ - while (len > 0 && strchr(NEWLINE, _value[len - 1])) - len--; - - /* value length is limited to 4k */ - if (len > 4096) - return -EINVAL; - - value = strndup(_value, len); - if (!value) - return -ENOMEM; + delete_trailing_chars(copied, NEWLINE); - r = write_string_file(path, value, WRITE_STRING_FILE_DISABLE_BUFFER | WRITE_STRING_FILE_NOFOLLOW); + r = write_string_file_fd(fd, copied, WRITE_STRING_FILE_DISABLE_BUFFER | WRITE_STRING_FILE_AVOID_NEWLINE); if (r < 0) { /* On failure, clear cache entry, as we do not know how it fails. */ - device_remove_cached_sysattr_value(device, sysattr); + sysattr_cache_entry_free(hashmap_remove(device->sysattr_values, resolved)); return r; } /* Do not cache action string written into uevent file. */ - if (streq(sysattr, "uevent")) + if (streq(last_path_component(resolved), "uevent")) return 0; - r = device_cache_sysattr_value(device, sysattr, value); + r = device_cache_sysattr_value(device, resolved, copied, 0); if (r < 0) log_device_debug_errno(device, r, - "sd-device: failed to cache attribute '%s' with '%s', ignoring: %m", - sysattr, value); - else - TAKE_PTR(value); + "sd-device: failed to cache written attribute '%s' with '%s', ignoring: %m", + resolved, copied); + else { + TAKE_PTR(resolved); + TAKE_PTR(copied); + } return 0; } @@ -2605,10 +2684,8 @@ _public_ int sd_device_set_sysattr_valuef(sd_device *device, const char *sysattr assert_return(device, -EINVAL); assert_return(sysattr, -EINVAL); - if (!format) { - device_remove_cached_sysattr_value(device, sysattr); - return 0; - } + if (!format) + return device_remove_cached_sysattr_value(device, sysattr); va_start(ap, format); r = vasprintf(&value, format, ap);