Skip to content

Add CLI option to pass common signals to sandboxed process #586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aaruni96 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add CLI option to pass common signals to sandboxed process #586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b32aa1d
Add option to propagate common signals to sandboxed process
aaruni96 Dec 11, 2024
e33e82f
Make --forward-signals work with --unshare-pid
q3cpma May 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 92 additions & 4 deletions bubblewrap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,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_forward_signals = false;
static uid_t opt_sandbox_uid = -1;
static gid_t opt_sandbox_gid = -1;
static int opt_sync_fd = -1;
Expand Down Expand Up @@ -373,6 +374,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"
" --forward-signals Forward various commonly used signals to the child process.\n"
);
exit (ecode);
}
Expand All @@ -387,6 +389,33 @@ handle_die_with_parent (void)
die_with_error ("prctl");
}

static int forwarded_signals[] =
{
SIGINT,
SIGTERM,
SIGCONT,
SIGHUP,
SIGQUIT,
SIGUSR1,
SIGUSR2,
SIGWINCH,
};

static void
block_forwarded_signals (sigset_t *prevmask)
{
sigset_t mask;
size_t i;

sigemptyset (&mask);

for (i = 0; i < N_ELEMENTS (forwarded_signals); i++)
sigaddset (&mask, forwarded_signals[i]);

if (sigprocmask (SIG_BLOCK, &mask, prevmask) == -1)
die_with_error ("sigprocmask");
}

