cgroup: additional validity checks for cgroup attribute names

5 files changed, 89 insertions, 34 deletions

diff --git a/src/core/unit.c b/src/core/unit.c
index 83359e126b..6cf02365e9 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -2156,26 +2156,27 @@ int unit_add_cgroup_attribute(
 
         _cleanup_free_ char *c = NULL;
         CGroupAttribute *a;
+        int r;
 
         assert(u);
         assert(name);
         assert(value);
 
         if (!controller) {
-                const char *dot;
-
-                dot = strchr(name, '.');
-                if (!dot)
+                r = cg_controller_from_attr(name, &c);
+                if (r < 0)
                         return -EINVAL;
 
-                c = strndup(name, dot - name);
-                if (!c)
-                        return -ENOMEM;
-
                 controller = c;
+        } else {
+                if (!filename_is_safe(name))
+                        return -EINVAL;
+
+                if (!filename_is_safe(controller))
+                        return -EINVAL;
         }
 
-        if (streq(controller, SYSTEMD_CGROUP_CONTROLLER))
+        if (!controller || streq(controller, SYSTEMD_CGROUP_CONTROLLER))
                 return -EINVAL;
 
         a = cgroup_attribute_find_list(u->cgroup_attributes, controller, name);
diff --git a/src/shared/cgroup-util.c b/src/shared/cgroup-util.c
index f0d0d4855b..acace52bc8 100644
--- a/src/shared/cgroup-util.c
+++ b/src/shared/cgroup-util.c
@@ -990,6 +990,8 @@ int cg_split_spec(const char *spec, char **controller, char **path) {
         assert(spec);
 
         if (*spec == '/') {
+                if (!path_is_safe(spec))
+                        return -EINVAL;
 
                 if (path) {
                         t = strdup(spec);
@@ -1007,7 +1009,7 @@ int cg_split_spec(const char *spec, char **controller, char **path) {
 
         e = strchr(spec, ':');
         if (!e) {
-                if (strchr(spec, '/') || spec[0] == 0)
+                if (!filename_is_safe(spec))
                         return -EINVAL;
 
                 if (controller) {
@@ -1024,29 +1026,34 @@ int cg_split_spec(const char *spec, char **controller, char **path) {
                 return 0;
         }
 
-        if (e[1] != '/' || e == spec || memchr(spec, '/', e-spec))
+        t = strndup(spec, e-spec);
+        if (!t)
+                return -ENOMEM;
+        if (!filename_is_safe(t)) {
+                free(t);
                 return -EINVAL;
-
-        if (controller) {
-                t = strndup(spec, e-spec);
-                if (!t)
-                        return -ENOMEM;
-
         }
 
-        if (path) {
-                u = strdup(e+1);
-                if (!u) {
-                        free(t);
-                        return -ENOMEM;
-                }
+        u = strdup(e+1);
+        if (!u) {
+                free(t);
+                return -ENOMEM;
+        }
+        if (!path_is_safe(u)) {
+                free(t);
+                free(u);
+                return -EINVAL;
         }
 
         if (controller)
                 *controller = t;
+        else
+                free(t);
 
         if (path)
                 *path = u;
+        else
+                free(u);
 
         return 0;
 }
@@ -1290,3 +1297,32 @@ int cg_pid_get_unit(pid_t pid, char **unit) {
 int cg_pid_get_user_unit(pid_t pid, char **unit) {
         return cg_pid_get("/user/", pid, unit);
 }
+
+int cg_controller_from_attr(const char *attr, char **controller) {
+        const char *dot;
+        char *c;
+
+        assert(attr);
+        assert(controller);
+
+        if (!filename_is_safe(attr))
+                return -EINVAL;
+
+        dot = strchr(attr, '.');
+        if (!dot) {
+                *controller = NULL;
+                return 0;
+        }
+
+        c = strndup(attr, dot - attr);
+        if (!c)
+                return -ENOMEM;
+
+        if (!filename_is_safe(c)) {
+                free(c);
+                return -EINVAL;
+        }
+
+        *controller = c;
+        return 1;
+}
diff --git a/src/shared/cgroup-util.h b/src/shared/cgroup-util.h
index 920cf631e5..06c6bfb2e3 100644
--- a/src/shared/cgroup-util.h
+++ b/src/shared/cgroup-util.h
@@ -76,3 +76,5 @@ int cg_pid_get_user_unit(pid_t pid, char **unit);
 int cgroup_to_unit(char *cgroup, char **unit);
 
 char **cg_shorten_controllers(char **controllers);
+
+int cg_controller_from_attr(const char *attr, char **controller);
diff --git a/src/shared/util.c b/src/shared/util.c
index 37e383f2ef..1aaebf0612 100644
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -561,9 +561,9 @@ int fchmod_umask(int fd, mode_t m) {
 }
 
 int write_one_line_file_atomic(const char *fn, const char *line) {
-        FILE *f;
+        _cleanup_fclose_ FILE *f = NULL;
+        _cleanup_free_ char *p = NULL;
         int r;
-        char *p;
 
         assert(fn);
         assert(line);
@@ -585,12 +585,9 @@ int write_one_line_file_atomic(const char *fn, const char *line) {
 
         fflush(f);
 
-        if (ferror(f)) {
-                if (errno != 0)
-                        r = -errno;
-                else
-                        r = -EIO;
-        } else {
+        if (ferror(f))
+                r = errno ? -errno : -EIO;
+        else {
                 if (rename(p, fn) < 0)
                         r = -errno;
                 else
@@ -601,9 +598,6 @@ finish:
         if (r < 0)
                 unlink(p);
 
-        fclose(f);
-        free(p);
-
         return r;
 }
 
@@ -5613,6 +5607,27 @@ bool string_is_safe(const char *p) {
         return true;
 }
 
+bool path_is_safe(const char *p) {
+
+        if (isempty(p))
+                return false;
+
+        if (streq(p, "..") || startswith(p, "../") || endswith(p, "/..") || strstr(p, "/../"))
+                return false;
+
+        if (strlen(p) > PATH_MAX)
+                return false;
+
+        /* The following two checks are not really dangerous, but hey, they still are confusing */
+        if (streq(p, ".") || startswith(p, "./") || endswith(p, "/.") || strstr(p, "/./"))
+                return false;
+
+        if (strstr(p, "//"))
+                return false;
+
+        return true;
+}
+
 /* hey glibc, APIs with callbacks without a user pointer are so useless */
 void *xbsearch_r(const void *key, const void *base, size_t nmemb, size_t size,
                  int (*compar) (const void *, const void *, void *), void *arg) {
diff --git a/src/shared/util.h b/src/shared/util.h
index cdaff45772..d260385991 100644
--- a/src/shared/util.h
+++ b/src/shared/util.h
@@ -543,6 +543,7 @@ _malloc_ static inline void *memdup_multiply(const void *p, size_t a, size_t b)
 }
 
 bool filename_is_safe(const char *p);
+bool path_is_safe(const char *p);
 bool string_is_safe(const char *p);
 
 void *xbsearch_r(const void *key, const void *base, size_t nmemb, size_t size,