core: make setup_pam() synchronous

If we spawn a unit with a non-empty 'PAMName=', we fork off a
child-process _inside_ the unit, known as '(sd-pam)', which watches the
session. It waits for the main-process to exit and then finishes it via
pam_close_session(3).

However, the '(sd-pam)' setup is highly asynchronous. There is no
guarantee that process gets spawned before we finish the unit setup.
Therefore, there might be a root-owned process inside of the cgroup of
the unit, thus causing cg_migrate() to error-out with EPERM.

This patch makes setup_pam() synchronous and waits for the '(sd-pam)'
setup to finish before continuing. This guarantees that setresuid(2) was
at least tried before we continue with the child setup of the real unit.
Note that if setresuid(2) fails, we already warn loudly about it. You
really must make sure that you own the passed user if using 'PAMName='.
It seems very plausible to rely on that assumption.

1 files changed, 21 insertions, 2 deletions

diff --git a/src/core/execute.c b/src/core/execute.c
index 6e14848cd4..a0e39ccae9 100644
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -50,6 +50,7 @@
 #include <sys/apparmor.h>
 #endif
 
+#include "barrier.h"
 #include "sd-messages.h"
 #include "rm-rf.h"
 #include "strv.h"
@@ -768,10 +769,11 @@ static int setup_pam(
                 .appdata_ptr = NULL
         };
 
+        _cleanup_(barrier_destroy) Barrier barrier = BARRIER_NULL;
         pam_handle_t *handle = NULL;
         sigset_t old_ss;
         int pam_code = PAM_SUCCESS;
-        int err;
+        int err = 0;
         char **e = NULL;
         bool close_session = false;
         pid_t pam_pid = 0, parent_pid;
@@ -788,6 +790,10 @@ static int setup_pam(
          * daemon. We do things this way to ensure that the main PID
          * of the daemon is the one we initially fork()ed. */
 
+        err = barrier_create(&barrier);
+        if (err < 0)
+                goto fail;
+
         if (log_get_max_level() < LOG_DEBUG)
                 flags |= PAM_SILENT;
 
@@ -836,6 +842,7 @@ static int setup_pam(
 
                 /* The child's job is to reset the PAM session on
                  * termination */
+                barrier_set_role(&barrier, BARRIER_CHILD);
 
                 /* This string must fit in 10 chars (i.e. the length
                  * of "/sbin/init"), to look pretty in /bin/ps */
@@ -863,6 +870,11 @@ static int setup_pam(
                 if (prctl(PR_SET_PDEATHSIG, SIGTERM) < 0)
                         goto child_finish;
 
+                /* Tell the parent that our setup is done. This is especially
+                 * important regarding dropping privileges. Otherwise, unit
+                 * setup might race against our setresuid(2) call. */
+                barrier_place(&barrier);
+
                 /* Check if our parent process might already have
                  * died? */
                 if (getppid() == parent_pid) {
@@ -898,6 +910,8 @@ static int setup_pam(
                 _exit(r);
         }
 
+        barrier_set_role(&barrier, BARRIER_PARENT);
+
         /* If the child was forked off successfully it will do all the
          * cleanups, so forget about the handle here. */
         handle = NULL;
@@ -909,6 +923,11 @@ static int setup_pam(
          * might have opened it, but we don't want this fd around. */
         closelog();
 
+        /* Synchronously wait for the child to initialize. We don't care for
+         * errors as we cannot recover. However, warn loudly if it happens. */
+        if (!barrier_place_and_sync(&barrier))
+                log_error("PAM initialization failed");
+
         *pam_env = e;
         e = NULL;
 
@@ -919,7 +938,7 @@ fail:
                 log_error("PAM failed: %s", pam_strerror(handle, pam_code));
                 err = -EPERM;  /* PAM errors do not map to errno */
         } else {
-                err = log_error_errno(errno, "PAM failed: %m");
+                err = log_error_errno(err < 0 ? err : errno, "PAM failed: %m");
         }
 
         if (handle) {