From f09b4632a5600180580f4b08fd851aa0d60ff371 Mon Sep 17 00:00:00 2001 From: "Mark A. Grondona" Date: Fri, 1 Nov 2024 20:24:43 +0000 Subject: [PATCH 1/3] imp: linger and forward signals to children of `flux-imp run` Problem: Unlike `exec`, `flux-imp run` doesn't linger and instead directly executes the target command. This then requires the use of `flux-imp kill` to kill the privlieged process(es) that are a result. Have the IMP fork() and exec() the run command. The lingering IMP process can then forward signals delivered to it, including the use of SIGUSR1 as a surrogate for SIGKILL. --- src/imp/run.c | 62 ++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 13 deletions(-) diff --git a/src/imp/run.c b/src/imp/run.c index 309a1b4c..ee7d97b1 100644 --- a/src/imp/run.c +++ b/src/imp/run.c @@ -51,6 +51,7 @@ #include #include +#include #include #include @@ -61,6 +62,7 @@ #include "imp_state.h" #include "impcmd.h" #include "privsep.h" +#include "signals.h" extern char **environ; @@ -142,7 +144,8 @@ static struct kv *get_run_env (struct kv *kv, const cf_t *allowed_env) } static void __attribute__((noreturn)) -imp_run (const char *name, +imp_run (struct imp_state *imp, + const char *name, const cf_t *run_cf, struct kv *kv_env) { @@ -151,6 +154,8 @@ imp_run (const char *name, char **env; const char *args[2]; int exit_code; + int status; + pid_t child; if (!(path = cf_string (cf_get_in (run_cf, "path"))) || path[0] != '/') @@ -181,20 +186,51 @@ imp_run (const char *name, if (chdir ("/") < 0) imp_die (1, "run: failed to chdir to /"); - args[0] = path; - args[1] = NULL; + /* Block signals so parent isn't terminated + */ + imp_sigblock_all (); + + if ((child = fork ()) < 0) + imp_die (1, "run: fork: %s", strerror (errno)); + + imp_set_signal_child (child); + + if (child == 0) { + /* unblock all signals */ + imp_sigunblock_all (); + + args[0] = path; + args[1] = NULL; #if CODE_COVERAGE_ENABLED - __gcov_dump (); - __gcov_reset (); + __gcov_dump (); + __gcov_reset (); #endif - execve (path, (char **) args, env); + execve (path, (char **) args, env); - if (errno == EPERM || errno == EACCES) - exit_code = 126; - else - exit_code = 127; + if (errno == EPERM || errno == EACCES) + exit_code = 126; + else + exit_code = 127; + imp_die (exit_code, "%s: %s", path, strerror (errno)); + } + + /* Parent: + */ + imp_setup_signal_forwarding (imp); - imp_die (exit_code, "%s: %s", path, strerror (errno)); + /* Parent: wait for child to exit */ + while (waitpid (child, &status, 0) != child) { + if (errno != EINTR) + imp_die (1, "waitpid: %s", strerror (errno)); + } + + /* Exit with status of the child process */ + if (WIFEXITED (status)) + exit (WEXITSTATUS (status)); + else if (WIFSIGNALED (status)) + imp_raise (WTERMSIG (status)); + else + exit (1); } @@ -239,7 +275,7 @@ int imp_run_privileged (struct imp_state *imp, || setgid (getegid()) < 0) imp_die (1, "setuid: %s", strerror (errno)); - imp_run (name, cf_run, kv_env); + imp_run (imp, name, cf_run, kv_env); return 0; } @@ -338,7 +374,7 @@ int imp_run_unprivileged (struct imp_state *imp, struct kv *kv) imp_die (1, "run: permission denied"); kv_env = get_run_env (kv, cf_get_in (cf_run, "allowed-environment")); - imp_run (imp->argv[2], cf_run, kv_env); + imp_run (imp, imp->argv[2], cf_run, kv_env); return 0; } From e4df56d06dc1aeb8547d7ddb17831dd6fd4b595a Mon Sep 17 00:00:00 2001 From: "Mark A. Grondona" Date: Sat, 2 Nov 2024 22:00:14 +0000 Subject: [PATCH 2/3] imp: run: don't drop real user/group ids in parent Problem: The IMP run implementation sets the real user and group id of the process to the effective user and group id in the privileged parent, but this makes it so that the invoking user can no longer deliver signals to the IMP parent process. Move the setuid()/setgid() calls to the child process just before execve(2) is called. The parent IMP thereby maintains the real uid/gid of the invoking user and can handle forwarding of signals from that user to the invoked run command. Since the parent IMP process no longer has a real userid of 0/root, update the call that obtains the userid for setting USER and HOME to use the effective uid. --- src/imp/run.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/imp/run.c b/src/imp/run.c index ee7d97b1..06211dd3 100644 --- a/src/imp/run.c +++ b/src/imp/run.c @@ -165,7 +165,7 @@ imp_run (struct imp_state *imp, imp_die (1, "run %s: relative path not allowed", name); /* Get passwd entry for current user to set HOME and USER */ - if (!(pwd = getpwuid (getuid ()))) + if (!(pwd = getpwuid (geteuid ()))) imp_die (1, "run: failed to find target user"); /* Set HOME and USER */ @@ -199,6 +199,10 @@ imp_run (struct imp_state *imp, /* unblock all signals */ imp_sigunblock_all (); + if (setuid (geteuid()) < 0 + || setgid (getegid()) < 0) + imp_die (1, "setuid: %s", strerror (errno)); + args[0] = path; args[1] = NULL; #if CODE_COVERAGE_ENABLED @@ -271,10 +275,6 @@ int imp_run_privileged (struct imp_state *imp, if (!kv_env) imp_die (1, "run: error processing command environment"); - if (setuid (geteuid()) < 0 - || setgid (getegid()) < 0) - imp_die (1, "setuid: %s", strerror (errno)); - imp_run (imp, name, cf_run, kv_env); return 0; From 309b1c3b7452579dd88731d8e31cb236c52d81d6 Mon Sep 17 00:00:00 2001 From: "Mark A. Grondona" Date: Fri, 1 Nov 2024 20:55:07 +0000 Subject: [PATCH 3/3] testsuite: test that IMP lingers with `imp run` Problem: There is no test that ensures the privileged IMP lingers to handle signal delivery with `flux-imp run`. Add a test to t2002-imp-run.t. --- t/t2002-imp-run.t | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/t/t2002-imp-run.t b/t/t2002-imp-run.t index 01e739b9..60f77830 100755 --- a/t/t2002-imp-run.t +++ b/t/t2002-imp-run.t @@ -10,6 +10,8 @@ Basic flux-imp run functionality and corner case handing tests test -n "$FLUX_TESTS_LOGFILE" && set -- "$@" --logfile . `dirname $0`/sharness.sh +test "$chain_lint" = "t" || test_set_prereq NO_CHAIN_LINT + flux_imp=${SHARNESS_BUILD_DIRECTORY}/src/imp/flux-imp sign=${SHARNESS_BUILD_DIRECTORY}/t/src/sign @@ -49,6 +51,10 @@ test_expect_success 'create configs for flux-imp exec and signer' ' path = "$TESTDIR/test.sh" [run.allowednotset] path = "$TESTDIR/test.sh" + [run.sleep] + allowed-users = [ "$(whoami)" ] + allowed-environment = [ "TEST_*", "EXACT_MATCH" ] + path = "$TESTDIR/sleep.sh" EOF ' test_expect_success 'create test shell scripts' ' @@ -61,12 +67,20 @@ test_expect_success 'create test shell scripts' ' env EOF chmod +x $TESTDIR/test.sh && + cat <<-EOF >$TESTDIR/sleep.sh && + #!/bin/sh + echo \$PPID >$TESTDIR/sleep.pid + sleep 30 + EOF + cat $TESTDIR/sleep.sh && + chmod +x $TESTDIR/sleep.sh && touch $TESTDIR/noexec.sh && chmod 600 $TESTDIR/noexec.sh ' test_expect_success SUDO 'set appropriate permissions for sudo based tests' ' $SUDO chown root.root $TESTDIR $TESTDIR/* && $SUDO chmod 755 $TESTDIR $TESTDIR/test.sh && + $SUDO chmod 755 $TESTDIR $TESTDIR/sleep.sh && $SUDO chmod 644 $TESTDIR/test.toml ' @@ -158,6 +172,26 @@ test_expect_success SUDO 'flux-imp run allows FLUX_JOB_ID and FLUX_JOB_USERID' ' grep ^FLUX_JOB_ID=1234 sudo-run-test-uid-jobid.out && grep ^FLUX_JOB_USERID=$(id -u) sudo-run-test-uid-jobid.out ' + +wait_for_file() { + count=0 && + while ! test -f $1; do + sleep 0.1 + count=$((count+1)) + test $count -gt 20 && break + done + test -f $1 +} +test_expect_success SUDO,NO_CHAIN_LINT 'flux-imp run: setuid IMP lingers' ' + $SUDO $flux_imp run sleep & + imp_pid=$! && + wait_for_file $TESTDIR/sleep.pid && + test_when_finished "$SUDO rm -f $pidfile" && + pid=$(cat $TESTDIR/sleep.pid) && + test $(ps --no-header -o comm -p ${pid}) = "flux-imp" && + kill -TERM $pid && + test_expect_code 143 wait $imp_pid +' test_expect_success SUDO 'flux-imp run will not run file with bad ownership' ' $SUDO chown $USER $TESTDIR/test.sh && test_must_fail $SUDO $flux_imp run test &&