From: Lennart Poettering Date: Fri, 12 Dec 2014 01:32:33 +0000 (+0100) Subject: util: when using basename() for creating temporary files, verify the resulting name... X-Git-Tag: v219~1041 X-Git-Url: https://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?p=elogind.git;a=commitdiff_plain;h=ae6c3cc009a21df4b51851fb8fe3fde0b7d6d8f0 util: when using basename() for creating temporary files, verify the resulting name is actually valid Also, rename filename_is_safe() to filename_is_valid(), since it actually does a full validation for what the kernel will accept as file name, it's not just a heuristic. --- diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 259323bd5..3fbe680cf 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3066,7 +3066,7 @@ int config_parse_runtime_directory( if (!n) return log_oom(); - if (!filename_is_safe(n)) { + if (!filename_is_valid(n)) { log_syntax(unit, LOG_ERR, filename, line, EINVAL, "Runtime directory is not valid, ignoring assignment: %s", rvalue); continue; diff --git a/src/hostname/hostnamed.c b/src/hostname/hostnamed.c index 970e80076..ef45e563c 100644 --- a/src/hostname/hostnamed.c +++ b/src/hostname/hostnamed.c @@ -552,7 +552,7 @@ static int set_machine_info(Context *c, sd_bus *bus, sd_bus_message *m, int prop /* The icon name might ultimately be used as file * name, so better be safe than sorry */ - if (prop == PROP_ICON_NAME && !filename_is_safe(name)) + if (prop == PROP_ICON_NAME && !filename_is_valid(name)) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid icon name '%s'", name); if (prop == PROP_PRETTY_HOSTNAME && string_has_cc(name, NULL)) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid pretty host name '%s'", name); diff --git a/src/journal/coredump.c b/src/journal/coredump.c index be45a684e..8678ec6a5 100644 --- a/src/journal/coredump.c +++ b/src/journal/coredump.c @@ -306,9 +306,9 @@ static int save_external_coredump( if (r < 0) return log_error_errno(r, "Failed to determine coredump file name: %m"); - tmp = tempfn_random(fn); - if (!tmp) - return log_oom(); + r = tempfn_random(fn, &tmp); + if (r < 0) + return log_error_errno(r, "Failed to determine temporary file name: %m"); mkdir_p_label("/var/lib/systemd/coredump", 0755); @@ -352,9 +352,9 @@ static int save_external_coredump( goto uncompressed; } - tmp_compressed = tempfn_random(fn_compressed); - if (!tmp_compressed) { - log_oom(); + r = tempfn_random(fn_compressed, &tmp_compressed); + if (r < 0) { + log_error_errno(r, "Failed to determine temporary file name for %s: %m", fn_compressed); goto uncompressed; } diff --git a/src/journal/journald-native.c b/src/journal/journald-native.c index f98269625..f701233bb 100644 --- a/src/journal/journald-native.c +++ b/src/journal/journald-native.c @@ -350,7 +350,7 @@ void server_process_native_file( return; } - if (!filename_is_safe(e)) { + if (!filename_is_valid(e)) { log_error("Received file in subdirectory of allowed directories. Refusing."); return; } diff --git a/src/locale/localed.c b/src/locale/localed.c index 8d60d0ff1..0aaa63de8 100644 --- a/src/locale/localed.c +++ b/src/locale/localed.c @@ -965,8 +965,8 @@ static int method_set_vc_keyboard(sd_bus *bus, sd_bus_message *m, void *userdata if (!streq_ptr(keymap, c->vc_keymap) || !streq_ptr(keymap_toggle, c->vc_keymap_toggle)) { - if ((keymap && (!filename_is_safe(keymap) || !string_is_safe(keymap))) || - (keymap_toggle && (!filename_is_safe(keymap_toggle) || !string_is_safe(keymap_toggle)))) + if ((keymap && (!filename_is_valid(keymap) || !string_is_safe(keymap))) || + (keymap_toggle && (!filename_is_valid(keymap_toggle) || !string_is_safe(keymap_toggle)))) return sd_bus_error_set_errnof(error, -EINVAL, "Received invalid keymap data"); r = bus_verify_polkit_async(m, CAP_SYS_ADMIN, "org.freedesktop.locale1.set-keyboard", interactive, &c->polkit_registry, error); diff --git a/src/shared/dropin.c b/src/shared/dropin.c index ac09be984..c5c4f9571 100644 --- a/src/shared/dropin.c +++ b/src/shared/dropin.c @@ -43,7 +43,7 @@ int drop_in_file(const char *dir, const char *unit, unsigned level, if (!b) return -ENOMEM; - if (!filename_is_safe(b)) + if (!filename_is_valid(b)) return -EINVAL; p = strjoin(dir, "/", unit, ".d", NULL); diff --git a/src/shared/locale-util.c b/src/shared/locale-util.c index 9addb05f0..61db9a812 100644 --- a/src/shared/locale-util.c +++ b/src/shared/locale-util.c @@ -195,7 +195,7 @@ bool locale_is_valid(const char *name) { if (!utf8_is_valid(name)) return false; - if (!filename_is_safe(name)) + if (!filename_is_valid(name)) return false; if (!string_is_safe(name)) diff --git a/src/shared/util.c b/src/shared/util.c index 6383c0f80..273552f62 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -4284,15 +4284,15 @@ int fd_wait_for_event(int fd, int event, usec_t t) { int fopen_temporary(const char *path, FILE **_f, char **_temp_path) { FILE *f; char *t; - int fd; + int r, fd; assert(path); assert(_f); assert(_temp_path); - t = tempfn_xxxxxx(path); - if (!t) - return -ENOMEM; + r = tempfn_xxxxxx(path, &t); + if (r < 0) + return r; fd = mkostemp_safe(t, O_WRONLY|O_CLOEXEC); if (fd < 0) { @@ -4403,13 +4403,14 @@ int vt_disallocate(const char *name) { int symlink_atomic(const char *from, const char *to) { _cleanup_free_ char *t = NULL; + int r; assert(from); assert(to); - t = tempfn_random(to); - if (!t) - return -ENOMEM; + r = tempfn_random(to, &t); + if (r < 0) + return r; if (symlink(from, t) < 0) return -errno; @@ -4424,12 +4425,13 @@ int symlink_atomic(const char *from, const char *to) { int mknod_atomic(const char *path, mode_t mode, dev_t dev) { _cleanup_free_ char *t = NULL; + int r; assert(path); - t = tempfn_random(path); - if (!t) - return -ENOMEM; + r = tempfn_random(path, &t); + if (r < 0) + return r; if (mknod(t, mode, dev) < 0) return -errno; @@ -4444,12 +4446,13 @@ int mknod_atomic(const char *path, mode_t mode, dev_t dev) { int mkfifo_atomic(const char *path, mode_t mode) { _cleanup_free_ char *t = NULL; + int r; assert(path); - t = tempfn_random(path); - if (!t) - return -ENOMEM; + r = tempfn_random(path, &t); + if (r < 0) + return r; if (mkfifo(t, mode) < 0) return -errno; @@ -5561,7 +5564,7 @@ int get_shell(char **_s) { return 0; } -bool filename_is_safe(const char *p) { +bool filename_is_valid(const char *p) { if (isempty(p)) return false; @@ -6963,42 +6966,45 @@ int fflush_and_check(FILE *f) { return 0; } -char *tempfn_xxxxxx(const char *p) { +int tempfn_xxxxxx(const char *p, char **ret) { const char *fn; char *t; - size_t k; assert(p); + assert(ret); + + fn = basename(p); + if (!filename_is_valid(fn)) + return -EINVAL; t = new(char, strlen(p) + 1 + 6 + 1); if (!t) - return NULL; - - fn = basename(p); - k = fn - p; + return -ENOMEM; - strcpy(stpcpy(stpcpy(mempcpy(t, p, k), "."), fn), "XXXXXX"); + strcpy(stpcpy(stpcpy(mempcpy(t, p, fn - p), "."), fn), "XXXXXX"); - return t; + *ret = t; + return 0; } -char *tempfn_random(const char *p) { +int tempfn_random(const char *p, char **ret) { const char *fn; char *t, *x; uint64_t u; - size_t k; unsigned i; assert(p); + assert(ret); + + fn = basename(p); + if (!filename_is_valid(fn)) + return -EINVAL; t = new(char, strlen(p) + 1 + 16 + 1); if (!t) - return NULL; - - fn = basename(p); - k = fn - p; + return -ENOMEM; - x = stpcpy(stpcpy(mempcpy(t, p, k), "."), fn); + x = stpcpy(stpcpy(mempcpy(t, p, fn - p), "."), fn); u = random_u64(); for (i = 0; i < 16; i++) { @@ -7008,7 +7014,8 @@ char *tempfn_random(const char *p) { *x = 0; - return t; + *ret = t; + return 0; } /* make sure the hostname is not "localhost" */ diff --git a/src/shared/util.h b/src/shared/util.h index 73bd9012f..a15ce95a6 100644 --- a/src/shared/util.h +++ b/src/shared/util.h @@ -714,7 +714,7 @@ _alloc_(2, 3) static inline void *memdup_multiply(const void *p, size_t a, size_ return memdup(p, a * b); } -bool filename_is_safe(const char *p) _pure_; +bool filename_is_valid(const char *p) _pure_; bool path_is_safe(const char *p) _pure_; bool string_is_safe(const char *p) _pure_; bool string_has_cc(const char *p, const char *ok) _pure_; @@ -1015,8 +1015,8 @@ int bind_remount_recursive(const char *prefix, bool ro); int fflush_and_check(FILE *f); -char *tempfn_xxxxxx(const char *p); -char *tempfn_random(const char *p); +int tempfn_xxxxxx(const char *p, char **ret); +int tempfn_random(const char *p, char **ret); bool is_localhost(const char *hostname); diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 2ceb303f1..5ed430c82 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -5730,16 +5730,16 @@ static int unit_file_find_path(LookupPaths *lp, const char *unit_name, char **un } static int create_edit_temp_file(const char *new_path, const char *original_path, char **ret_tmp_fn) { - int r; char *t; + int r; assert(new_path); assert(original_path); assert(ret_tmp_fn); - t = tempfn_random(new_path); - if (!t) - return log_oom(); + r = tempfn_random(new_path, &t); + if (r < 0) + return log_error_errno(r, "Failed to determine temporary filename for %s: %m", new_path); r = mkdir_parents(new_path, 0755); if (r < 0) { diff --git a/src/test/test-util.c b/src/test/test-util.c index fe54586ee..6c7d77b19 100644 --- a/src/test/test-util.c +++ b/src/test/test-util.c @@ -802,24 +802,24 @@ static void test_foreach_string(void) { assert_se(streq(x, "zzz")); } -static void test_filename_is_safe(void) { +static void test_filename_is_valid(void) { char foo[FILENAME_MAX+2]; int i; - assert_se(!filename_is_safe("")); - assert_se(!filename_is_safe("/bar/foo")); - assert_se(!filename_is_safe("/")); - assert_se(!filename_is_safe(".")); - assert_se(!filename_is_safe("..")); + assert_se(!filename_is_valid("")); + assert_se(!filename_is_valid("/bar/foo")); + assert_se(!filename_is_valid("/")); + assert_se(!filename_is_valid(".")); + assert_se(!filename_is_valid("..")); for (i=0; i