bus: handle env vars safely

Make sure that our library is safe for usage in SUID programs when it
comes to env var handling

4 files changed, 162 insertions, 68 deletions

diff --git a/src/libsystemd-bus/sd-bus.c b/src/libsystemd-bus/sd-bus.c
index f2dd81235f..c7511c32d2 100644
--- a/src/libsystemd-bus/sd-bus.c
+++ b/src/libsystemd-bus/sd-bus.c
@@ -31,6 +31,7 @@
 #include "macro.h"
 #include "strv.h"
 #include "set.h"
+#include "missing.h"
 
 #include "sd-bus.h"
 #include "bus-internal.h"
@@ -841,7 +842,7 @@ int sd_bus_open_system(sd_bus **ret) {
         if (r < 0)
                 return r;
 
-        e = getenv("DBUS_SYSTEM_BUS_ADDRESS");
+        e = secure_getenv("DBUS_SYSTEM_BUS_ADDRESS");
         if (e) {
                 r = sd_bus_set_address(b, e);
                 if (r < 0)
@@ -879,13 +880,13 @@ int sd_bus_open_user(sd_bus **ret) {
         if (r < 0)
                 return r;
 
-        e = getenv("DBUS_SESSION_BUS_ADDRESS");
+        e = secure_getenv("DBUS_SESSION_BUS_ADDRESS");
         if (e) {
                 r = sd_bus_set_address(b, e);
                 if (r < 0)
                         goto fail;
         } else {
-                e = getenv("XDG_RUNTIME_DIR");
+                e = secure_getenv("XDG_RUNTIME_DIR");
                 if (!e) {
                         r = -ENOENT;
                         goto fail;
diff --git a/src/shared/cgroup-util.c b/src/shared/cgroup-util.c
index c17e1d4d1b..075260783c 100644
--- a/src/shared/cgroup-util.c
+++ b/src/shared/cgroup-util.c
@@ -1148,8 +1148,6 @@ int cg_get_user_path(char **path) {
 char **cg_shorten_controllers(char **controllers) {
         char **f, **t;
 
-        controllers = strv_uniq(controllers);
-
         if (!controllers)
                 return controllers;
 
@@ -1175,11 +1173,11 @@ char **cg_shorten_controllers(char **controllers) {
         }
 
         *t = NULL;
-        return controllers;
+        return strv_uniq(controllers);
 }
 
 int cg_pid_get_cgroup(pid_t pid, char **root, char **cgroup) {
-        char *cg_process, *cg_init, *p;
+        char *cg_process, *cg_init, *p, *q;
         int r;
 
         assert(pid >= 0);
@@ -1202,11 +1200,8 @@ int cg_pid_get_cgroup(pid_t pid, char **root, char **cgroup) {
         else if (streq(cg_init, "/"))
                 cg_init[0] = 0;
 
-        if (startswith(cg_process, cg_init))
-                p = cg_process + strlen(cg_init);
-        else
-                p = cg_process;
-
+        q = startswith(cg_process, cg_init);
+        p = q ? q : cg_process;
         free(cg_init);
 
         if (cgroup) {
@@ -1230,84 +1225,126 @@ int cg_pid_get_cgroup(pid_t pid, char **root, char **cgroup) {
         return 0;
 }
 
-static int instance_unit_from_cgroup(char *cgroup){
-        char *at;
+/* non-static only for testing purposes */
+int cg_cgroup_to_unit(const char *cgroup, char **unit){
+        char *p, *e, *c, *s, *k;
 
         assert(cgroup);
+        assert(unit);
 
-        at = strstr(cgroup, "@.");
-        if (at) {
-                /* This is a templated service */
+        e = strchrnul(cgroup, '/');
+        c = strndupa(cgroup, e - cgroup);
 
-                char *i;
-                char _cleanup_free_ *i2 = NULL, *s = NULL;
+        /* Could this be a valid unit name? */
+        if (!unit_name_is_valid(c, true))
+                return -EINVAL;
 
-                i = strchr(at, '/');
-                if (!i || !i[1]) /* disallow empty instances */
+        if (!unit_name_is_template(c))
+                s = strdup(c);
+        else {
+                if (*e != '/')
                         return -EINVAL;
 
-                s = strndup(at + 1, i - at - 1);
-                i2 = strdup(i + 1);
-                if (!s || !i2)
-                        return -ENOMEM;
+                e += strspn(e, "/");
+                p = strchrnul(e, '/');
 
-                strcpy(at + 1, i2);
-                strcat(at + 1, s);
+                /* Don't allow empty instance strings */
+                if (p == e)
+                        return -EINVAL;
+
+                k = strndupa(e, p - e);
+
+                s = unit_name_replace_instance(c, k);
         }
 
+        if (!s)
+                return -ENOMEM;
+
+        *unit = s;
         return 0;
 }
 
-/* non-static only for testing purposes */
-int cgroup_to_unit(char *cgroup, char **unit){
+int cg_path_get_unit(const char *path, char **unit) {
+        const char *e;
+
+        assert(path);
+        assert(unit);
+
+        e = path_startswith(path, "/system/");
+        if (!e)
+                return -ENOENT;
+
+        return cg_cgroup_to_unit(e, unit);
+}
+
+int cg_pid_get_unit(pid_t pid, char **unit) {
+        char _cleanup_free_ *cgroup = NULL;
         int r;
-        char *p;
 
-        assert(cgroup);
         assert(unit);
 
-        r = instance_unit_from_cgroup(cgroup);
+        r = cg_pid_get_cgroup(pid, NULL, &cgroup);
         if (r < 0)
                 return r;
 
-        p = strrchr(cgroup, '/');
-        assert(p);
+        return cg_path_get_unit(cgroup, unit);
+}
 
-        r = unit_name_is_valid(p + 1, true);
-        if (!r)
-                return -EINVAL;
+static const char *skip_label(const char *e) {
+        assert(e);
 
-        *unit = strdup(p + 1);
-        if (!*unit)
-                return -ENOMEM;
+        e += strspn(e, "/");
+        e = strchr(e, '/');
+        if (!e)
+                return NULL;
 
-        return 0;
+        e += strspn(e, "/");
+        return e;
 }
 
-static int cg_pid_get(const char *prefix, pid_t pid, char **unit) {
-        int r;
-        char _cleanup_free_ *cgroup = NULL;
+int cg_path_get_user_unit(const char *path, char **unit) {
+        const char *e;
 
-        assert(pid >= 0);
+        assert(path);
         assert(unit);
 
-        r = cg_pid_get_cgroup(pid, NULL, &cgroup);
-        if (r < 0)
-                return r;
+        /* We always have to parse the path from the beginning as unit
+         * cgroups might have arbitrary child cgroups and we shouldn't get
+         * confused by those */
 
-        if (!startswith(cgroup, prefix))
+        e = path_startswith(path, "/user/");
+        if (!e)
                 return -ENOENT;
 
-        r = cgroup_to_unit(cgroup, unit);
-        return r;
-}
+        /* Skip the user name */
+        e = skip_label(e);
+        if (!e)
+                return -ENOENT;
 
-int cg_pid_get_unit(pid_t pid, char **unit) {
-        return cg_pid_get("/system/", pid, unit);
+        /* Skip the session ID */
+        e = skip_label(e);
+        if (!e)
+                return -ENOENT;
+
+        /* Skip the systemd cgroup */
+        e = skip_label(e);
+        if (!e)
+                return -ENOENT;
+
+        return cg_cgroup_to_unit(e, unit);
 }
 
 int cg_pid_get_user_unit(pid_t pid, char **unit) {
-        return cg_pid_get("/user/", pid, unit);
+        char _cleanup_free_ *cgroup = NULL;
+        int r;
+
+        assert(unit);
+
+        r = cg_pid_get_cgroup(pid, NULL, &cgroup);
+        if (r < 0)
+                return r;
+
+        return cg_path_get_user_unit(cgroup, unit);
 }
 
 int cg_controller_from_attr(const char *attr, char **controller) {
diff --git a/src/shared/cgroup-util.h b/src/shared/cgroup-util.h
index 06c6bfb2e3..123f72c69f 100644
--- a/src/shared/cgroup-util.h
+++ b/src/shared/cgroup-util.h
@@ -69,11 +69,15 @@ int cg_is_empty_by_spec(const char *spec, bool ignore_self);
 int cg_is_empty_recursive(const char *controller, const char *path, bool ignore_self);
 
 int cg_get_user_path(char **path);
+
+int cg_path_get_unit(const char *path, char **unit);
+int cg_path_get_user_unit(const char *path, char **unit);
+
 int cg_pid_get_cgroup(pid_t pid, char **root, char **cgroup);
 int cg_pid_get_unit(pid_t pid, char **unit);
 int cg_pid_get_user_unit(pid_t pid, char **unit);
 
-int cgroup_to_unit(char *cgroup, char **unit);
+int cg_cgroup_to_unit(const char *cgroup, char **unit);
 
 char **cg_shorten_controllers(char **controllers);
 
diff --git a/src/test/test-cgroup-util.c b/src/test/test-cgroup-util.c
index b30bf23a80..8e24d1ceca 100644
--- a/src/test/test-cgroup-util.c
+++ b/src/test/test-cgroup-util.c
@@ -1,27 +1,79 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+  This file is part of systemd.
+
+  Copyright 2013 Zbigniew Jędrzejewski-Szmek
+
+  systemd is free software; you can redistribute it and/or modify it
+  under the terms of the GNU Lesser General Public License as published by
+  the Free Software Foundation; either version 2.1 of the License, or
+  (at your option) any later version.
+
+  systemd is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+  Lesser General Public License for more details.
+
+  You should have received a copy of the GNU Lesser General Public License
+  along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
 #include <assert.h>
 
 #include "util.h"
 #include "cgroup-util.h"
 
-#define check_c_t_u(path, code, result) \
-{ \
-   char a[] = path; \
-   char *unit = NULL; \
-   assert_se(cgroup_to_unit(a, &unit) == code); \
-   assert(code < 0 || streq(unit, result));                 \
-}
+static void check_c_t_u(const char *path, int code, const char *result) {
+        _cleanup_free_ char *unit = NULL;
 
+        assert_se(cg_cgroup_to_unit(path, &unit) == code);
+        assert_se(streq_ptr(unit, result));
+}
 
 static void test_cgroup_to_unit(void) {
-        check_c_t_u("/system/getty@.service/tty2", 0, "getty@tty2.service");
-        check_c_t_u("/system/getty@.service/", -EINVAL, "getty@tty2.service");
-        check_c_t_u("/system/getty@.service", -EINVAL, "getty@tty2.service");
-        check_c_t_u("/system/getty.service", 0, "getty.service");
-        check_c_t_u("/system/getty", -EINVAL, "getty.service");
+        check_c_t_u("getty@.service/tty2", 0, "getty@tty2.service");
+        check_c_t_u("getty@.service/tty2/xxx", 0, "getty@tty2.service");
+        check_c_t_u("getty@.service/", -EINVAL, NULL);
+        check_c_t_u("getty@.service", -EINVAL, NULL);
+        check_c_t_u("getty.service", 0, "getty.service");
+        check_c_t_u("getty", -EINVAL, NULL);
+}
+
+static void check_p_g_u(const char *path, int code, const char *result) {
+        _cleanup_free_ char *unit = NULL;
+
+        assert_se(cg_path_get_unit(path, &unit) == code);
+        assert_se(streq_ptr(unit, result));
+}
+
+static void check_p_g_u_u(const char *path, int code, const char *result) {
+        _cleanup_free_ char *unit = NULL;
+
+        assert_se(cg_path_get_user_unit(path, &unit) == code);
+        assert_se(streq_ptr(unit, result));
+}
+
+static void test_path_get_unit(void) {
+        check_p_g_u("/system/foobar.service/sdfdsaf", 0, "foobar.service");
+        check_p_g_u("/system/getty@.service/tty5", 0, "getty@tty5.service");
+        check_p_g_u("/system/getty@.service/tty5/aaa/bbb", 0, "getty@tty5.service");
+        check_p_g_u("/system/getty@.service/tty5/", 0, "getty@tty5.service");
+        check_p_g_u("/system/getty@tty6.service/tty5", 0, "getty@tty6.service");
+        check_p_g_u("sadfdsafsda", -ENOENT, NULL);
+        check_p_g_u("/system/getty####@tty6.service/tty5", -EINVAL, NULL);
+
+        check_p_g_u_u("/user/lennart/2/systemd-21548/foobar.service", 0, "foobar.service");
+        check_p_g_u_u("/user/lennart/2/systemd-21548/foobar.service/waldo", 0, "foobar.service");
+        check_p_g_u_u("/user/lennart/2/systemd-21548/foobar.service/waldo/uuuux", 0, "foobar.service");
+        check_p_g_u_u("/user/lennart/2/systemd-21548/waldo/waldo/uuuux", -EINVAL, NULL);
+        check_p_g_u_u("/user/lennart/2/foobar.service", -ENOENT, NULL);
+        check_p_g_u_u("/user/lennart/2/systemd-21548/foobar@.service/pie/pa/po", 0, "foobar@pie.service");
 }
 
 int main(void) {
         test_cgroup_to_unit();
+        test_path_get_unit();
 
         return 0;
 }