Pablo Greco 48fc63
From c0f32feb77768aa76d8c813471b3484c93bc2651 Mon Sep 17 00:00:00 2001
Pablo Greco 48fc63
From: Lennart Poettering <lennart@poettering.net>
Pablo Greco 48fc63
Date: Fri, 5 Jan 2018 12:20:22 +0100
Pablo Greco 48fc63
Subject: [PATCH] core: be stricter when handling PID files and MAINPID
Pablo Greco 48fc63
 sd_notify() messages
Pablo Greco 48fc63
Pablo Greco 48fc63
Let's be more restrictive when validating PID files and MAINPID=
Pablo Greco 48fc63
messages: don't accept PIDs that make no sense, and if the configuration
Pablo Greco 48fc63
source is not trusted, don't accept out-of-cgroup PIDs. A configuratin
Pablo Greco 48fc63
source is considered trusted when the PID file is owned by root, or the
Pablo Greco 48fc63
message was received from root.
Pablo Greco 48fc63
Pablo Greco 48fc63
This should lock things down a bit, in case service authors write out
Pablo Greco 48fc63
PID files from unprivileged code or use NotifyAccess=all with
Pablo Greco 48fc63
unprivileged code. Note that doing so was always problematic, just now
Pablo Greco 48fc63
it's a bit less problematic.
Pablo Greco 48fc63
Pablo Greco 48fc63
When we open the PID file we'll now use the CHASE_SAFE chase_symlinks()
Pablo Greco 48fc63
logic, to ensure that we won't follow an unpriviled-owned symlink to a
Pablo Greco 48fc63
privileged-owned file thinking this was a valid privileged PID file,
Pablo Greco 48fc63
even though it really isn't.
Pablo Greco 48fc63
Pablo Greco 48fc63
Fixes: #6632
Pablo Greco 48fc63
(cherry picked from commit db256aab13d8a89d583ecd2bacf0aca87c66effc)
Pablo Greco 48fc63
Pablo Greco 48fc63
Resolves: #1663143
Pablo Greco 48fc63
---
Pablo Greco 48fc63
 man/systemd.service.xml                |  18 ++-
Pablo Greco 48fc63
 src/core/manager.c                     |  17 ++-
Pablo Greco 48fc63
 src/core/service.c                     | 166 ++++++++++++++++------
Pablo Greco 48fc63
 src/core/unit.h                        |   2 +-
Pablo Greco 48fc63
 test/TEST-20-MAINPIDGAMES/Makefile     |   1 +
Pablo Greco 48fc63
 test/TEST-20-MAINPIDGAMES/test.sh      |  81 +++++++++++
Pablo Greco 48fc63
 test/TEST-20-MAINPIDGAMES/testsuite.sh | 189 +++++++++++++++++++++++++
Pablo Greco 48fc63
 test/test-functions                    |   2 +-
Pablo Greco 48fc63
 8 files changed, 418 insertions(+), 58 deletions(-)
Pablo Greco 48fc63
 create mode 120000 test/TEST-20-MAINPIDGAMES/Makefile
Pablo Greco 48fc63
 create mode 100755 test/TEST-20-MAINPIDGAMES/test.sh
Pablo Greco 48fc63
 create mode 100755 test/TEST-20-MAINPIDGAMES/testsuite.sh
Pablo Greco 48fc63
Pablo Greco 48fc63
diff --git a/man/systemd.service.xml b/man/systemd.service.xml
Pablo Greco 48fc63
index d147e449a6..565a783f72 100644
Pablo Greco 48fc63
--- a/man/systemd.service.xml
Pablo Greco 48fc63
+++ b/man/systemd.service.xml
Pablo Greco 48fc63
@@ -221,16 +221,14 @@
Pablo Greco 48fc63
       <varlistentry>
Pablo Greco 48fc63
         <term><varname>PIDFile=</varname></term>
Pablo Greco 48fc63
 
Pablo Greco 48fc63
-        <listitem><para>Takes an absolute file name pointing to the
Pablo Greco 48fc63
-        PID file of this daemon. Use of this option is recommended for
Pablo Greco 48fc63
-        services where <varname>Type=</varname> is set to
Pablo Greco 48fc63
-        <option>forking</option>. systemd will read the PID of the
Pablo Greco 48fc63
-        main process of the daemon after start-up of the service.
Pablo Greco 48fc63
-        systemd will not write to the file configured here, although
Pablo Greco 48fc63
-        it will remove the file after the service has shut down if it
Pablo Greco 48fc63
-        still exists.
Pablo Greco 48fc63
-        </para>
Pablo Greco 48fc63
-        </listitem>
Pablo Greco 48fc63
+        <listitem><para>Takes an absolute path referring to the PID file of the service. Usage of this option is
Pablo Greco 48fc63
+        recommended for services where <varname>Type=</varname> is set to <option>forking</option>. The service manager
Pablo Greco 48fc63
+        will read the PID of the main process of the service from this file after start-up of the service. The service
Pablo Greco 48fc63
+        manager will not write to the file configured here, although it will remove the file after the service has shut
Pablo Greco 48fc63
+        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
Pablo Greco 48fc63
+        unprivileged user additional safety restrictions are enforced: the file may not be a symlink to a file owned by
Pablo Greco 48fc63
+        a different user (neither directly nor indirectly), and the PID file must refer to a process already belonging
Pablo Greco 48fc63
+        to the service.</para></listitem>
Pablo Greco 48fc63
       </varlistentry>
