From 490584c23dd256d5e494b18a4823ff6f61def965 Mon Sep 17 00:00:00 2001 From: Aaruni Kaushik Date: Mon, 24 Jul 2023 14:57:21 +0200 Subject: [PATCH 1/6] Add patch from https://github.com/containers/bubblewrap/pull/402 [bubblewrap] Propagate SIGTERM and SIGINT to child Co-authored-by: Earl Chew Signed-off-by: Aaruni Kaushik --- bubblewrap.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/bubblewrap.c b/bubblewrap.c index 9b78a9ae..1ec2fd42 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -381,6 +381,25 @@ handle_die_with_parent (void) die_with_error ("prctl"); } +static void +gate_signals (int action, sigset_t *prevmask) +{ + sigset_t mask; + + /* When unblocking, only restore if not previously blocked. */ + + sigemptyset (&mask); + + if (action == SIG_BLOCK || !sigismember (prevmask, SIGINT)) + sigaddset (&mask, SIGINT); + + if (action == SIG_BLOCK || !sigismember (prevmask, SIGTERM)) + sigaddset (&mask, SIGTERM); + + if (sigprocmask (action, &mask, prevmask) == -1) + die_with_error ("sigprocmask"); +} + static void block_sigchild (void) { @@ -516,6 +535,8 @@ monitor_child (int event_fd, pid_t child_pid, int setup_finished_fd) sigemptyset (&mask); sigaddset (&mask, SIGCHLD); + sigaddset (&mask, SIGINT); + sigaddset (&mask, SIGTERM); signal_fd = signalfd (-1, &mask, SFD_CLOEXEC | SFD_NONBLOCK); if (signal_fd == -1) @@ -555,12 +576,17 @@ monitor_child (int event_fd, pid_t child_pid, int setup_finished_fd) } /* We need to read the signal_fd, or it will keep polling as read, - * however we ignore the details as we get them from waitpid + * however we ignore the details for SIGCHLD as we get them from waitpid * below anyway */ s = read (signal_fd, &fdsi, sizeof (struct signalfd_siginfo)); if (s == -1 && errno != EINTR && errno != EAGAIN) die_with_error ("read signalfd"); + /* Propagate signal to child so that it will take the correct + * action. This avoids the parent terminating, leaving an orphan. */ + if (fdsi.ssi_signo != SIGCHLD && kill (child_pid, fdsi.ssi_signo)) + die_with_error ("kill child"); + /* We may actually get several sigchld compressed into one SIGCHLD, so we have to handle all of them. */ while ((died_pid = waitpid (-1, &died_status, WNOHANG)) > 0) @@ -2666,6 +2692,7 @@ main (int argc, cleanup_free char *args_data UNUSED = NULL; int intermediate_pids_sockets[2] = {-1, -1}; const char *exec_path = NULL; + sigset_t sigmask; /* Handle --version early on before we try to acquire/drop * any capabilities so it works in a build environment; @@ -2839,6 +2866,9 @@ main (int argc, /* We block sigchild here so that we can use signalfd in the monitor. */ block_sigchild (); + /* We block other signals here to avoid leaving an orphan. */ + gate_signals (SIG_BLOCK, &sigmask); + clone_flags = SIGCHLD | CLONE_NEWNS; if (opt_unshare_user) clone_flags |= CLONE_NEWUSER; @@ -2989,6 +3019,9 @@ main (int argc, return monitor_child (event_fd, pid, setup_finished_pipe[0]); } + /* Unblock other signals here to receive signals from the parent. */ + gate_signals (SIG_UNBLOCK, &sigmask); + if (opt_pidns_fd > 0) { if (setns (opt_pidns_fd, CLONE_NEWPID) != 0) From 28016e9451b6995f0b2a3613543da6232b65ed8e Mon Sep 17 00:00:00 2001 From: Aaruni Kaushik Date: Tue, 1 Aug 2023 16:07:31 +0200 Subject: [PATCH 2/6] Add option to propagate SIGTERM,SIGINT to child Signed-off-by: Aaruni Kaushik --- bubblewrap.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index 1ec2fd42..a46783e5 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -87,6 +87,7 @@ static bool opt_unshare_cgroup_try = FALSE; static bool opt_needs_devpts = FALSE; static bool opt_new_session = FALSE; static bool opt_die_with_parent = FALSE; +static bool opt_signal_propogate = FALSE; static uid_t opt_sandbox_uid = -1; static gid_t opt_sandbox_gid = -1; static int opt_sync_fd = -1; @@ -367,6 +368,7 @@ usage (int ecode, FILE *out) " --perms OCTAL Set permissions of next argument (--bind-data, --file, etc.)\n" " --size BYTES Set size of next argument (only for --tmpfs)\n" " --chmod OCTAL PATH Change permissions of PATH (must already exist)\n" + " --no-int-term Don't handle SIGINT and SIGTERM, but pass them to sandboxed process.\n" ); exit (ecode); } @@ -382,7 +384,7 @@ handle_die_with_parent (void) } static void -gate_signals (int action, sigset_t *prevmask) +gate_signals (int action, sigset_t *prevmask) // here { sigset_t mask; @@ -972,7 +974,15 @@ drop_privs (bool keep_requested_caps, die_with_error ("can't set dumpable"); } -static void +static char * +get_newroot_path (const char *path) +{ + while (*path == '/') + path++; + return strconcat ("/newroot/", path); +} + +static void //fix for uid maps range, instead of single will come here | but that's for later... write_uid_gid_map (uid_t sandbox_uid, uid_t parent_uid, uid_t sandbox_gid, @@ -2553,6 +2563,10 @@ parse_args_recurse (int *argcp, argc -= 1; break; } + else if (strcmp (arg, "--no-int-term") == 0) + { + opt_signal_propogate = TRUE; + } else if (*arg == '-') { die ("Unknown option %s", arg); @@ -2867,7 +2881,8 @@ main (int argc, block_sigchild (); /* We block other signals here to avoid leaving an orphan. */ - gate_signals (SIG_BLOCK, &sigmask); + if (opt_signal_propogate) + gate_signals (SIG_BLOCK, &sigmask); clone_flags = SIGCHLD | CLONE_NEWNS; if (opt_unshare_user) @@ -3020,7 +3035,8 @@ main (int argc, } /* Unblock other signals here to receive signals from the parent. */ - gate_signals (SIG_UNBLOCK, &sigmask); + if (opt_signal_propogate) + gate_signals (SIG_UNBLOCK, &sigmask); if (opt_pidns_fd > 0) { From 38cf0a119dd561103b620a9bd0a917caab2ca09c Mon Sep 17 00:00:00 2001 From: Aaruni Kaushik Date: Mon, 5 Feb 2024 14:20:42 +0100 Subject: [PATCH 3/6] Rename to --forward-signals Signed-off-by: Aaruni Kaushik --- bubblewrap.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index a46783e5..4b320b10 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -87,7 +87,7 @@ static bool opt_unshare_cgroup_try = FALSE; static bool opt_needs_devpts = FALSE; static bool opt_new_session = FALSE; static bool opt_die_with_parent = FALSE; -static bool opt_signal_propogate = FALSE; +static bool opt_forward_signals = FALSE; static uid_t opt_sandbox_uid = -1; static gid_t opt_sandbox_gid = -1; static int opt_sync_fd = -1; @@ -368,7 +368,7 @@ usage (int ecode, FILE *out) " --perms OCTAL Set permissions of next argument (--bind-data, --file, etc.)\n" " --size BYTES Set size of next argument (only for --tmpfs)\n" " --chmod OCTAL PATH Change permissions of PATH (must already exist)\n" - " --no-int-term Don't handle SIGINT and SIGTERM, but pass them to sandboxed process.\n" + " --forward-signals Forward SIGNALs to the child process.\n" ); exit (ecode); } @@ -2563,9 +2563,9 @@ parse_args_recurse (int *argcp, argc -= 1; break; } - else if (strcmp (arg, "--no-int-term") == 0) + else if (strcmp (arg, "--forward-signals") == 0) { - opt_signal_propogate = TRUE; + opt_forward_signals = TRUE; } else if (*arg == '-') { @@ -2881,7 +2881,7 @@ main (int argc, block_sigchild (); /* We block other signals here to avoid leaving an orphan. */ - if (opt_signal_propogate) + if (opt_forward_signals) gate_signals (SIG_BLOCK, &sigmask); clone_flags = SIGCHLD | CLONE_NEWNS; @@ -3035,7 +3035,7 @@ main (int argc, } /* Unblock other signals here to receive signals from the parent. */ - if (opt_signal_propogate) + if (opt_forward_signals) gate_signals (SIG_UNBLOCK, &sigmask); if (opt_pidns_fd > 0) From 9ea946f6b78608fb371f0645d96015d6333bd25d Mon Sep 17 00:00:00 2001 From: Aaruni Kaushik Date: Mon, 5 Feb 2024 14:24:01 +0100 Subject: [PATCH 4/6] Remove comments unrelated to this PR Signed-off-by: Aaruni Kaushik --- bubblewrap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index 4b320b10..ae78e319 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -384,7 +384,7 @@ handle_die_with_parent (void) } static void -gate_signals (int action, sigset_t *prevmask) // here +gate_signals (int action, sigset_t *prevmask) { sigset_t mask; @@ -982,7 +982,7 @@ get_newroot_path (const char *path) return strconcat ("/newroot/", path); } -static void //fix for uid maps range, instead of single will come here | but that's for later... +static void write_uid_gid_map (uid_t sandbox_uid, uid_t parent_uid, uid_t sandbox_gid, From f804c07fc8e90a5c7dc8f85de67471f4c1557504 Mon Sep 17 00:00:00 2001 From: Aaruni Kaushik Date: Mon, 5 Feb 2024 15:05:49 +0100 Subject: [PATCH 5/6] Rework logic and naming according to review. Signed-off-by: Aaruni Kaushik --- bubblewrap.c | 49 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index ae78e319..abf73ec9 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -383,23 +383,30 @@ handle_die_with_parent (void) die_with_error ("prctl"); } +static int forwarded_signals[] = +{ + SIGINT, + SIGTERM, + SIGCONT, + SIGHUP, + SIGQUIT, + SIGUSR1, + SIGUSR2, + SIGWINCH, +}; + static void -gate_signals (int action, sigset_t *prevmask) +block_forwarded_signals (sigset_t *prevmask) { sigset_t mask; - - /* When unblocking, only restore if not previously blocked. */ + size_t i; sigemptyset (&mask); - if (action == SIG_BLOCK || !sigismember (prevmask, SIGINT)) - sigaddset (&mask, SIGINT); - - if (action == SIG_BLOCK || !sigismember (prevmask, SIGTERM)) - sigaddset (&mask, SIGTERM); - - if (sigprocmask (action, &mask, prevmask) == -1) - die_with_error ("sigprocmask"); + for (i = 0; i < N_ELEMENTS (forwarded_signals); i++) + { + sigaddset (&mask, forwarded_signals[i]); + } } static void @@ -523,6 +530,7 @@ monitor_child (int event_fd, pid_t child_pid, int setup_finished_fd) int exitc; pid_t died_pid; int died_status; + size_t i; /* Close all extra fds in the monitoring process. Any passed in fds have been passed on to the child anyway. */ @@ -537,8 +545,11 @@ monitor_child (int event_fd, pid_t child_pid, int setup_finished_fd) sigemptyset (&mask); sigaddset (&mask, SIGCHLD); - sigaddset (&mask, SIGINT); - sigaddset (&mask, SIGTERM); + + for (i = 0; i < N_ELEMENTS(forwarded_signals); i++) + { + sigaddset(&mask, forwarded_signals[i]); + } signal_fd = signalfd (-1, &mask, SFD_CLOEXEC | SFD_NONBLOCK); if (signal_fd == -1) @@ -2706,7 +2717,8 @@ main (int argc, cleanup_free char *args_data UNUSED = NULL; int intermediate_pids_sockets[2] = {-1, -1}; const char *exec_path = NULL; - sigset_t sigmask; + sigset_t sigmask_before_forwarding; + sigemptyset (&sigmask_before_forwarding); /* Handle --version early on before we try to acquire/drop * any capabilities so it works in a build environment; @@ -2882,7 +2894,7 @@ main (int argc, /* We block other signals here to avoid leaving an orphan. */ if (opt_forward_signals) - gate_signals (SIG_BLOCK, &sigmask); + block_forwarded_signals (&sigmask_before_forwarding); clone_flags = SIGCHLD | CLONE_NEWNS; if (opt_unshare_user) @@ -3034,9 +3046,12 @@ main (int argc, return monitor_child (event_fd, pid, setup_finished_pipe[0]); } - /* Unblock other signals here to receive signals from the parent. */ + /* Restore the state of sigmask from before the blocking. */ if (opt_forward_signals) - gate_signals (SIG_UNBLOCK, &sigmask); + { + if (sigprocmask (SIG_SETMASK, &sigmask_before_forwarding, NULL) != 0) + die_with_error ("sigprocmask"); + } if (opt_pidns_fd > 0) { From 25ec39a3acb61499703e7f68e1ebd301740b4503 Mon Sep 17 00:00:00 2001 From: Herwig Hochleitner Date: Fri, 12 Jul 2024 16:44:56 +0200 Subject: [PATCH 6/6] remove bubblewrap.c::get_newroot_path (defined in utils.c) --- bubblewrap.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index abf73ec9..125ff385 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -985,14 +985,6 @@ drop_privs (bool keep_requested_caps, die_with_error ("can't set dumpable"); } -static char * -get_newroot_path (const char *path) -{ - while (*path == '/') - path++; - return strconcat ("/newroot/", path); -} - static void write_uid_gid_map (uid_t sandbox_uid, uid_t parent_uid,