From 7feb2b5737ad110eb3985e8e9d8189f18d1c5147 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Feb 2022 12:37:51 +0100 Subject: [PATCH 1/4] pid1: pass PAM_DATA_SILENT to pam_end() in child Fixes: #22318 --- src/core/execute.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index f2b58303df0..be2116e0eca 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1343,7 +1343,9 @@ static int setup_pam( ret = 0; child_finish: - pam_end(handle, pam_code | flags); + /* NB: pam_end() when called in child processes should set PAM_DATA_SILENT to let the module + * know about this. See pam_end(3) */ + (void) pam_end(handle, pam_code | flags | PAM_DATA_SILENT); _exit(ret); } @@ -1378,7 +1380,7 @@ fail: if (close_session) pam_code = pam_close_session(handle, flags); - pam_end(handle, pam_code | flags); + (void) pam_end(handle, pam_code | flags); } strv_free(e); From 46e5bbab5895b7137b03453dee08bd1c89c710e9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Feb 2022 13:49:56 +0100 Subject: [PATCH 2/4] execute: use _cleanup_ logic where appropriate --- src/core/execute.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index be2116e0eca..74dea32a509 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1183,10 +1183,11 @@ static int setup_pam( }; _cleanup_(barrier_destroy) Barrier barrier = BARRIER_NULL; + _cleanup_strv_free_ char **e = NULL; pam_handle_t *handle = NULL; sigset_t old_ss; int pam_code = PAM_SUCCESS, r; - char **nv, **e = NULL; + char **nv; bool close_session = false; pid_t pam_pid = 0, parent_pid; int flags = 0; @@ -1383,9 +1384,7 @@ fail: (void) pam_end(handle, pam_code | flags); } - strv_free(e); closelog(); - return r; #else return 0; From cafc5ca147cb05b90bd731661d8594c299601f79 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Feb 2022 13:50:13 +0100 Subject: [PATCH 3/4] execute: line break comments a bit less aggressively --- src/core/execute.c | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 74dea32a509..0b433fa6250 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1258,8 +1258,7 @@ static int setup_pam( goto fail; } - /* Block SIGTERM, so that we know that it won't get lost in - * the child */ + /* Block SIGTERM, so that we know that it won't get lost in the child */ assert_se(sigprocmask_many(SIG_BLOCK, &old_ss, SIGTERM, -1) >= 0); @@ -1271,18 +1270,16 @@ static int setup_pam( if (r == 0) { int sig, ret = EXIT_PAM; - /* The child's job is to reset the PAM session on - * termination */ + /* The child's job is to reset the PAM session on termination */ barrier_set_role(&barrier, BARRIER_CHILD); /* Make sure we don't keep open the passed fds in this child. We assume that otherwise only * those fds are open here that have been opened by PAM. */ (void) close_many(fds, n_fds); - /* Drop privileges - we don't need any to pam_close_session - * and this will make PR_SET_PDEATHSIG work in most cases. - * If this fails, ignore the error - but expect sd-pam threads - * to fail to exit normally */ + /* Drop privileges - we don't need any to pam_close_session and this will make + * PR_SET_PDEATHSIG work in most cases. If this fails, ignore the error - but expect sd-pam + * threads to fail to exit normally */ r = maybe_setgroups(0, NULL); if (r < 0) @@ -1294,20 +1291,16 @@ static int setup_pam( (void) ignore_signals(SIGPIPE); - /* Wait until our parent died. This will only work if - * the above setresuid() succeeds, otherwise the kernel - * will not allow unprivileged parents kill their privileged - * children this way. We rely on the control groups kill logic - * to do the rest for us. */ + /* Wait until our parent died. This will only work if the above setresuid() succeeds, + * otherwise the kernel will not allow unprivileged parents kill their privileged children + * this way. We rely on the control groups kill logic to do the rest for us. */ 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. + /* 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. * - * If the parent aborted, we'll detect this below, hence ignore - * return failure here. */ + * If the parent aborted, we'll detect this below, hence ignore return failure here. */ (void) barrier_place(&barrier); /* Check if our parent process might already have died? */ @@ -1352,19 +1345,19 @@ static int setup_pam( 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. */ + /* If the child was forked off successfully it will do all the cleanups, so forget about the handle + * here. */ handle = NULL; /* Unblock SIGTERM again in the parent */ assert_se(sigprocmask(SIG_SETMASK, &old_ss, NULL) >= 0); - /* We close the log explicitly here, since the PAM modules - * might have opened it, but we don't want this fd around. */ + /* We close the log explicitly here, since the PAM modules 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. */ + /* 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"); From 421bb42d1b366c00392ef5bbab6a67412295b6dc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Feb 2022 13:50:28 +0100 Subject: [PATCH 4/4] execute: document that the 'env' param is input *and* output --- src/core/execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/execute.c b/src/core/execute.c index 0b433fa6250..380755eb09b 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1172,7 +1172,7 @@ static int setup_pam( uid_t uid, gid_t gid, const char *tty, - char ***env, + char ***env, /* updated on success */ const int fds[], size_t n_fds) { #if HAVE_PAM