cgroup: always validate cgroup controller names

Let's better be safe than sorry.

diff --git a/TODO b/TODO
index 88a3b2cb457739faf3ab4901f6e2fd6d1c134d1b..cfd42ce745b1755d33839fda5f8d853ae41dad89 100644 (file)
--- a/TODO
+++ b/TODO
@@ -57,10 +57,6 @@ Features:
 
 * add s.th. like "systemctl set-log-level debug"
 
 
 * add s.th. like "systemctl set-log-level debug"
 

-* sd-login: allow enumerating machines and add inotify iface
-
-* cgroup-util: verify syntax of cgroup controllers
-

 * cgtop: make cgtop useful in a container
 
 * make sure cg_pid_get_path() works properly for co-mounted controllers
 * cgtop: make cgtop useful in a container
 
 * make sure cg_pid_get_path() works properly for co-mounted controllers

diff --git a/src/core/unit.c b/src/core/unit.c
index 4b9abf32dc04379cc210b79cc4211276d778c16c..c0c3ce90a7af56d01307cde4b90a74d69ba653fb 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -1938,7 +1938,7 @@ char *unit_dbus_path(Unit *u) {
         return unit_dbus_path_from_name(u->id);
 }
 
         return unit_dbus_path_from_name(u->id);
 }
 

