From e860c0a7c37536b00454abbc5451b8cc1c85f14c Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 30 Oct 2024 14:06:23 +0000 Subject: [PATCH 1/3] Use PIPE_READ_END, PIPE_WRITE_END to clarify use of a socketpair Both sockets in the socket pair are technically bidirectional, but we are using them as though they were unidirectional, with the same convention as pipe(): the first socket in the array is used for reading, and the second is for writing. Signed-off-by: Simon McVittie --- bubblewrap.c | 14 +++++++------- utils.c | 8 +++++++- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index f8728c7e..564c4534 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -3144,11 +3144,11 @@ main (int argc, { /* Parent, outside sandbox, privileged (initially) */ - if (intermediate_pids_sockets[0] != -1) + if (intermediate_pids_sockets[PIPE_READ_END] != -1) { - close (intermediate_pids_sockets[1]); - pid = read_pid_from_socket (intermediate_pids_sockets[0]); - close (intermediate_pids_sockets[0]); + close (intermediate_pids_sockets[PIPE_WRITE_END]); + pid = read_pid_from_socket (intermediate_pids_sockets[PIPE_READ_END]); + close (intermediate_pids_sockets[PIPE_READ_END]); } /* Discover namespace ids before we drop privileges */ @@ -3232,9 +3232,9 @@ main (int argc, /* We're back, either in a child or grandchild, so message the actual pid to the monitor */ - close (intermediate_pids_sockets[0]); - send_pid_on_socket (intermediate_pids_sockets[1]); - close (intermediate_pids_sockets[1]); + close (intermediate_pids_sockets[PIPE_READ_END]); + send_pid_on_socket (intermediate_pids_sockets[PIPE_WRITE_END]); + close (intermediate_pids_sockets[PIPE_WRITE_END]); } /* Child, in sandbox, privileged in the parent or in the user namespace (if --unshare-user). diff --git a/utils.c b/utils.c index 51875aea..4995edda 100644 --- a/utils.c +++ b/utils.c @@ -782,6 +782,12 @@ send_pid_on_socket (int sockfd) die_with_error ("Can't send pid"); } +/* + * Create a socket pair such that if one process calls + * send_pid_on_socket(sockets[PIPE_WRITE_END]), + * another process will be able to call + * read_pid_from_socket(sockets[PIPE_READ_END]). + */ void create_pid_socketpair (int sockets[2]) { @@ -790,7 +796,7 @@ create_pid_socketpair (int sockets[2]) if (socketpair (AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sockets) != 0) die_with_error ("Can't create intermediate pids socket"); - if (setsockopt (sockets[0], SOL_SOCKET, SO_PASSCRED, &enable, sizeof (enable)) < 0) + if (setsockopt (sockets[PIPE_READ_END], SOL_SOCKET, SO_PASSCRED, &enable, sizeof (enable)) < 0) die_with_error ("Can't set SO_PASSCRED"); } From d43805b67befe337a6065b602e0a532c1e122c25 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 30 Oct 2024 14:31:19 +0000 Subject: [PATCH 2/3] Close unused ends of intermediate_pids_sockets sooner Instead of making this conditional and keeping track of the correct condition under which to call it, we can use cleanup_fdp(), which is a no-op when called with a pointer to a negative number, to close the socket unconditionally. In the parent bwrap monitor process (outside the sandbox), we never want to use the write end (which is reserved for the child), so we can and should close it as soon as we have forked. Conversely, in the child process, we never want to use the read end (which is reserved for the parent), so we should close that as soon as we know we are in the child. Signed-off-by: Simon McVittie --- bubblewrap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index 564c4534..8d83fb24 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -3143,10 +3143,10 @@ main (int argc, if (pid != 0) { /* Parent, outside sandbox, privileged (initially) */ + cleanup_fdp (&intermediate_pids_sockets[PIPE_WRITE_END]); if (intermediate_pids_sockets[PIPE_READ_END] != -1) { - close (intermediate_pids_sockets[PIPE_WRITE_END]); pid = read_pid_from_socket (intermediate_pids_sockets[PIPE_READ_END]); close (intermediate_pids_sockets[PIPE_READ_END]); } @@ -3212,6 +3212,8 @@ main (int argc, return monitor_child (event_fd, pid, setup_finished_pipe[0]); } + cleanup_fdp (&intermediate_pids_sockets[PIPE_READ_END]); + if (opt_pidns_fd > 0) { if (setns (opt_pidns_fd, CLONE_NEWPID) != 0) @@ -3231,8 +3233,6 @@ main (int argc, } /* We're back, either in a child or grandchild, so message the actual pid to the monitor */ - - close (intermediate_pids_sockets[PIPE_READ_END]); send_pid_on_socket (intermediate_pids_sockets[PIPE_WRITE_END]); close (intermediate_pids_sockets[PIPE_WRITE_END]); } From 1484a95ccc7bcdbad614d9592e65251dbc9dceda Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 30 Oct 2024 14:32:43 +0000 Subject: [PATCH 3/3] Use cleanup_fdp to close intermediate pid sockets This closes the fd and sets the variable to -1 as a single operation, which is easier to reason about because it does not leave any variables containing dangling references to invalid fds. Signed-off-by: Simon McVittie --- bubblewrap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index 8d83fb24..a176f533 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -3148,7 +3148,7 @@ main (int argc, if (intermediate_pids_sockets[PIPE_READ_END] != -1) { pid = read_pid_from_socket (intermediate_pids_sockets[PIPE_READ_END]); - close (intermediate_pids_sockets[PIPE_READ_END]); + cleanup_fdp (&intermediate_pids_sockets[PIPE_READ_END]); } /* Discover namespace ids before we drop privileges */ @@ -3234,7 +3234,7 @@ main (int argc, /* We're back, either in a child or grandchild, so message the actual pid to the monitor */ send_pid_on_socket (intermediate_pids_sockets[PIPE_WRITE_END]); - close (intermediate_pids_sockets[PIPE_WRITE_END]); + cleanup_fdp (&intermediate_pids_sockets[PIPE_WRITE_END]); } /* Child, in sandbox, privileged in the parent or in the user namespace (if --unshare-user).