From 78edb35ab4f4227485cb9ec816b43c37e0d5e62a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Apr 2013 19:01:29 -0300 Subject: [PATCH] cgroup: always validate cgroup controller names Let's better be safe than sorry. --- TODO | 4 -- src/core/unit.c | 26 +++++++------ src/core/unit.h | 1 - src/shared/cgroup-util.c | 77 ++++++++++++++++++++++++++++--------- src/shared/cgroup-util.h | 2 + src/test/test-cgroup-util.c | 14 +++++++ 6 files changed, 89 insertions(+), 35 deletions(-) diff --git a/TODO b/TODO index 88a3b2cb4..cfd42ce74 100644 --- a/TODO +++ b/TODO @@ -57,10 +57,6 @@ Features: * 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 diff --git a/src/core/unit.c b/src/core/unit.c index 4b9abf32d..c0c3ce90a 100644 --- 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); } -int unit_add_cgroup(Unit *u, CGroupBonding *b) { +static int unit_add_cgroup(Unit *u, CGroupBonding *b) { int r; assert(u); @@ -2100,6 +2100,9 @@ static int unit_add_one_default_cgroup(Unit *u, const char *controller) { assert(u); + if (controller && !cg_controller_is_valid(controller, true)) + return -EINVAL; + if (!controller) controller = SYSTEMD_CGROUP_CONTROLLER; @@ -2202,13 +2205,15 @@ int unit_add_cgroup_attribute( 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; - if (!filename_is_safe(controller)) + if (!cg_controller_is_valid(controller, false)) 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) { - char *t; + _cleanup_free_ char *t = NULL; 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); - free(t); - assert(r < 0 || *_found != u); - return r; } int unit_get_related_unit(Unit *u, const char *type, Unit **_found) { + _cleanup_free_ char *t = NULL; Unit *found; - char *t; 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); - free(t); - if (!found) return -ENOENT; diff --git a/src/core/unit.h b/src/core/unit.h index 51a8364d6..6bfe58c8b 100644 --- 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_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); diff --git a/src/shared/cgroup-util.c b/src/shared/cgroup-util.c index 9ec4f40c8..b79a24a49 100644 --- 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); + if (controller && !cg_controller_is_valid(controller, true)) + return -EINVAL; + 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); - if (isempty(controller)) + if (!cg_controller_is_valid(controller, true)) 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); + if (controller && !cg_controller_is_valid(controller, true)) + return -EINVAL; + 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) { - if (!filename_is_safe(spec)) + if (!cg_controller_is_valid(spec, true)) 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; - if (!filename_is_safe(t)) { + if (!cg_controller_is_valid(t, true)) { free(t); return -EINVAL; } @@ -987,18 +993,19 @@ int cg_join_spec(const char *controller, const char *path, char **spec) { assert(path); + 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; - controller = normalize_controller(controller); - 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) { - char *t, *c, *p; + _cleanup_free_ char *c = NULL, *p = NULL; + char *t; int r; assert(path); @@ -1030,11 +1038,7 @@ int cg_mangle_path(const char *path, char **result) { 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) { @@ -1138,14 +1142,20 @@ char **cg_shorten_controllers(char **controllers) { 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) { - 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; } @@ -1457,7 +1467,7 @@ int cg_controller_from_attr(const char *attr, char **controller) { if (!c) return -ENOMEM; - if (!filename_is_safe(c)) { + if (!cg_controller_is_valid(c, false)) { free(c); return -EINVAL; } @@ -1517,3 +1527,34 @@ char *cg_unescape(const 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 2099f934b..a2ee72d67 100644 --- 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); + +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 95cede7a2..6726f8fb1 100644 --- 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"); } +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(); @@ -160,6 +173,7 @@ int main(void) { test_get_paths(); test_proc(); test_escape(); + test_controller_is_valid(); return 0; } -- 2.30.2