core: be more strict when manipulating slices names and unescaping paths from unit names

Let's better be safe then sorry.

4 files changed, 178 insertions, 40 deletions

diff --git a/src/core/slice.c b/src/core/slice.c
index 0bebdbcbc..b965850da 100644
--- a/src/core/slice.c
+++ b/src/core/slice.c
@@ -93,27 +93,28 @@ static int slice_add_default_dependencies(Slice *s) {
         return 0;
 }
 
+
 static int slice_verify(Slice *s) {
+        _cleanup_free_ char *parent = NULL;
+        int r;
+
         assert(s);
 
         if (UNIT(s)->load_state != UNIT_LOADED)
                 return 0;
 
-        if (UNIT_DEREF(UNIT(s)->slice)) {
-                char *a, *dash;
+        if (!slice_name_is_valid(UNIT(s)->id)) {
+                log_unit_error(UNIT(s)->id, "Slice name %s is not valid. Refusing.", UNIT(s)->id);
+                return -EINVAL;
+        }
 
-                a = strdupa(UNIT(s)->id);
-                dash = strrchr(a, '-');
-                if (dash)
-                        strcpy(dash, ".slice");
-                else
-                        a = (char*) SPECIAL_ROOT_SLICE;
+        r = slice_build_parent_slice(UNIT(s)->id, &parent);
+        if (r < 0)
+                return log_unit_error_errno(UNIT(s)->id, r, "Failed to determine parent slice: %m");
 
-                if (!unit_has_name(UNIT_DEREF(UNIT(s)->slice), a)) {
-                        log_unit_error(UNIT(s)->id,
-                                       "%s located outside its parent slice. Refusing.", UNIT(s)->id);
-                        return -EINVAL;
-                }
+        if (parent ? !unit_has_name(UNIT_DEREF(UNIT(s)->slice), parent) : UNIT_ISSET(UNIT(s)->slice)) {
+                log_unit_error(UNIT(s)->id, "%s located outside its parent slice. Refusing.", UNIT(s)->id);
+                return -EINVAL;
         }
 
         return 0;
diff --git a/src/shared/unit-name.c b/src/shared/unit-name.c
index c41d7d86a..b23e47a2e 100644
--- a/src/shared/unit-name.c
+++ b/src/shared/unit-name.c
@@ -339,7 +339,7 @@ int unit_name_unescape(const char *f, char **ret) {
                         if (b < 0)
                                 return -EINVAL;
 
-                        *(t++) = (char) ((a << 4) | b);
+                        *(t++) = (char) (((uint8_t) a << 4U) | (uint8_t) b);
                         f += 3;
                 } else
                         *(t++) = *f;
@@ -392,42 +392,48 @@ int unit_name_path_escape(const char *f, char **ret) {
 }
 
 int unit_name_path_unescape(const char *f, char **ret) {
-        char *s, *w;
+        char *s;
         int r;
 
         assert(f);
 
+        if (isempty(f))
+                return -EINVAL;
+
         if (streq(f, "-")) {
                 s = strdup("/");
                 if (!s)
                         return -ENOMEM;
+        } else {
+                char *w;
 
-                *ret = s;
-                return 0;
-        }
-
-        r = unit_name_unescape(f, &s);
-        if (r < 0)
-                return r;
-
-        /* Don't accept trailing or leading slashes */
-        if (startswith(s, "/") || endswith(s, "/")) {
-                free(s);
-                return -EINVAL;
-        }
+                r = unit_name_unescape(f, &w);
+                if (r < 0)
+                        return r;
 
-        /* Prefix a slash again */
-        w = strappend("/", s);
-        free(s);
-        if (!w)
-                return -ENOMEM;
+                /* Don't accept trailing or leading slashes */
+                if (startswith(w, "/") || endswith(w, "/")) {
+                        free(w);
+                        return -EINVAL;
+                }
 
-        if (!path_is_safe(w)) {
+                /* Prefix a slash again */
+                s = strappend("/", w);
                 free(w);
-                return -EINVAL;
+                if (!s)
+                        return -ENOMEM;
+
+                if (!path_is_safe(s)) {
+                        free(s);
+                        return -EINVAL;
+                }
         }
 
-        *ret = w;
+        if (ret)
+                *ret = s;
+        else
+                free(s);
+
         return 0;
 }
 
@@ -665,6 +671,39 @@ int unit_name_mangle_with_suffix(const char *name, UnitNameMangle allow_globs, c
         return 1;
 }
 
+int slice_build_parent_slice(const char *slice, char **ret) {
+        char *s, *dash;
+
+        assert(slice);
+        assert(ret);
+
+        if (!slice_name_is_valid(slice))
+                return -EINVAL;
+
+        if (streq(slice, "-.slice")) {
+                *ret = NULL;
+                return 0;
+        }
+
+        s = strdup(slice);
+        if (!s)
+                return -ENOMEM;
+
+        dash = strrchr(s, '-');
+        if (dash)
+                strcpy(dash, ".slice");
+        else {
+                free(s);
+
+                s = strdup("-.slice");
+                if (!s)
+                        return -ENOMEM;
+        }
+
+        *ret = s;
+        return 1;
+}
+
 int slice_build_subslice(const char *slice, const char*name, char **ret) {
         char *subslice;
 
@@ -672,7 +711,7 @@ int slice_build_subslice(const char *slice, const char*name, char **ret) {
         assert(name);
         assert(ret);
 
-        if (!unit_name_is_valid(slice, UNIT_NAME_PLAIN))
+        if (!slice_name_is_valid(slice))
                 return -EINVAL;
 
         if (!unit_prefix_is_valid(name))
@@ -683,9 +722,7 @@ int slice_build_subslice(const char *slice, const char*name, char **ret) {
         else {
                 char *e;
 
-                e = endswith(slice, ".slice");
-                if (!e)
-                        return -EINVAL;
+                assert_se(e = endswith(slice, ".slice"));
 
                 subslice = new(char, (e - slice) + 1 + strlen(name) + 6 + 1);
                 if (!subslice)
@@ -698,6 +735,44 @@ int slice_build_subslice(const char *slice, const char*name, char **ret) {
         return 0;
 }
 
+bool slice_name_is_valid(const char *name) {
+        const char *p, *e;
+        bool dash = false;
+
+        if (!unit_name_is_valid(name, UNIT_NAME_PLAIN))
+                return false;
+
+        if (streq(name, "-.slice"))
+                return true;
+
+        e = endswith(name, ".slice");
+        if (!e)
+                return false;
+
+        for (p = name; p < e; p++) {
+
+                if (*p == '-') {
+
+                        /* Don't allow initial dash */
+                        if (p == name)
+                                return false;
+
+                        /* Don't allow multiple dashes */
+                        if (dash)
+                                return false;
+
+                        dash = true;
+                } else
+                        dash = false;
+        }
+
+        /* Don't allow trailing hash */
+        if (dash)
+                return false;
+
+        return true;
+}
+
 static const char* const unit_type_table[_UNIT_TYPE_MAX] = {
         [UNIT_SERVICE] = "service",
         [UNIT_SOCKET] = "socket",
diff --git a/src/shared/unit-name.h b/src/shared/unit-name.h
index 057512cbd..c2f31e3f9 100644
--- a/src/shared/unit-name.h
+++ b/src/shared/unit-name.h
@@ -161,7 +161,9 @@ static inline int unit_name_mangle(const char *name, UnitNameMangle allow_globs,
         return unit_name_mangle_with_suffix(name, allow_globs, ".service", ret);
 }
 
+int slice_build_parent_slice(const char *slice, char **ret);
 int slice_build_subslice(const char *slice, const char*name, char **subslice);
+bool slice_name_is_valid(const char *name);
 
 const char *unit_type_to_string(UnitType i) _const_;
 UnitType unit_type_from_string(const char *s) _pure_;
diff --git a/src/test/test-unit-name.c b/src/test/test-unit-name.c
index 5e34f18e3..de8f7ddfa 100644
--- a/src/test/test-unit-name.c
+++ b/src/test/test-unit-name.c
@@ -328,6 +328,21 @@ static void test_unit_name_build(void) {
         free(t);
 }
 
+static void test_slice_name_is_valid(void) {
+        assert_se(slice_name_is_valid("-.slice"));
+        assert_se(slice_name_is_valid("foo.slice"));
+        assert_se(slice_name_is_valid("foo-bar.slice"));
+        assert_se(slice_name_is_valid("foo-bar-baz.slice"));
+        assert_se(!slice_name_is_valid("-foo-bar-baz.slice"));
+        assert_se(!slice_name_is_valid("foo-bar-baz-.slice"));
+        assert_se(!slice_name_is_valid("-foo-bar-baz-.slice"));
+        assert_se(!slice_name_is_valid("foo-bar--baz.slice"));
+        assert_se(!slice_name_is_valid("foo--bar--baz.slice"));
+        assert_se(!slice_name_is_valid(".slice"));
+        assert_se(!slice_name_is_valid(""));
+        assert_se(!slice_name_is_valid("foo.service"));
+}
+
 static void test_build_subslice(void) {
         char *a;
         char *b;
@@ -346,6 +361,25 @@ static void test_build_subslice(void) {
         assert_se(slice_build_subslice("foo", "bar", &a) < 0);
 }
 
+static void test_build_parent_slice_one(const char *name, const char *expect, int ret) {
+        _cleanup_free_ char *s = NULL;
+
+        assert_se(slice_build_parent_slice(name, &s) == ret);
+        assert_se(streq_ptr(s, expect));
+}
+
+static void test_build_parent_slice(void) {
+        test_build_parent_slice_one("-.slice", NULL, 0);
+        test_build_parent_slice_one("foo.slice", "-.slice", 1);
+        test_build_parent_slice_one("foo-bar.slice", "foo.slice", 1);
+        test_build_parent_slice_one("foo-bar-baz.slice", "foo-bar.slice", 1);
+        test_build_parent_slice_one("foo-bar--baz.slice", NULL, -EINVAL);
+        test_build_parent_slice_one("-foo-bar.slice", NULL, -EINVAL);
+        test_build_parent_slice_one("foo-bar-.slice", NULL, -EINVAL);
+        test_build_parent_slice_one("foo-bar.service", NULL, -EINVAL);
+        test_build_parent_slice_one(".slice", NULL, -EINVAL);
+}
+
 static void test_unit_name_to_instance(void) {
         char *instance;
         int r;
@@ -398,6 +432,29 @@ static void test_unit_name_template(void) {
         test_u_n_t_one("foo.mount", NULL, -EINVAL);
 }
 
+static void test_unit_name_path_unescape_one(const char *name, const char *path, int ret) {
+        _cleanup_free_ char *p = NULL;
+
+        assert_se(unit_name_path_unescape(name, &p) == ret);
+        assert_se(streq_ptr(path, p));
+}
+
+static void test_unit_name_path_unescape(void) {
+
+        test_unit_name_path_unescape_one("foo", "/foo", 0);
+        test_unit_name_path_unescape_one("foo-bar", "/foo/bar", 0);
+        test_unit_name_path_unescape_one("foo-.bar", "/foo/.bar", 0);
+        test_unit_name_path_unescape_one("foo-bar-baz", "/foo/bar/baz", 0);
+        test_unit_name_path_unescape_one("-", "/", 0);
+        test_unit_name_path_unescape_one("--", NULL, -EINVAL);
+        test_unit_name_path_unescape_one("-foo-bar", NULL, -EINVAL);
+        test_unit_name_path_unescape_one("foo--bar", NULL, -EINVAL);
+        test_unit_name_path_unescape_one("foo-bar-", NULL, -EINVAL);
+        test_unit_name_path_unescape_one(".-bar", NULL, -EINVAL);
+        test_unit_name_path_unescape_one("foo-..", NULL, -EINVAL);
+        test_unit_name_path_unescape_one("", NULL, -EINVAL);
+}
+
 int main(int argc, char* argv[]) {
         int rc = 0;
         test_unit_name_is_valid();
@@ -411,10 +468,13 @@ int main(int argc, char* argv[]) {
         test_unit_prefix_is_valid();
         test_unit_name_change_suffix();
         test_unit_name_build();
+        test_slice_name_is_valid();
         test_build_subslice();
+        test_build_parent_slice();
         test_unit_name_to_instance();
         test_unit_name_escape();
         test_unit_name_template();
+        test_unit_name_path_unescape();
 
         return rc;
 }