From db256aab13d8a89d583ecd2bacf0aca87c66effc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 12:20:22 +0100 Subject: [PATCH] core: be stricter when handling PID files and MAINPID sd_notify() messages Let's be more restrictive when validating PID files and MAINPID= messages: don't accept PIDs that make no sense, and if the configuration source is not trusted, don't accept out-of-cgroup PIDs. A configuratin source is considered trusted when the PID file is owned by root, or the message was received from root. This should lock things down a bit, in case service authors write out PID files from unprivileged code or use NotifyAccess=all with unprivileged code. Note that doing so was always problematic, just now it's a bit less problematic. When we open the PID file we'll now use the CHASE_SAFE chase_symlinks() logic, to ensure that we won't follow an unpriviled-owned symlink to a privileged-owned file thinking this was a valid privileged PID file, even though it really isn't. Fixes: #6632 --- man/systemd.service.xml | 18 ++--- src/core/manager.c | 17 ++-- src/core/service.c | 137 ++++++++++++++++++++++++-------- src/core/unit.h | 2 +- test/TEST-20-MAINPIDGAMES/Makefile | 4 + test/TEST-20-MAINPIDGAMES/test.sh | 42 ++++++++++ test/TEST-20-MAINPIDGAMES/testsuite.sh | 141 +++++++++++++++++++++++++++++++++ test/test-functions | 2 +- 8 files changed, 313 insertions(+), 50 deletions(-) create mode 100644 test/TEST-20-MAINPIDGAMES/Makefile create mode 100755 test/TEST-20-MAINPIDGAMES/test.sh create mode 100755 test/TEST-20-MAINPIDGAMES/testsuite.sh diff --git a/man/systemd.service.xml b/man/systemd.service.xml index b98264c..e796245 100644 --- a/man/systemd.service.xml +++ b/man/systemd.service.xml @@ -264,16 +264,14 @@ PIDFile= - Takes an absolute filename pointing to the - PID file of this daemon. Use of this option is recommended for - services where Type= is set to - . systemd will read the PID of the - main process of the daemon after start-up of the service. - systemd will not write to the file configured here, although - it will remove the file after the service has shut down if it - still exists. - - + Takes an absolute path referring to the PID file of the service. Usage of this option is + recommended for services where Type= is set to . The service manager + will read the PID of the main process of the service from this file after start-up of the service. The service + manager will not write to the file configured here, although it will remove the file after the service has shut + down if it still exists. The PID file does not need to be owned by a privileged user, but if it is owned by an + unprivileged user additional safety restrictions are enforced: the file may not be a symlink to a file owned by + a different user (neither directly nor indirectly), and the PID file must refer to a process already belonging + to the service. diff --git a/src/core/manager.c b/src/core/manager.c index 35cd9ee..f922202 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1879,11 +1879,18 @@ static int manager_dispatch_cgroups_agent_fd(sd_event_source *source, int fd, ui return 0; } -static void manager_invoke_notify_message(Manager *m, Unit *u, pid_t pid, const char *buf, FDSet *fds) { +static void manager_invoke_notify_message( + Manager *m, + Unit *u, + const struct ucred *ucred, + const char *buf, + FDSet *fds) { + _cleanup_strv_free_ char **tags = NULL; assert(m); assert(u); + assert(ucred); assert(buf); tags = strv_split(buf, NEWLINE); @@ -1893,7 +1900,7 @@ static void manager_invoke_notify_message(Manager *m, Unit *u, pid_t pid, const } if (UNIT_VTABLE(u)->notify_message) - UNIT_VTABLE(u)->notify_message(u, pid, tags, fds); + UNIT_VTABLE(u)->notify_message(u, ucred, tags, fds); else if (DEBUG_LOGGING) { _cleanup_free_ char *x = NULL, *y = NULL; @@ -2001,15 +2008,15 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t * to avoid notifying the same one multiple times. */ u1 = manager_get_unit_by_pid_cgroup(m, ucred->pid); if (u1) - manager_invoke_notify_message(m, u1, ucred->pid, buf, fds); + manager_invoke_notify_message(m, u1, ucred, buf, fds); u2 = hashmap_get(m->watch_pids1, PID_TO_PTR(ucred->pid)); if (u2 && u2 != u1) - manager_invoke_notify_message(m, u2, ucred->pid, buf, fds); + manager_invoke_notify_message(m, u2, ucred, buf, fds); u3 = hashmap_get(m->watch_pids2, PID_TO_PTR(ucred->pid)); if (u3 && u3 != u2 && u3 != u1) - manager_invoke_notify_message(m, u3, ucred->pid, buf, fds); + manager_invoke_notify_message(m, u3, ucred, buf, fds); if (!u1 && !u2 && !u3) log_warning("Cannot find unit for notify message of PID "PID_FMT".", ucred->pid); diff --git a/src/core/service.c b/src/core/service.c index c6835a4..fde6406 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -866,9 +866,45 @@ static void service_dump(Unit *u, FILE *f, const char *prefix) { cgroup_context_dump(&s->cgroup_context, f, prefix); } +static int service_is_suitable_main_pid(Service *s, pid_t pid, int prio) { + Unit *owner; + + assert(s); + assert(pid_is_valid(pid)); + + /* Checks whether the specified PID is suitable as main PID for this service. returns negative if not, 0 if the + * PID is questionnable but should be accepted if the source of configuration is trusted. > 0 if the PID is + * good */ + + if (pid == getpid_cached() || pid == 1) { + log_unit_full(UNIT(s), prio, 0, "New main PID "PID_FMT" is the manager, refusing.", pid); + return -EPERM; + } + + if (pid == s->control_pid) { + log_unit_full(UNIT(s), prio, 0, "New main PID "PID_FMT" is the control process, refusing.", pid); + return -EPERM; + } + + if (!pid_is_alive(pid)) { + log_unit_full(UNIT(s), prio, 0, "New main PID "PID_FMT" does not exist or is a zombie.", pid); + return -ESRCH; + } + + owner = manager_get_unit_by_pid(UNIT(s)->manager, pid); + if (owner == UNIT(s)) { + log_unit_debug(UNIT(s), "New main PID "PID_FMT" belongs to service, we are happy.", pid); + return 1; /* Yay, it's definitely a good PID */ + } + + return 0; /* Hmm it's a suspicious PID, let's accept it if configuration source is trusted */ +} + static int service_load_pid_file(Service *s, bool may_warn) { + char procfs[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; _cleanup_free_ char *k = NULL; - int r; + _cleanup_close_ int fd = -1; + int r, prio; pid_t pid; assert(s); @@ -876,30 +912,47 @@ static int service_load_pid_file(Service *s, bool may_warn) { if (!s->pid_file) return -ENOENT; - r = read_one_line_file(s->pid_file, &k); - if (r < 0) { - if (may_warn) - log_unit_info_errno(UNIT(s), r, "PID file %s not readable (yet?) after %s: %m", s->pid_file, service_state_to_string(s->state)); - return r; - } + prio = may_warn ? LOG_INFO : LOG_DEBUG; + + fd = chase_symlinks(s->pid_file, NULL, CHASE_OPEN|CHASE_SAFE, NULL); + if (fd == -EPERM) + return log_unit_full(UNIT(s), prio, fd, "Permission denied while opening PID file or unsafe symlink chain: %s", s->pid_file); + if (fd < 0) + return log_unit_full(UNIT(s), prio, fd, "Can't open PID file %s (yet?) after %s: %m", s->pid_file, service_state_to_string(s->state)); + + /* Let's read the PID file now that we chased it down. But we need to convert the O_PATH fd chase_symlinks() returned us into a proper fd first. */ + xsprintf(procfs, "/proc/self/fd/%i", fd); + r = read_one_line_file(procfs, &k); + if (r < 0) + return log_unit_error_errno(UNIT(s), r, "Can't convert PID files %s O_PATH file descriptor to proper file descriptor: %m", s->pid_file); r = parse_pid(k, &pid); - if (r < 0) { - if (may_warn) - log_unit_info_errno(UNIT(s), r, "Failed to read PID from file %s: %m", s->pid_file); + if (r < 0) + return log_unit_full(UNIT(s), prio, r, "Failed to parse PID from file %s: %m", s->pid_file); + + if (s->main_pid_known && pid == s->main_pid) + return 0; + + r = service_is_suitable_main_pid(s, pid, prio); + if (r < 0) return r; - } + if (r == 0) { + struct stat st; - if (!pid_is_alive(pid)) { - if (may_warn) - log_unit_info(UNIT(s), "PID "PID_FMT" read from file %s does not exist or is a zombie.", pid, s->pid_file); - return -ESRCH; + /* Hmm, it's not clear if the new main PID is safe. Let's allow this if the PID file is owned by root */ + + if (fstat(fd, &st) < 0) + return log_unit_error_errno(UNIT(s), errno, "Failed to fstat() PID file O_PATH fd: %m"); + + if (st.st_uid != 0) { + log_unit_error(UNIT(s), "New main PID "PID_FMT" does not belong to service, and PID file is not owned by root. Refusing.", pid); + return -EPERM; + } + + log_unit_debug(UNIT(s), "New main PID "PID_FMT" does not belong to service, but we'll accept it since PID file is owned by root.", pid); } if (s->main_pid_known) { - if (pid == s->main_pid) - return 0; - log_unit_debug(UNIT(s), "Main PID changing: "PID_FMT" -> "PID_FMT, s->main_pid, pid); service_unwatch_main_pid(s); @@ -915,7 +968,7 @@ static int service_load_pid_file(Service *s, bool may_warn) { if (r < 0) /* FIXME: we need to do something here */ return log_unit_warning_errno(UNIT(s), r, "Failed to watch PID "PID_FMT" for service: %m", pid); - return 0; + return 1; } static void service_search_main_pid(Service *s) { @@ -2981,7 +3034,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { /* Forking services may occasionally move to a new PID. * As long as they update the PID file before exiting the old * PID, they're fine. */ - if (service_load_pid_file(s, false) == 0) + if (service_load_pid_file(s, false) > 0) return; s->main_pid = 0; @@ -3406,37 +3459,55 @@ static bool service_notify_message_authorized(Service *s, pid_t pid, char **tags return true; } -static void service_notify_message(Unit *u, pid_t pid, char **tags, FDSet *fds) { +static void service_notify_message( + Unit *u, + const struct ucred *ucred, + char **tags, + FDSet *fds) { + Service *s = SERVICE(u); bool notify_dbus = false; const char *e; char **i; + int r; assert(u); + assert(ucred); - if (!service_notify_message_authorized(SERVICE(u), pid, tags, fds)) + if (!service_notify_message_authorized(SERVICE(u), ucred->pid, tags, fds)) return; if (DEBUG_LOGGING) { _cleanup_free_ char *cc = NULL; cc = strv_join(tags, ", "); - log_unit_debug(u, "Got notification message from PID "PID_FMT" (%s)", pid, isempty(cc) ? "n/a" : cc); + log_unit_debug(u, "Got notification message from PID "PID_FMT" (%s)", ucred->pid, isempty(cc) ? "n/a" : cc); } /* Interpret MAINPID= */ e = strv_find_startswith(tags, "MAINPID="); if (e && IN_SET(s->state, SERVICE_START, SERVICE_START_POST, SERVICE_RUNNING, SERVICE_RELOAD)) { - if (parse_pid(e, &pid) < 0) - log_unit_warning(u, "Failed to parse MAINPID= field in notification message: %s", e); - else if (pid == s->control_pid) - log_unit_warning(u, "A control process cannot also be the main process"); - else if (pid == getpid_cached() || pid == 1) - log_unit_warning(u, "Service manager can't be main process, ignoring sd_notify() MAINPID= field"); - else if (pid != s->main_pid) { - service_set_main_pid(s, pid); - unit_watch_pid(UNIT(s), pid); - notify_dbus = true; + pid_t new_main_pid; + + if (parse_pid(e, &new_main_pid) < 0) + log_unit_warning(u, "Failed to parse MAINPID= field in notification message, ignoring: %s", e); + else if (!s->main_pid_known || new_main_pid != s->main_pid) { + + r = service_is_suitable_main_pid(s, new_main_pid, LOG_WARNING); + if (r == 0) { + /* The new main PID is a bit suspicous, which is OK if the sender is privileged. */ + + if (ucred->uid == 0) { + log_unit_debug(u, "New main PID "PID_FMT" does not belong to service, but we'll accept it as the request to change it came from a privileged process.", new_main_pid); + r = 1; + } else + log_unit_debug(u, "New main PID "PID_FMT" does not belong to service, refusing.", new_main_pid); + } + if (r > 0) { + service_set_main_pid(s, new_main_pid); + unit_watch_pid(UNIT(s), new_main_pid); + notify_dbus = true; + } } } diff --git a/src/core/unit.h b/src/core/unit.h index 80585ac..f8a1294 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -506,7 +506,7 @@ struct UnitVTable { void (*notify_cgroup_empty)(Unit *u); /* Called whenever a process of this unit sends us a message */ - void (*notify_message)(Unit *u, pid_t pid, char **tags, FDSet *fds); + void (*notify_message)(Unit *u, const struct ucred *ucred, char **tags, FDSet *fds); /* Called whenever a name this Unit registered for comes or goes away. */ void (*bus_name_owner_change)(Unit *u, const char *name, const char *old_owner, const char *new_owner); diff --git a/test/TEST-20-MAINPIDGAMES/Makefile b/test/TEST-20-MAINPIDGAMES/Makefile new file mode 100644 index 0000000..34d7cc6 --- /dev/null +++ b/test/TEST-20-MAINPIDGAMES/Makefile @@ -0,0 +1,4 @@ +BUILD_DIR=$(shell ../../tools/find-build-dir.sh) + +all setup clean run: + @basedir=../.. TEST_BASE_DIR=../ BUILD_DIR=$(BUILD_DIR) ./test.sh --$@ diff --git a/test/TEST-20-MAINPIDGAMES/test.sh b/test/TEST-20-MAINPIDGAMES/test.sh new file mode 100755 index 0000000..b14083a --- /dev/null +++ b/test/TEST-20-MAINPIDGAMES/test.sh @@ -0,0 +1,42 @@ +#!/bin/bash +# -*- mode: shell-script; indent-tabs-mode: nil; sh-basic-offset: 4; -*- +# ex: ts=8 sw=4 sts=4 et filetype=sh +set -e +TEST_DESCRIPTION="test changing main PID" + +. $TEST_BASE_DIR/test-functions + +test_setup() { + create_empty_image + mkdir -p $TESTDIR/root + mount ${LOOPDEV}p1 $TESTDIR/root + + ( + LOG_LEVEL=5 + eval $(udevadm info --export --query=env --name=${LOOPDEV}p2) + + setup_basic_environment + + # setup the testsuite service + cat >$initdir/etc/systemd/system/testsuite.service </tmp/mainpid.sh < /run/mainpidsh/pid +EOF +chmod +x /tmp/mainpid.sh + +systemd-run --unit=mainpidsh.service -p StandardOutput=tty -p StandardError=tty -p Type=forking -p RuntimeDirectory=mainpidsh -p PIDFile=/run/mainpidsh/pid /tmp/mainpid.sh +test `systemctl show -p MainPID --value mainpidsh.service` -eq `cat /run/mainpidsh/pid` + +cat >/tmp/mainpid2.sh < /run/mainpidsh2/pid +chown 1001:1001 /run/mainpidsh2/pid +EOF +chmod +x /tmp/mainpid2.sh + +systemd-run --unit=mainpidsh2.service -p StandardOutput=tty -p StandardError=tty -p Type=forking -p RuntimeDirectory=mainpidsh2 -p PIDFile=/run/mainpidsh2/pid /tmp/mainpid2.sh +test `systemctl show -p MainPID --value mainpidsh2.service` -eq `cat /run/mainpidsh2/pid` + +cat >/dev/shm/mainpid3.sh < /testok + +exit 0 diff --git a/test/test-functions b/test/test-functions index a2f8272..018bdca 100644 --- a/test/test-functions +++ b/test/test-functions @@ -21,7 +21,7 @@ if ! ROOTLIBDIR=$(pkg-config --variable=systemdutildir systemd); then ROOTLIBDIR=/usr/lib/systemd fi -BASICTOOLS="test sh bash setsid loadkeys setfont login sulogin gzip sleep echo mount umount cryptsetup date dmsetup modprobe sed cmp tee rm true false" +BASICTOOLS="test sh bash setsid loadkeys setfont login sulogin gzip sleep echo mount umount cryptsetup date dmsetup modprobe sed cmp tee rm true false chmod chown ln" DEBUGTOOLS="df free ls stty cat ps ln ip route dmesg dhclient mkdir cp ping dhclient strace less grep id tty touch du sort hostname find" STATEDIR="${BUILD_DIR:-.}/test/$(basename $(dirname $(realpath $0)))" -- 2.7.4