static void
block_sigchild (void)
{
Expand Down Expand Up @@ -493,7 +522,7 @@ report_child_exit_status (int exitc, int setup_finished_fd)
* pid 1 via a signalfd for SIGCHLD, and exit with an error in this case.
* This is to catch e.g. problems during setup. */
static int
monitor_child (int event_fd, pid_t child_pid, int setup_finished_fd)
monitor_child (int event_fd, pid_t child_pid, pid_t exec_pid, int setup_finished_fd)
{
int res;
uint64_t val;
Expand All @@ -508,6 +537,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. */
Expand All @@ -523,6 +553,9 @@ monitor_child (int event_fd, pid_t child_pid, int setup_finished_fd)
sigemptyset (&mask);
sigaddset (&mask, SIGCHLD);

for (i = 0; i < N_ELEMENTS (forwarded_signals); i++)
sigaddset (&mask, forwarded_signals[i]);
Comment on lines +556 to +557
Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be guarded by if (opt_forward_signals)?


signal_fd = signalfd (-1, &mask, SFD_CLOEXEC | SFD_NONBLOCK);
if (signal_fd == -1)
die_with_error ("Can't create signalfd");
Expand Down Expand Up @@ -561,12 +594,21 @@ 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 executable so that it will take the correct
* action. This avoids the parent terminating, leaving an orphan. */
if (fdsi.ssi_signo != SIGCHLD)
Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (fdsi.ssi_signo != SIGCHLD)
if (opt_forward_signals && fdsi.ssi_signo != SIGCHLD)

Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need a check for child_pid != 0 (or for exec_pid != 0 after the second commit), too.

{
s = kill (exec_pid, fdsi.ssi_signo);
if (s != 0 && s != ESRCH)
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)
Expand Down Expand Up @@ -2742,6 +2784,10 @@ parse_args_recurse (int *argcp,
argc -= 1;
break;
}
else if (strcmp (arg, "--forward-signals") == 0)
{
opt_forward_signals = true;
}
else if (*arg == '-')
{
die ("Unknown option %s", arg);
Expand Down Expand Up @@ -2876,6 +2922,7 @@ main (int argc,
int clone_flags;
char *old_cwd = NULL;
pid_t pid;
pid_t exec_pid = -1;
int event_fd = -1;
int child_wait_fd = -1;
int setup_finished_pipe[] = {-1, -1};
Expand All @@ -2887,8 +2934,11 @@ main (int argc,
int res UNUSED;
cleanup_free char *args_data UNUSED = NULL;
int intermediate_pids_sockets[2] = {-1, -1};
int executable_pids_sockets[2] = {-1, -1};
const char *exec_path = NULL;
int i;
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;
Expand Down Expand Up @@ -3033,6 +3083,10 @@ main (int argc,
if (opt_as_pid_1 && !opt_unshare_pid)
die ("Specifying --as-pid-1 requires --unshare-pid");

/* Because pid 1 ignores signals without handlers (cf pid_namespaces(7)) */
if (opt_as_pid_1 && opt_forward_signals)
die ("Specifying --as-pid-1 and --forward-signals is not permitted");
Comment on lines +3086 to +3088
Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what if the user has specified a "reaper" process to be run as pid 1 that does accept (some of) SIGTERM, etc.? Shouldn't we pass the signals through to it in that situation?

The user already needs to choose their pid 1 carefully, to be something that is designed to be usable as a pid 1 - if it doesn't reap its child processes, then it will leave orphan processes hanging around until it terminates.

Similarly, I think it seems like a reasonable limitation if bwrap --forward-signals --as-pid-1 ... -- my-pid-1 can only forward SIGTERM to my-pid-1 if my-pid-1 has established a signal handler for SIGTERM, and so on - and probably better than having an artificial limitation that that combination of command-line options is forbidden just because it might not work.

As minijail implies, they kept the -I flag for experts who can make sure the program being run will handle this situation as expected.

It seems like minijail -I is the equivalent of bwrap --as-pid-1, which makes it equally necessary to make sure the user-specified program (the COMMAND) will handle situations unique to a pid 1 as expected. It seems like it would be equally reasonable for bwrap to mention in its man page, in the documentation of --as-pid-1, that signals forwarded via --forward-signals will only be received by the COMMAND if it explicitly establishes a signal handler for them.


if (opt_as_pid_1 && lock_files != NULL)
die ("Specifying --as-pid-1 and --lock-file is not permitted");

Expand Down Expand Up @@ -3062,6 +3116,16 @@ 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. */
if (opt_forward_signals)
{
block_forwarded_signals (&sigmask_before_forwarding);
/* We need to get the final executable pid, not just the child's, to forward signals to it */
if (opt_unshare_pid)
Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We run an init process (do_init()) under more circumstances than just this:

  if (!opt_as_pid_1 && (opt_unshare_pid || lock_files != NULL || opt_sync_fd != -1))

so I think you might well need similar logic here.

Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, rather than duplicating the same logic in more than one place, it would be better to have something like

bool need_init_process = (some complicated condition);

...

if (need_init_process)
  do setup that is required for the init process;

...

if (need_init_process)
  actually fork the init process;

create_pid_socketpair (executable_pids_sockets);
}


clone_flags = SIGCHLD | CLONE_NEWNS;
if (opt_unshare_user)
clone_flags |= CLONE_NEWUSER;
Expand Down Expand Up @@ -3209,9 +3273,25 @@ main (int argc,
/* Ignore res, if e.g. the child died and closed child_wait_fd we don't want to error out here */
close (child_wait_fd);

return monitor_child (event_fd, pid, setup_finished_pipe[0]);
if (executable_pids_sockets[0] != -1)
{
close (executable_pids_sockets[1]);
exec_pid = read_pid_from_socket (executable_pids_sockets[0]);
close (executable_pids_sockets[0]);
Comment on lines +3278 to +3280
Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the fds being left "dangling", such that if they are mistakenly close()d again, some other unrelated fd could be closed by mistake:

Suggested change
close (executable_pids_sockets[1]);
exec_pid = read_pid_from_socket (executable_pids_sockets[0]);
close (executable_pids_sockets[0]);
close (executable_pids_sockets[1]);
executable_pids_sockets[1] = -1;
exec_pid = read_pid_from_socket (executable_pids_sockets[0]);
close (executable_pids_sockets[0]);
executable_pids_sockets[0] = -1;

(One day we should make cleanup_fdp() set *fdp = -1 so that it can be used to close-and-invalidate a fd in this way, like GLib's g_clear_fd() - and that would let me resume #665, #666 and #667 for better robustness.)

}
else
exec_pid = pid;
Comment on lines +3282 to +3283
Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace nitpick: if one arm of an if/[else if/.../]else needs {} then I prefer to add them to both arms, even if the other one is only one line and therefore doesn't strictly need them:

Suggested change
else
exec_pid = pid;
else
{
exec_pid = pid;
}


return monitor_child (event_fd, pid, exec_pid, setup_finished_pipe[0]);
}

/* Restore the state of sigmask from before the blocking. */
if (opt_forward_signals)
{
if (sigprocmask (SIG_SETMASK, &sigmask_before_forwarding, NULL) != 0)
die_with_error ("sigprocmask");
}

if (opt_pidns_fd > 0)
{
if (setns (opt_pidns_fd, CLONE_NEWPID) != 0)
Expand Down Expand Up @@ -3564,12 +3644,14 @@ main (int argc,
process).
Any other fds will been passed on to the child though. */
{
int dont_close[3];
int dont_close[4];
int j = 0;
if (event_fd != -1)
dont_close[j++] = event_fd;
if (opt_sync_fd != -1)
dont_close[j++] = opt_sync_fd;
if (executable_pids_sockets[1] != -1)
dont_close[j++] = executable_pids_sockets[1];
dont_close[j++] = -1;
fdwalk (proc_fd, close_extra_fds, dont_close);
}
Expand All @@ -3590,6 +3672,12 @@ main (int argc,
if (opt_sync_fd != -1)
close (opt_sync_fd);
}
/* Send the final executable pid to the monitor for signal forwarding */
if (executable_pids_sockets[1] != -1)
{
send_pid_on_socket (executable_pids_sockets[1]);
close (executable_pids_sockets[1]);
Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

      close (executable_pids_sockets[1]);
      executable_pids_sockets[1] = -1;

}

/* We want sigchild in the child */
unblock_sigchild ();
Expand Down
1 change: 1 addition & 0 deletions tests/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ test_scripts = [
'test-seccomp.py',
'test-specifying-pidns.sh',
'test-specifying-userns.sh',
'test-forward-signals.sh'
]

test_env = environment()
Expand Down
55 changes: 55 additions & 0 deletions tests/test-forward-signals.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
#!/usr/bin/env bash

set -xeuo pipefail

srcd=$(cd $(dirname "$0") && pwd)
. ${srcd}/libtest.sh
test_count=0
ok () {
test_count=$((test_count + 1))
echo ok $test_count "$@"
}
ok_skip () {
ok "# SKIP" "$@"
}
done_testing () {
echo "1..$test_count"
}

sh_path=/bin/sh

out=$(
$RUN \
"$sh_path" -c 'trap "echo USR1; exit" USR1; sleep 0.5' &
sleep 0.1
kill -USR1 $!
)
if [ -z "$out" ]; then
ok "No signals forwarded without --forward-signal"
else
false
Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false will not give a very comprehensible error message if the test fails. Better to have something like:

Suggested change
false
assert_not_reached "Unexpected output from child, was SIGUSR1 forwarded? $out"

or something along those lines

fi

out=$(
$RUN --forward-signals \
"$sh_path" -c 'trap "echo USR1; exit" USR1; sleep 0.5' &
sleep 0.1
Comment on lines +35 to +36
Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An arbitrary sleep in unit tests is race-condition-prone. On slow or heavily-loaded machines, 0.1 seconds might not be long enough for bwrap and sh to work through all of their setup and reach the point after the trap command has been executed.

To make this test reliable, the parent (script) will need to, somehow, wait for the child (bwrap → sh) to have put its trap in place before killing the child.

One way to do this (if I'm remembering my sh arcana correctly) would be:

  1. Use mkfifo to create a fifo: mkfifo wait-for-trap or something
  2. In the -c script, after the trap, write something to the fifo: 'trap ...; echo done > wait-for-trap; sleep ...'
  3. In the parent, instead of the sleep 0.1, use something like cat wait-for-trap >/dev/null, which will block until there is a writer

That way, the interactions with wait-for-trap establish a sequence point: the trap command happens before them, and the kill -USR1 happens after.

Similarly, if the host is really heavily loaded, 0.5 seconds might not be long enough for the parent to wake up and send the signal. A sleep of a few seconds should be enough, though.

kill -USR1 $!
)
if [ "$out" = USR1 ]; then
ok "Successfully forwarded signals with --forward-signals"
else
false
fi

out=$(
$RUN --forward-signals --unshare-pid \
"$sh_path" -c 'trap "echo USR1; exit" USR1; sleep 0.5' &
sleep 0.1
Comment on lines +47 to +48
Copy link
Collaborator

@smcv smcv Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issues with the sleep being race-prone as in the other commit.

kill -USR1 $!
)
if [ "$out" = USR1 ]; then
ok "Successfully forwarded signals with --forward-signals and --unshare-pid"
else
false
fi