From 223f192cde9aacd40e0a2e1560de7a01cea0ea67 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 bc75da47..d5377705 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -383,6 +383,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) { @@ -518,6 +537,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) @@ -557,12 +578,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) @@ -2716,6 +2742,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; @@ -2889,6 +2916,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; @@ -3039,6 +3069,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 a665599311d715a3f3c74d9c4e113a0234e7e58d 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 d5377705..a9da2dab 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; @@ -369,6 +370,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); } @@ -384,7 +386,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; @@ -974,7 +976,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, @@ -2603,6 +2613,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); @@ -2917,7 +2931,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) @@ -3070,7 +3085,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 1dcc2099f0fc6b492996fb78c435afb4374bd53a 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 a9da2dab..87af4bb8 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; @@ -370,7 +370,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); } @@ -2613,9 +2613,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 == '-') { @@ -2931,7 +2931,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; @@ -3085,7 +3085,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 e6f098544132f68e1a1342e3286a69af7fec4b8d 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 87af4bb8..f5bf6d00 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -386,7 +386,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; @@ -984,7 +984,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 8303eac656a939f4a9d8b3eb0dabdd7533cd138c 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 f5bf6d00..8d6078ec 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -385,23 +385,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 @@ -525,6 +532,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. */ @@ -539,8 +547,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) @@ -2756,7 +2767,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; @@ -2932,7 +2944,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) @@ -3084,9 +3096,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 402b66342782871666529347c3f9978096192ce1 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) Signed-off-by: Herwig Hochleitner --- bubblewrap.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index 8d6078ec..1aa9a361 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -987,14 +987,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,