summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDavid Herrmann <dh.herrmann@gmail.com>2015-09-23 00:51:20 +0200
committerDavid Herrmann <dh.herrmann@gmail.com>2015-09-23 00:51:20 +0200
commit2d7c6aa20cef0128e7a90c4da3d3519ed5c6b0f3 (patch)
tree180deba3d9fa1352fb7b6a56ff8c677c7b23c88e
parent3c0fffb74ef7ea572182d9637847e950e9a3a922 (diff)
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.
-rw-r--r--src/core/execute.c23
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) {