Pablo Greco 48fc63
 
Pablo Greco 48fc63
       <varlistentry>
Pablo Greco 48fc63
diff --git a/src/core/manager.c b/src/core/manager.c
Pablo Greco 48fc63
index 73d6c81fdb..3bca61d0b1 100644
Pablo Greco 48fc63
--- a/src/core/manager.c
Pablo Greco 48fc63
+++ b/src/core/manager.c
Pablo Greco 48fc63
@@ -1658,11 +1658,18 @@ static int manager_dispatch_cgroups_agent_fd(sd_event_source *source, int fd, ui
Pablo Greco 48fc63
         return 0;
Pablo Greco 48fc63
 }
Pablo Greco 48fc63
 
Pablo Greco 48fc63
-static void manager_invoke_notify_message(Manager *m, Unit *u, pid_t pid, const char *buf, FDSet *fds) {
Pablo Greco 48fc63
+static void manager_invoke_notify_message(
Pablo Greco 48fc63
+                Manager *m,
Pablo Greco 48fc63
+                Unit *u,
Pablo Greco 48fc63
+                const struct ucred *ucred,
Pablo Greco 48fc63
+                const char *buf,
Pablo Greco 48fc63
+                FDSet *fds) {
Pablo Greco 48fc63
+
Pablo Greco 48fc63
         _cleanup_strv_free_ char **tags = NULL;
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         assert(m);
Pablo Greco 48fc63
         assert(u);
Pablo Greco 48fc63
+        assert(ucred);
Pablo Greco 48fc63
         assert(buf);
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         tags = strv_split(buf, "\n\r");
Pablo Greco 48fc63
@@ -1674,7 +1681,7 @@ static void manager_invoke_notify_message(Manager *m, Unit *u, pid_t pid, const
Pablo Greco 48fc63
         log_unit_debug(u->id, "Got notification message for unit %s", u->id);
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         if (UNIT_VTABLE(u)->notify_message)
Pablo Greco 48fc63
-                UNIT_VTABLE(u)->notify_message(u, pid, tags, fds);
Pablo Greco 48fc63
+                UNIT_VTABLE(u)->notify_message(u, ucred, tags, fds);
Pablo Greco 48fc63
         else if (_unlikely_(log_get_max_level() >= LOG_DEBUG)) {
Pablo Greco 48fc63
                 _cleanup_free_ char *x = NULL, *y = NULL;
Pablo Greco 48fc63
 
Pablo Greco 48fc63
@@ -1777,19 +1784,19 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t
Pablo Greco 48fc63
          * to avoid notifying the same one multiple times. */
Pablo Greco 48fc63
         u1 = manager_get_unit_by_pid(m, ucred->pid);
Pablo Greco 48fc63
         if (u1) {
Pablo Greco 48fc63
-                manager_invoke_notify_message(m, u1, ucred->pid, buf, fds);
Pablo Greco 48fc63
+                manager_invoke_notify_message(m, u1, ucred, buf, fds);
Pablo Greco 48fc63
                 found = true;
Pablo Greco 48fc63
         }
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         u2 = hashmap_get(m->watch_pids1, LONG_TO_PTR(ucred->pid));
Pablo Greco 48fc63
         if (u2 && u2 != u1) {
Pablo Greco 48fc63
-                manager_invoke_notify_message(m, u2, ucred->pid, buf, fds);
Pablo Greco 48fc63
+                manager_invoke_notify_message(m, u2, ucred, buf, fds);
Pablo Greco 48fc63
                 found = true;
Pablo Greco 48fc63
         }
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         u3 = hashmap_get(m->watch_pids2, LONG_TO_PTR(ucred->pid));
Pablo Greco 48fc63
         if (u3 && u3 != u2 && u3 != u1) {
Pablo Greco 48fc63
-                manager_invoke_notify_message(m, u3, ucred->pid, buf, fds);
Pablo Greco 48fc63
+                manager_invoke_notify_message(m, u3, ucred, buf, fds);
Pablo Greco 48fc63
                 found = true;
Pablo Greco 48fc63
         }
Pablo Greco 48fc63
 
Pablo Greco 48fc63
diff --git a/src/core/service.c b/src/core/service.c
Pablo Greco 48fc63
index fe6e2ff17c..06b39e3a5a 100644
Pablo Greco 48fc63
--- a/src/core/service.c
Pablo Greco 48fc63
+++ b/src/core/service.c
Pablo Greco 48fc63
@@ -700,9 +700,45 @@ static void service_dump(Unit *u, FILE *f, const char *prefix) {
Pablo Greco 48fc63
         }
Pablo Greco 48fc63
 }
Pablo Greco 48fc63
 
Pablo Greco 48fc63
+static int service_is_suitable_main_pid(Service *s, pid_t pid, int prio) {
Pablo Greco 48fc63
+        Unit *owner;
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        assert(s);
Pablo Greco 48fc63
+        assert(pid > 0);
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        /* Checks whether the specified PID is suitable as main PID for this service. returns negative if not, 0 if the
Pablo Greco 48fc63
+         * PID is questionnable but should be accepted if the source of configuration is trusted. > 0 if the PID is
Pablo Greco 48fc63
+         * good */
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        if (pid == getpid() || pid == 1) {
Pablo Greco 48fc63
+                log_unit_full(UNIT(s)->id, prio, "New main PID "PID_FMT" is the manager, refusing.", pid);
Pablo Greco 48fc63
+                return -EPERM;
Pablo Greco 48fc63
+        }
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        if (pid == s->control_pid) {
Pablo Greco 48fc63
+                log_unit_full(UNIT(s)->id, prio, "New main PID "PID_FMT" is the control process, refusing.", pid);
Pablo Greco 48fc63
+                return -EPERM;
Pablo Greco 48fc63
+        }
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        if (!pid_is_alive(pid)) {
Pablo Greco 48fc63
+                log_unit_full(UNIT(s)->id, prio, "New main PID "PID_FMT" does not exist or is a zombie.", pid);
Pablo Greco 48fc63
+                return -ESRCH;
Pablo Greco 48fc63
+        }
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        owner = manager_get_unit_by_pid(UNIT(s)->manager, pid);
Pablo Greco 48fc63
+        if (owner == UNIT(s)) {
Pablo Greco 48fc63
+                log_unit_debug(UNIT(s)->id, "New main PID "PID_FMT" belongs to service, we are happy.", pid);
Pablo Greco 48fc63
+                return 1; /* Yay, it's definitely a good PID */
Pablo Greco 48fc63
+        }
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        return 0; /* Hmm it's a suspicious PID, let's accept it if configuration source is trusted */
Pablo Greco 48fc63
+}
Pablo Greco 48fc63
+
Pablo Greco 48fc63
 static int service_load_pid_file(Service *s, bool may_warn) {
Pablo Greco 48fc63
+        char procfs[sizeof("/proc/self/fd/") - 1 + DECIMAL_STR_MAX(int)];
Pablo Greco 48fc63
         _cleanup_free_ char *k = NULL;
Pablo Greco 48fc63
-        int r;
Pablo Greco 48fc63
+        _cleanup_close_ int fd = -1;
Pablo Greco 48fc63
+        int r, prio;
Pablo Greco 48fc63
         pid_t pid;
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         assert(s);
Pablo Greco 48fc63
@@ -710,30 +746,47 @@ static int service_load_pid_file(Service *s, bool may_warn) {
Pablo Greco 48fc63
         if (!s->pid_file)
Pablo Greco 48fc63
                 return -ENOENT;
Pablo Greco 48fc63
 
Pablo Greco 48fc63
-        r = read_one_line_file(s->pid_file, &k);
Pablo Greco 48fc63
-        if (r < 0) {
Pablo Greco 48fc63
-                if (may_warn)
Pablo Greco 48fc63
-                        log_unit_info(UNIT(s)->id, "PID file %s not readable (yet?) after %s.", s->pid_file, service_state_to_string(s->state));
Pablo Greco 48fc63
-                return r;
Pablo Greco 48fc63
-        }
Pablo Greco 48fc63
+        prio = may_warn ? LOG_INFO : LOG_DEBUG;
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        fd = chase_symlinks(s->pid_file, NULL, CHASE_OPEN|CHASE_SAFE, NULL);
Pablo Greco 48fc63
+        if (fd == -EPERM)
Pablo Greco 48fc63
+                return log_unit_full(UNIT(s)->id, prio, "Permission denied while opening PID file or unsafe symlink chain: %s", s->pid_file);
Pablo Greco 48fc63
+        if (fd < 0)
Pablo Greco 48fc63
+                return log_unit_full(UNIT(s)->id, prio, "Can't open PID file %s (yet?) after %s: %m", s->pid_file, service_state_to_string(s->state));
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        /* 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. */
Pablo Greco 48fc63
+        xsprintf(procfs, "/proc/self/fd/%i", fd);
Pablo Greco 48fc63
+        r = read_one_line_file(procfs, &k);
Pablo Greco 48fc63
+        if (r < 0)
Pablo Greco 48fc63
+                return log_unit_error_errno(UNIT(s)->id, r, "Can't convert PID files %s O_PATH file descriptor to proper file descriptor: %m", s->pid_file);
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         r = parse_pid(k, &pid;;
Pablo Greco 48fc63
-        if (r < 0) {
Pablo Greco 48fc63
-                if (may_warn)
Pablo Greco 48fc63
-                        log_unit_info_errno(UNIT(s)->id, r, "Failed to read PID from file %s: %m", s->pid_file);
Pablo Greco 48fc63
+        if (r < 0)
Pablo Greco 48fc63
+                return log_unit_full(UNIT(s)->id, prio, "Failed to parse PID from file %s: %m", s->pid_file);
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        if (s->main_pid_known && pid == s->main_pid)
Pablo Greco 48fc63
+                return 0;
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        r = service_is_suitable_main_pid(s, pid, prio);
Pablo Greco 48fc63
+        if (r < 0)
Pablo Greco 48fc63
                 return r;
Pablo Greco 48fc63
-        }
Pablo Greco 48fc63
+        if (r == 0) {
Pablo Greco 48fc63
+                struct stat st;
Pablo Greco 48fc63
 
Pablo Greco 48fc63
-        if (!pid_is_alive(pid)) {
Pablo Greco 48fc63
-                if (may_warn)
Pablo Greco 48fc63
-                        log_unit_info(UNIT(s)->id, "PID "PID_FMT" read from file %s does not exist or is a zombie.", pid, s->pid_file);
Pablo Greco 48fc63
-                return -ESRCH;
Pablo Greco 48fc63
+                /* Hmm, it's not clear if the new main PID is safe. Let's allow this if the PID file is owned by root */
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+                if (fstat(fd, &st) < 0)
Pablo Greco 48fc63
+                        return log_unit_error_errno(UNIT(s)->id, errno, "Failed to fstat() PID file O_PATH fd: %m");
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+                if (st.st_uid != 0) {
Pablo Greco 48fc63
+                        log_unit_error(UNIT(s)->id, "New main PID "PID_FMT" does not belong to service, and PID file is not owned by root. Refusing.", pid);
Pablo Greco 48fc63
+                        return -EPERM;
Pablo Greco 48fc63
+                }
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+                log_unit_debug(UNIT(s)->id, "New main PID "PID_FMT" does not belong to service, but we'll accept it since PID file is owned by root.", pid);
Pablo Greco 48fc63
         }
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         if (s->main_pid_known) {
Pablo Greco 48fc63
-                if (pid == s->main_pid)
Pablo Greco 48fc63
-                        return 0;
Pablo Greco 48fc63
-
Pablo Greco 48fc63
                 log_unit_debug(UNIT(s)->id, "Main PID changing: "PID_FMT" -> "PID_FMT, s->main_pid, pid);
Pablo Greco 48fc63
 
Pablo Greco 48fc63
                 service_unwatch_main_pid(s);
Pablo Greco 48fc63
@@ -752,7 +805,7 @@ static int service_load_pid_file(Service *s, bool may_warn) {
Pablo Greco 48fc63
                 return r;
Pablo Greco 48fc63
         }
Pablo Greco 48fc63
 
Pablo Greco 48fc63
-        return 0;
Pablo Greco 48fc63
+        return 1;
Pablo Greco 48fc63
 }
Pablo Greco 48fc63
 
Pablo Greco 48fc63
 static int service_search_main_pid(Service *s) {
Pablo Greco 48fc63
@@ -2584,7 +2637,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) {
Pablo Greco 48fc63
                 /* Forking services may occasionally move to a new PID.
Pablo Greco 48fc63
                  * As long as they update the PID file before exiting the old
Pablo Greco 48fc63
                  * PID, they're fine. */
Pablo Greco 48fc63
-                if (service_load_pid_file(s, false) == 0)
Pablo Greco 48fc63
+                if (service_load_pid_file(s, false) > 0)
Pablo Greco 48fc63
                         return;
Pablo Greco 48fc63
 
Pablo Greco 48fc63
                 s->main_pid = 0;
Pablo Greco 48fc63
@@ -2957,42 +3010,73 @@ static int service_dispatch_watchdog(sd_event_source *source, usec_t usec, void
Pablo Greco 48fc63
         return 0;
Pablo Greco 48fc63
 }
Pablo Greco 48fc63
 
Pablo Greco 48fc63
-static void service_notify_message(Unit *u, pid_t pid, char **tags, FDSet *fds) {
Pablo Greco 48fc63
+static bool service_notify_message_authorized(Service *s, pid_t pid, char **tags, FDSet *fds) {
Pablo Greco 48fc63
+        assert(s);
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        if (s->notify_access == NOTIFY_NONE) {
Pablo Greco 48fc63
+                log_unit_warning(UNIT(s)->id, "Got notification message from PID "PID_FMT", but reception is disabled.", pid);
Pablo Greco 48fc63
+                return false;
Pablo Greco 48fc63
+        }
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        if (s->notify_access == NOTIFY_MAIN && pid != s->main_pid) {
Pablo Greco 48fc63
+                if (s->main_pid != 0)
Pablo Greco 48fc63
+                        log_unit_warning(UNIT(s)->id, "Got notification message from PID "PID_FMT", but reception only permitted for main PID "PID_FMT, pid, s->main_pid);
Pablo Greco 48fc63
+                else
Pablo Greco 48fc63
+                        log_unit_warning(UNIT(s)->id, "Got notification message from PID "PID_FMT", but reception only permitted for main PID which is currently not known", pid);
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+                return false;
Pablo Greco 48fc63
+        }
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        return true;
Pablo Greco 48fc63
+}
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+static void service_notify_message(
Pablo Greco 48fc63
+                Unit *u,
Pablo Greco 48fc63
+                const struct ucred *ucred,
Pablo Greco 48fc63
+                char **tags,
Pablo Greco 48fc63
+                FDSet *fds) {
Pablo Greco 48fc63
         Service *s = SERVICE(u);
Pablo Greco 48fc63
-        _cleanup_free_ char *cc = NULL;
Pablo Greco 48fc63
         bool notify_dbus = false;
Pablo Greco 48fc63
         const char *e;
Pablo Greco 48fc63
+        int r;
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         assert(u);
Pablo Greco 48fc63
+        assert(ucred);
Pablo Greco 48fc63
 
Pablo Greco 48fc63
-        cc = strv_join(tags, ", ");
Pablo Greco 48fc63
-        log_unit_debug(u->id, "%s: Got notification message from PID "PID_FMT" (%s)",
Pablo Greco 48fc63
-                       u->id, pid, isempty(cc) ? "n/a" : cc);
Pablo Greco 48fc63
+        if (!service_notify_message_authorized(SERVICE(u), ucred->pid, tags, fds))
Pablo Greco 48fc63
+                return;
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         if (s->notify_access == NOTIFY_NONE) {
Pablo Greco 48fc63
-                log_unit_warning(u->id, "%s: Got notification message from PID "PID_FMT", but reception is disabled.", u->id, pid);
Pablo Greco 48fc63
-                return;
Pablo Greco 48fc63
-        }
Pablo Greco 48fc63
+                _cleanup_free_ char *cc = NULL;
Pablo Greco 48fc63
 
Pablo Greco 48fc63
-        if (s->notify_access == NOTIFY_MAIN && pid != s->main_pid) {
Pablo Greco 48fc63
-                if (s->main_pid != 0)
Pablo Greco 48fc63
-                        log_unit_warning(u->id, "%s: Got notification message from PID "PID_FMT", but reception only permitted for main PID "PID_FMT, u->id, pid, s->main_pid);
Pablo Greco 48fc63
-                else
Pablo Greco 48fc63
-                        log_unit_debug(u->id, "%s: Got notification message from PID "PID_FMT", but reception only permitted for main PID which is currently not known", u->id, pid);
Pablo Greco 48fc63
-                return;
Pablo Greco 48fc63
+                cc = strv_join(tags, ", ");
Pablo Greco 48fc63
+                log_unit_debug(u->id, "Got notification message from PID "PID_FMT" (%s)", ucred->pid, isempty(cc) ? "n/a" : cc);
Pablo Greco 48fc63
         }
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         /* Interpret MAINPID= */
Pablo Greco 48fc63
         e = strv_find_startswith(tags, "MAINPID=");
Pablo Greco 48fc63
         if (e && IN_SET(s->state, SERVICE_START, SERVICE_START_POST, SERVICE_RUNNING, SERVICE_RELOAD)) {
Pablo Greco 48fc63
-                if (parse_pid(e, &pid) < 0)
Pablo Greco 48fc63
-                        log_unit_warning(u->id, "Failed to parse MAINPID= field in notification message: %s", e);
Pablo Greco 48fc63
-                else {
Pablo Greco 48fc63
-                        log_unit_debug(u->id, "%s: got MAINPID=%s", u->id, e);
Pablo Greco 48fc63
+                pid_t new_main_pid;
Pablo Greco 48fc63
 
Pablo Greco 48fc63
-                        service_set_main_pid(s, pid);
Pablo Greco 48fc63
-                        unit_watch_pid(UNIT(s), pid);
Pablo Greco 48fc63
-                        notify_dbus = true;
Pablo Greco 48fc63
+                if (parse_pid(e, &new_main_pid) < 0)
Pablo Greco 48fc63
+                        log_unit_warning(u->id, "Failed to parse MAINPID= field in notification message, ignoring: %s", e);
Pablo Greco 48fc63
+                else if (!s->main_pid_known || new_main_pid != s->main_pid) {
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+                        r = service_is_suitable_main_pid(s, new_main_pid, LOG_WARNING);
Pablo Greco 48fc63
+                        if (r == 0) {
Pablo Greco 48fc63
+                                /* The new main PID is a bit suspicous, which is OK if the sender is privileged. */
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+                                if (ucred->uid == 0) {
Pablo Greco 48fc63
+                                        log_unit_debug(u->id, "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);
Pablo Greco 48fc63
+                                        r = 1;
Pablo Greco 48fc63
+                                } else
Pablo Greco 48fc63
+                                        log_unit_debug(u->id, "New main PID "PID_FMT" does not belong to service, refusing.", new_main_pid);
Pablo Greco 48fc63
+                        }
Pablo Greco 48fc63
+                        if (r > 0) {
Pablo Greco 48fc63
+                                service_set_main_pid(s, new_main_pid);
Pablo Greco 48fc63
+                                unit_watch_pid(UNIT(s), new_main_pid);
Pablo Greco 48fc63
+                                notify_dbus = true;
Pablo Greco 48fc63
+                        }
Pablo Greco 48fc63
                 }
Pablo Greco 48fc63
         }
Pablo Greco 48fc63
 
Pablo Greco 48fc63
diff --git a/src/core/unit.h b/src/core/unit.h
Pablo Greco 48fc63
index dfec9cea01..091ef7596e 100644
Pablo Greco 48fc63
--- a/src/core/unit.h
Pablo Greco 48fc63
+++ b/src/core/unit.h
Pablo Greco 48fc63
@@ -376,7 +376,7 @@ struct UnitVTable {
Pablo Greco 48fc63
         void (*notify_cgroup_empty)(Unit *u);
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         /* Called whenever a process of this unit sends us a message */
Pablo Greco 48fc63
-        void (*notify_message)(Unit *u, pid_t pid, char **tags, FDSet *fds);
Pablo Greco 48fc63
+        void (*notify_message)(Unit *u, const struct ucred *ucred, char **tags, FDSet *fds);
Pablo Greco 48fc63
 
Pablo Greco 48fc63
         /* Called whenever a name this Unit registered for comes or
Pablo Greco 48fc63
          * goes away. */
Pablo Greco 48fc63
diff --git a/test/TEST-20-MAINPIDGAMES/Makefile b/test/TEST-20-MAINPIDGAMES/Makefile
Pablo Greco 48fc63
new file mode 120000
Pablo Greco 48fc63
index 0000000000..e9f93b1104
Pablo Greco 48fc63
--- /dev/null
Pablo Greco 48fc63
+++ b/test/TEST-20-MAINPIDGAMES/Makefile
Pablo Greco 48fc63
@@ -0,0 +1 @@
Pablo Greco 48fc63
+../TEST-01-BASIC/Makefile
Pablo Greco 48fc63
\ No newline at end of file
Pablo Greco 48fc63
diff --git a/test/TEST-20-MAINPIDGAMES/test.sh b/test/TEST-20-MAINPIDGAMES/test.sh
Pablo Greco 48fc63
new file mode 100755
Pablo Greco 48fc63
index 0000000000..733532b718
Pablo Greco 48fc63
--- /dev/null
Pablo Greco 48fc63
+++ b/test/TEST-20-MAINPIDGAMES/test.sh
Pablo Greco 48fc63
@@ -0,0 +1,81 @@
Pablo Greco 48fc63
+#!/bin/bash
Pablo Greco 48fc63
+# -*- mode: shell-script; indent-tabs-mode: nil; sh-basic-offset: 4; -*-
Pablo Greco 48fc63
+# ex: ts=8 sw=4 sts=4 et filetype=sh
Pablo Greco 48fc63
+TEST_DESCRIPTION="test changing main PID"
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+. $TEST_BASE_DIR/test-functions
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+check_result_qemu() {
Pablo Greco 48fc63
+    ret=1
Pablo Greco 48fc63
+    mkdir -p $TESTDIR/root
Pablo Greco 48fc63
+    mount ${LOOPDEV}p1 $TESTDIR/root
Pablo Greco 48fc63
+    [[ -e $TESTDIR/root/testok ]] && ret=0
Pablo Greco 48fc63
+    [[ -f $TESTDIR/root/failed ]] && cp -a $TESTDIR/root/failed $TESTDIR
Pablo Greco 48fc63
+    [[ -f $TESTDIR/root/var/log/journal ]] && cp -a $TESTDIR/root/var/log/journal $TESTDIR
Pablo Greco 48fc63
+    umount $TESTDIR/root
Pablo Greco 48fc63
+    [[ -f $TESTDIR/failed ]] && cat $TESTDIR/failed
Pablo Greco 48fc63
+    ls -l $TESTDIR/journal/*/*.journal
Pablo Greco 48fc63
+    test -s $TESTDIR/failed && ret=$(($ret+1))
Pablo Greco 48fc63
+    return $ret
Pablo Greco 48fc63
+}
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+test_run() {
Pablo Greco 48fc63
+    if run_qemu; then
Pablo Greco 48fc63
+        check_result_qemu || return 1
Pablo Greco 48fc63
+    else
Pablo Greco 48fc63
+        dwarn "can't run QEMU, skipping"
Pablo Greco 48fc63
+    fi
Pablo Greco 48fc63
+    if check_nspawn; then
Pablo Greco 48fc63
+        run_nspawn
Pablo Greco 48fc63
+        check_result_nspawn || return 1
Pablo Greco 48fc63
+    else
Pablo Greco 48fc63
+        dwarn "can't run systemd-nspawn, skipping"
Pablo Greco 48fc63
+    fi
Pablo Greco 48fc63
+    return 0
Pablo Greco 48fc63
+}
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+test_setup() {
Pablo Greco 48fc63
+    create_empty_image
Pablo Greco 48fc63
+    mkdir -p $TESTDIR/root
Pablo Greco 48fc63
+    mount ${LOOPDEV}p1 $TESTDIR/root
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+    (
Pablo Greco 48fc63
+        LOG_LEVEL=5
Pablo Greco 48fc63
+        eval $(udevadm info --export --query=env --name=${LOOPDEV}p2)
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        setup_basic_environment
Pablo Greco 48fc63
+        inst_binary cut
Pablo Greco 48fc63
+        inst_binary useradd
Pablo Greco 48fc63
+        inst /etc/login.defs
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        # setup the testsuite service
Pablo Greco 48fc63
+        cat >$initdir/etc/systemd/system/testsuite.service <
Pablo Greco 48fc63
+[Unit]
Pablo Greco 48fc63
+Description=Testsuite service
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+[Service]
Pablo Greco 48fc63
+ExecStart=/bin/bash -x /testsuite.sh
Pablo Greco 48fc63
+Type=oneshot
Pablo Greco 48fc63
+StandardOutput=tty
Pablo Greco 48fc63
+StandardError=tty
Pablo Greco 48fc63
+NotifyAccess=all
Pablo Greco 48fc63
+EOF
Pablo Greco 48fc63
+        cp testsuite.sh $initdir/
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        useradd -R $initdir -U -u 1234 test
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+        setup_testsuite
Pablo Greco 48fc63
+    )
Pablo Greco 48fc63
+    setup_nspawn_root
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+    ddebug "umount $TESTDIR/root"
Pablo Greco 48fc63
+    umount $TESTDIR/root
Pablo Greco 48fc63
+}
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+test_cleanup() {
Pablo Greco 48fc63
+    umount $TESTDIR/root 2>/dev/null
Pablo Greco 48fc63
+    [[ $LOOPDEV ]] && losetup -d $LOOPDEV
Pablo Greco 48fc63
+    return 0
Pablo Greco 48fc63
+}
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+do_test "$@"
Pablo Greco 48fc63
diff --git a/test/TEST-20-MAINPIDGAMES/testsuite.sh b/test/TEST-20-MAINPIDGAMES/testsuite.sh
Pablo Greco 48fc63
new file mode 100755
Pablo Greco 48fc63
index 0000000000..d4ad63865c
Pablo Greco 48fc63
--- /dev/null
Pablo Greco 48fc63
+++ b/test/TEST-20-MAINPIDGAMES/testsuite.sh
Pablo Greco 48fc63
@@ -0,0 +1,189 @@
Pablo Greco 48fc63
+#!/bin/bash
Pablo Greco 48fc63
+# -*- mode: shell-script; indent-tabs-mode: nil; sh-basic-offset: 4; -*-
Pablo Greco 48fc63
+# ex: ts=8 sw=4 sts=4 et filetype=sh
Pablo Greco 48fc63
+set -ex
Pablo Greco 48fc63
+set -o pipefail
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+systemctl_show_value() {
Pablo Greco 48fc63
+    systemctl show "$@" | cut -d = -f 2-
Pablo Greco 48fc63
+}
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+systemd-analyze set-log-level debug
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID testsuite.service` -eq $$
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Start a test process inside of our own cgroup
Pablo Greco 48fc63
+sleep infinity &
Pablo Greco 48fc63
+INTERNALPID=$!
Pablo Greco 48fc63
+disown
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Start a test process outside of our own cgroup
Pablo Greco 48fc63
+systemd-run -p User=test --unit=sleep.service /bin/sleep infinity
Pablo Greco 48fc63
+EXTERNALPID=`systemctl_show_value -p MainPID sleep.service`
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Update our own main PID to the external test PID, this should work
Pablo Greco 48fc63
+systemd-notify MAINPID=$EXTERNALPID
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID testsuite.service` -eq $EXTERNALPID
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Update our own main PID to the internal test PID, this should work, too
Pablo Greco 48fc63
+systemd-notify MAINPID=$INTERNALPID
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID testsuite.service` -eq $INTERNALPID
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Update it back to our own PID, this should also work
Pablo Greco 48fc63
+systemd-notify MAINPID=$$
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID testsuite.service` -eq $$
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Try to set it to PID 1, which it should ignore, because that's the manager
Pablo Greco 48fc63
+systemd-notify MAINPID=1
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID testsuite.service` -eq $$
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Try to set it to PID 0, which is invalid and should be ignored
Pablo Greco 48fc63
+systemd-notify MAINPID=0
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID testsuite.service` -eq $$
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Try to set it to a valid but non-existing PID, which should be ignored. (Note
Pablo Greco 48fc63
+# that we set the PID to a value well above any known /proc/sys/kernel/pid_max,
Pablo Greco 48fc63
+# which means we can be pretty sure it doesn't exist by coincidence)
Pablo Greco 48fc63
+systemd-notify MAINPID=1073741824
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID testsuite.service` -eq $$
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Change it again to the external PID, without priviliges this time. This should be ignored, because the PID is from outside of our cgroup and we lack privileges.
Pablo Greco 48fc63
+systemd-notify --uid=1000 MAINPID=$EXTERNALPID
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID testsuite.service` -eq $$
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Change it again to the internal PID, without priviliges this time. This should work, as the process is on our cgroup, and that's enough even if we lack privileges.
Pablo Greco 48fc63
+systemd-notify --uid=1000 MAINPID=$INTERNALPID
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID testsuite.service` -eq $INTERNALPID
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Update it back to our own PID, this should also work
Pablo Greco 48fc63
+systemd-notify --uid=1000 MAINPID=$$
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID testsuite.service` -eq $$
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+cat >/tmp/mainpid.sh <
Pablo Greco 48fc63
+#!/bin/bash
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+set -eux
Pablo Greco 48fc63
+set -o pipefail
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Create a number of children, and make one the main one
Pablo Greco 48fc63
+sleep infinity &
Pablo Greco 48fc63
+disown
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+sleep infinity &
Pablo Greco 48fc63
+MAINPID=\$!
Pablo Greco 48fc63
+disown
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+sleep infinity &
Pablo Greco 48fc63
+disown
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+echo \$MAINPID > /run/mainpidsh/pid
Pablo Greco 48fc63
+EOF
Pablo Greco 48fc63
+chmod +x /tmp/mainpid.sh
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+cat > /etc/systemd/system/mainpidsh.service <
Pablo Greco 48fc63
+[Unit]
Pablo Greco 48fc63
+Description=MainPID test 1 service
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+[Service]
Pablo Greco 48fc63
+StandardOutput=tty
Pablo Greco 48fc63
+StandardError=tty
Pablo Greco 48fc63
+Type=forking
Pablo Greco 48fc63
+RuntimeDirectory=mainpidsh
Pablo Greco 48fc63
+PIDFile=/run/mainpidsh/pid
Pablo Greco 48fc63
+ExecStart=/tmp/mainpid.sh
Pablo Greco 48fc63
+EOF
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+systemctl daemon-reload
Pablo Greco 48fc63
+systemctl start mainpidsh.service
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID mainpidsh.service` -eq `cat /run/mainpidsh/pid`
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+cat >/tmp/mainpid2.sh <
Pablo Greco 48fc63
+#!/bin/bash
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+set -eux
Pablo Greco 48fc63
+set -o pipefail
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Create a number of children, and make one the main one
Pablo Greco 48fc63
+sleep infinity &
Pablo Greco 48fc63
+disown
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+sleep infinity &
Pablo Greco 48fc63
+MAINPID=\$!
Pablo Greco 48fc63
+disown
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+sleep infinity &
Pablo Greco 48fc63
+disown
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+echo \$MAINPID > /run/mainpidsh2/pid
Pablo Greco 48fc63
+chown 1001:1001 /run/mainpidsh2/pid
Pablo Greco 48fc63
+EOF
Pablo Greco 48fc63
+chmod +x /tmp/mainpid2.sh
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+cat > /etc/systemd/system/mainpidsh2.service <
Pablo Greco 48fc63
+[Unit]
Pablo Greco 48fc63
+Description=MainPID test 2 service
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+[Service]
Pablo Greco 48fc63
+StandardOutput=tty
Pablo Greco 48fc63
+StandardError=tty
Pablo Greco 48fc63
+Type=forking
Pablo Greco 48fc63
+RuntimeDirectory=mainpidsh2
Pablo Greco 48fc63
+PIDFile=/run/mainpidsh2/pid
Pablo Greco 48fc63
+ExecStart=/tmp/mainpid2.sh
Pablo Greco 48fc63
+EOF
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+systemctl daemon-reload
Pablo Greco 48fc63
+systemctl start mainpidsh2.service
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID mainpidsh2.service` -eq `cat /run/mainpidsh2/pid`
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+cat >/dev/shm/mainpid3.sh <
Pablo Greco 48fc63
+#!/bin/bash
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+set -eux
Pablo Greco 48fc63
+set -o pipefail
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+sleep infinity &
Pablo Greco 48fc63
+disown
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+sleep infinity &
Pablo Greco 48fc63
+disown
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+sleep infinity &
Pablo Greco 48fc63
+disown
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Let's try to play games, and link up a privileged PID file
Pablo Greco 48fc63
+ln -s ../mainpidsh/pid /run/mainpidsh3/pid
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Quick assertion that the link isn't dead
Pablo Greco 48fc63
+test -f /run/mainpidsh3/pid
Pablo Greco 48fc63
+EOF
Pablo Greco 48fc63
+chmod 755 /dev/shm/mainpid3.sh
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+cat > /etc/systemd/system/mainpidsh3.service <
Pablo Greco 48fc63
+[Unit]
Pablo Greco 48fc63
+Description=MainPID test 3 service
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+[Service]
Pablo Greco 48fc63
+StandardOutput=tty
Pablo Greco 48fc63
+StandardError=tty
Pablo Greco 48fc63
+Type=forking
Pablo Greco 48fc63
+RuntimeDirectory=mainpidsh3
Pablo Greco 48fc63
+PIDFile=/run/mainpidsh3/pid
Pablo Greco 48fc63
+User=test
Pablo Greco 48fc63
+TimeoutStartSec=2s
Pablo Greco 48fc63
+ExecStart=/dev/shm/mainpid3.sh
Pablo Greco 48fc63
+EOF
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+systemctl daemon-reload
Pablo Greco 48fc63
+systemctl start mainpidsh3.service
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+# Test that this failed due to timeout, and not some other error
Pablo Greco 48fc63
+# test `systemctl_show_value -p Result mainpidsh3.service` = timeout
Pablo Greco 48fc63
+# Just check that there is no MainPID => the pid file was ignored
Pablo Greco 48fc63
+test `systemctl_show_value -p MainPID mainpidsh3.service` -eq 0
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+systemd-analyze set-log-level info
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+echo OK > /testok
Pablo Greco 48fc63
+
Pablo Greco 48fc63
+exit 0
Pablo Greco 48fc63
diff --git a/test/test-functions b/test/test-functions
Pablo Greco 48fc63
index 78e725d5b9..e50ce556fd 100644
Pablo Greco 48fc63
--- a/test/test-functions
Pablo Greco 48fc63
+++ b/test/test-functions
Pablo Greco 48fc63
@@ -12,7 +12,7 @@ if ! ROOTLIBDIR=$(pkg-config --variable=systemdutildir systemd); then
Pablo Greco 48fc63
     ROOTLIBDIR=/usr/lib/systemd
Pablo Greco 48fc63
 fi
Pablo Greco 48fc63
 
Pablo Greco 48fc63
-BASICTOOLS="sh bash setsid loadkeys setfont login sulogin gzip sleep echo mount umount cryptsetup date dmsetup modprobe"
Pablo Greco 48fc63
+BASICTOOLS="test sh bash setsid loadkeys setfont login sulogin gzip sleep echo mount umount cryptsetup date dmsetup modprobe chmod chown ln"
Pablo Greco 48fc63
 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"
Pablo Greco 48fc63
 
Pablo Greco 48fc63
 function find_qemu_bin() {