-int unit_add_cgroup(Unit *u, CGroupBonding *b) {
+static int unit_add_cgroup(Unit *u, CGroupBonding *b) {

         int r;
 
         assert(u);
         int r;
 
         assert(u);


@@ -2100,6 +2100,9 @@ static int unit_add_one_default_cgroup(Unit *u, const char *controller) {
 
         assert(u);
 
 
         assert(u);
 

+        if (controller && !cg_controller_is_valid(controller, true))
+                return -EINVAL;
+

         if (!controller)
                 controller = SYSTEMD_CGROUP_CONTROLLER;
 
         if (!controller)
                 controller = SYSTEMD_CGROUP_CONTROLLER;
 


@@ -2202,13 +2205,15 @@ int unit_add_cgroup_attribute(
                 controller = c;
         }
 
                 controller = c;
         }
 

-        if (!controller || streq(controller, SYSTEMD_CGROUP_CONTROLLER))
+        if (!controller ||
+            streq(controller, SYSTEMD_CGROUP_CONTROLLER) ||
+            streq(controller, "systemd"))

                 return -EINVAL;
 
         if (!filename_is_safe(name))
                 return -EINVAL;
 
                 return -EINVAL;
 
         if (!filename_is_safe(name))
                 return -EINVAL;
 

-        if (!filename_is_safe(controller))
+        if (!cg_controller_is_valid(controller, false))

                 return -EINVAL;
 
         /* Check if this attribute already exists. Note that we will
                 return -EINVAL;
 
         /* Check if this attribute already exists. Note that we will


@@ -2276,42 +2281,39 @@ int unit_add_cgroup_attribute(
 }
 
 int unit_load_related_unit(Unit *u, const char *type, Unit **_found) {
 }
 
 int unit_load_related_unit(Unit *u, const char *type, Unit **_found) {

-        char *t;
+        _cleanup_free_ char *t = NULL;

         int r;
 
         assert(u);
         assert(type);
         assert(_found);
 
         int r;
 
         assert(u);
         assert(type);
         assert(_found);
 

-        if (!(t = unit_name_change_suffix(u->id, type)))
+        t = unit_name_change_suffix(u->id, type);
+        if (!t)

                 return -ENOMEM;
 
         assert(!unit_has_name(u, t));
 
         r = manager_load_unit(u->manager, t, NULL, NULL, _found);
                 return -ENOMEM;
 
         assert(!unit_has_name(u, t));
 
         r = manager_load_unit(u->manager, t, NULL, NULL, _found);

-        free(t);
-

         assert(r < 0 || *_found != u);
         assert(r < 0 || *_found != u);

-

         return r;
 }
 
 int unit_get_related_unit(Unit *u, const char *type, Unit **_found) {
         return r;
 }
 
 int unit_get_related_unit(Unit *u, const char *type, Unit **_found) {

+        _cleanup_free_ char *t = NULL;

         Unit *found;
         Unit *found;

-        char *t;

 
         assert(u);
         assert(type);
         assert(_found);
 
 
         assert(u);
         assert(type);
         assert(_found);
 

-        if (!(t = unit_name_change_suffix(u->id, type)))
+        t = unit_name_change_suffix(u->id, type);
+        if (!t)

                 return -ENOMEM;
 
         assert(!unit_has_name(u, t));
 
         found = manager_get_unit(u->manager, t);
                 return -ENOMEM;
 
         assert(!unit_has_name(u, t));
 
         found = manager_get_unit(u->manager, t);

-        free(t);
-

         if (!found)
                 return -ENOENT;
 
         if (!found)
                 return -ENOENT;
 

diff --git a/src/core/unit.h b/src/core/unit.h
index 51a8364d63e43b1d4c136cf1dac4a3bbdf6d0e8e..6bfe58c8bfaec36b34677fe219d1935e92c6596f 100644 (file)
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -450,7 +450,6 @@ int unit_add_two_dependencies_by_name_inverse(Unit *u, UnitDependency d, UnitDep
 
 int unit_add_exec_dependencies(Unit *u, ExecContext *c);
 
 
 int unit_add_exec_dependencies(Unit *u, ExecContext *c);
 

-int unit_add_cgroup(Unit *u, CGroupBonding *b);

 int unit_add_cgroup_from_text(Unit *u, const char *name, bool overwrite, CGroupBonding **ret);
 int unit_add_default_cgroups(Unit *u);
 CGroupBonding* unit_get_default_cgroup(Unit *u);
 int unit_add_cgroup_from_text(Unit *u, const char *name, bool overwrite, CGroupBonding **ret);
 int unit_add_default_cgroups(Unit *u);
 CGroupBonding* unit_get_default_cgroup(Unit *u);

diff --git a/src/shared/cgroup-util.c b/src/shared/cgroup-util.c
index 9ec4f40c889ef2b80606abf6bde9f45874dc39e2..b79a24a495e6f6ddccf8e09d6c1e144df01af113 100644 (file)
--- a/src/shared/cgroup-util.c
+++ b/src/shared/cgroup-util.c
@@ -510,6 +510,9 @@ int cg_get_path(const char *controller, const char *path, const char *suffix, ch
 
         assert(fs);
 
 
         assert(fs);
 

+        if (controller && !cg_controller_is_valid(controller, true))
+                return -EINVAL;
+

         if (_unlikely_(!good)) {
                 int r;
 
         if (_unlikely_(!good)) {
                 int r;
 


@@ -546,7 +549,7 @@ int cg_get_path_and_check(const char *controller, const char *path, const char *
 
         assert(fs);
 
 
         assert(fs);
 

-        if (isempty(controller))
+        if (!cg_controller_is_valid(controller, true))

                 return -EINVAL;
 
         /* Normalize the controller syntax */
                 return -EINVAL;
 
         /* Normalize the controller syntax */


@@ -741,6 +744,9 @@ int cg_pid_get_path(const char *controller, pid_t pid, char **path) {
         assert(path);
         assert(pid >= 0);
 
         assert(path);
         assert(pid >= 0);
 

+        if (controller && !cg_controller_is_valid(controller, true))
+                return -EINVAL;
+

         if (!controller)
                 controller = SYSTEMD_CGROUP_CONTROLLER;
 
         if (!controller)
                 controller = SYSTEMD_CGROUP_CONTROLLER;
 


@@ -933,7 +939,7 @@ int cg_split_spec(const char *spec, char **controller, char **path) {
 
         e = strchr(spec, ':');
         if (!e) {
 
         e = strchr(spec, ':');
         if (!e) {

-                if (!filename_is_safe(spec))
+                if (!cg_controller_is_valid(spec, true))

                         return -EINVAL;
 
                 if (controller) {
                         return -EINVAL;
 
                 if (controller) {


@@ -953,7 +959,7 @@ int cg_split_spec(const char *spec, char **controller, char **path) {
         t = strndup(spec, e-spec);
         if (!t)
                 return -ENOMEM;
         t = strndup(spec, e-spec);
         if (!t)
                 return -ENOMEM;

-        if (!filename_is_safe(t)) {
+        if (!cg_controller_is_valid(t, true)) {

                 free(t);
                 return -EINVAL;
         }
                 free(t);
                 return -EINVAL;
         }


@@ -987,18 +993,19 @@ int cg_join_spec(const char *controller, const char *path, char **spec) {
 
         assert(path);
 
 
         assert(path);
 

+

         if (!controller)
                 controller = "systemd";
         if (!controller)
                 controller = "systemd";

-        else if (controller[0] == 0 ||
-                 strchr(controller, ':') ||
-                 strchr(controller, '/'))
-                return -EINVAL;
+        else {
+                if (!cg_controller_is_valid(controller, true))
+                        return -EINVAL;
+
+                controller = normalize_controller(controller);
+        }

 
         if (!path_is_absolute(path))
                 return -EINVAL;
 
 
         if (!path_is_absolute(path))
                 return -EINVAL;
 

-        controller = normalize_controller(controller);
-

         s = strjoin(controller, ":", path, NULL);
         if (!s)
                 return -ENOMEM;
         s = strjoin(controller, ":", path, NULL);
         if (!s)
                 return -ENOMEM;


@@ -1008,7 +1015,8 @@ int cg_join_spec(const char *controller, const char *path, char **spec) {
 }
 
 int cg_mangle_path(const char *path, char **result) {
 }
 
 int cg_mangle_path(const char *path, char **result) {

-        char *t, *c, *p;
+        _cleanup_free_ char *c = NULL, *p = NULL;
+        char *t;

         int r;
 
         assert(path);
         int r;
 
         assert(path);


@@ -1030,11 +1038,7 @@ int cg_mangle_path(const char *path, char **result) {
         if (r < 0)
                 return r;
 
         if (r < 0)
                 return r;
 

-        r = cg_get_path(c ? c : SYSTEMD_CGROUP_CONTROLLER, p ? p : "/", NULL, result);
-        free(c);
-        free(p);
-
-        return r;
+        return cg_get_path(c ? c : SYSTEMD_CGROUP_CONTROLLER, p ? p : "/", NULL, result);

 }
 
 int cg_get_system_path(char **path) {
 }
 
 int cg_get_system_path(char **path) {


@@ -1138,14 +1142,20 @@ char **cg_shorten_controllers(char **controllers) {
 
                 p = normalize_controller(*f);
 
 
                 p = normalize_controller(*f);
 

-                if (streq(*f, "systemd")) {
+                if (streq(p, "systemd")) {
+                        free(*f);
+                        continue;
+                }
+
+                if (!cg_controller_is_valid(p, true)) {
+                        log_warning("Controller %s is not valid, removing from controllers list.", p);

                         free(*f);
                         continue;
                 }
 
                 r = check_hierarchy(p);
                 if (r < 0) {
                         free(*f);
                         continue;
                 }
 
                 r = check_hierarchy(p);
                 if (r < 0) {

-                        log_debug("Controller %s is not available, removing from controllers list.", *f);
+                        log_debug("Controller %s is not available, removing from controllers list.", p);

                         free(*f);
                         continue;
                 }
                         free(*f);
                         continue;
                 }


@@ -1457,7 +1467,7 @@ int cg_controller_from_attr(const char *attr, char **controller) {
         if (!c)
                 return -ENOMEM;
 
         if (!c)
                 return -ENOMEM;
 

-        if (!filename_is_safe(c)) {
+        if (!cg_controller_is_valid(c, false)) {

                 free(c);
                 return -EINVAL;
         }
                 free(c);
                 return -EINVAL;
         }


@@ -1517,3 +1527,34 @@ char *cg_unescape(const char *p) {
 
         return (char*) p;
 }
 
         return (char*) p;
 }

+
+#define CONTROLLER_VALID                        \
+        "0123456789"                            \
+        "abcdefghijklmnopqrstuvwxyz"            \
+        "ABCDEFGHIJKLMNOPQRSTUVWXYZ"            \
+        "_"
+
+bool cg_controller_is_valid(const char *p, bool allow_named) {
+        const char *t, *s;
+
+        if (!p)
+                return false;
+
+        if (allow_named) {
+                s = startswith(p, "name=");
+                if (s)
+                        p = s;
+        }
+
+        if (*p == 0 || *p == '_')
+                return false;
+
+        for (t = p; *t; t++)
+                if (!strchr(CONTROLLER_VALID, *t))
+                        return false;
+
+        if (t - p > FILENAME_MAX)
+                return false;
+
+        return true;
+}

diff --git a/src/shared/cgroup-util.h b/src/shared/cgroup-util.h
index 2099f934b4eb6a186e76086f88c80d88e59c597b..a2ee72d67ffa48201e56b2c7ce6bb0db625f720a 100644 (file)
--- a/src/shared/cgroup-util.h
+++ b/src/shared/cgroup-util.h
@@ -96,3 +96,5 @@ int cg_controller_from_attr(const char *attr, char **controller);
 
 char *cg_escape(const char *p);
 char *cg_unescape(const char *p);
 
 char *cg_escape(const char *p);
 char *cg_unescape(const char *p);

+
+bool cg_controller_is_valid(const char *p, bool allow_named);

diff --git a/src/test/test-cgroup-util.c b/src/test/test-cgroup-util.c
index 95cede7a22865a85b659fc4c0680cc28a2246b35..6726f8fb136bc1e11c249f5589021bda2343387c 100644 (file)
--- a/src/test/test-cgroup-util.c
+++ b/src/test/test-cgroup-util.c
@@ -153,6 +153,19 @@ static void test_escape(void) {
         test_escape_one("_foobar", "__foobar");
 }
 
         test_escape_one("_foobar", "__foobar");
 }
 

+static void test_controller_is_valid(void) {
+        assert_se(cg_controller_is_valid("foobar", false));
+        assert_se(cg_controller_is_valid("foo_bar", false));
+        assert_se(cg_controller_is_valid("name=foo", true));
+        assert_se(!cg_controller_is_valid("", false));
+        assert_se(!cg_controller_is_valid("name=", true));
+        assert_se(!cg_controller_is_valid("=", false));
+        assert_se(!cg_controller_is_valid("cpu,cpuacct", false));
+        assert_se(!cg_controller_is_valid("_", false));
+        assert_se(!cg_controller_is_valid("_foobar", false));
+        assert_se(!cg_controller_is_valid("tatü", false));
+}
+

 int main(void) {
         test_path_decode_unit();
         test_path_get_unit();
 int main(void) {
         test_path_decode_unit();
         test_path_get_unit();


@@ -160,6 +173,7 @@ int main(void) {
         test_get_paths();
         test_proc();
         test_escape();
         test_get_paths();
         test_proc();
         test_escape();

+        test_controller_is_valid();

 
         return 0;
 }
 
         return 0;
 }