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.

11 files changed, 67 insertions, 60 deletions

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<FILENAME_MAX+1; i++)
                 foo[i] = 'a';
         foo[FILENAME_MAX+1] = '\0';
 
-        assert_se(!filename_is_safe(foo));
+        assert_se(!filename_is_valid(foo));
 
-        assert_se(filename_is_safe("foo_bar-333"));
-        assert_se(filename_is_safe("o.o"));
+        assert_se(filename_is_valid("foo_bar-333"));
+        assert_se(filename_is_valid("o.o"));
 }
 
 static void test_string_has_cc(void) {
@@ -1360,7 +1360,7 @@ int main(int argc, char *argv[]) {
         test_hexdump();
         test_log2i();
         test_foreach_string();
-        test_filename_is_safe();
+        test_filename_is_valid();
         test_string_has_cc();
         test_ascii_strlower();
         test_files_same();