Skip to content

Add an option to use private (instead of slave) propagation for bind mounts. #522

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

Draft
ChrysoliteAzalea wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add an option to use private (instead of slave) propagation for bind mounts. #522

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
3885143
Added an option to use private propagation for bind mounts.
ChrysoliteAzalea Jul 24, 2022
6a2838f
Added mentions of private propagation in the manpage and completion f…
ChrysoliteAzalea Jul 25, 2022
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
12 changes: 7 additions & 5 deletions bind-mount.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,6 @@ parse_mountinfo (int proc_fd,
int max_id;
unsigned int n_lines;
int root;

Copy link
Collaborator

@smcv smcv Jul 25, 2022

Choose a reason for hiding this comment

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

Please don't adjust whitespace in locations that you're not otherwise editing.

This blank line is intentional: bubblewrap is mostly written in a C89 style where variable declarations must come before statements, and when writing in that style it's common to have an empty line between the last variable declaration and the first statement.

mountinfo = load_file_at (proc_fd, "self/mountinfo");
if (mountinfo == NULL)
die_with_error ("Can't open /proc/self/mountinfo");
Expand Down Expand Up @@ -376,6 +375,7 @@ parse_mountinfo (int proc_fd,

bind_mount_result
bind_mount (int proc_fd,
int p_priv,
Copy link
Collaborator

@smcv smcv Jul 25, 2022

Choose a reason for hiding this comment

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

This seems like it should be a BIND_PRIVATE flag in bind_option_t options. It's a lot clearer what the flag means if you see

bind_mount (..., BIND_PRIVATE);

rather than

bind_mount (..., 1, ...);    /* what does 1 mean? is it a boolean? is it a length? ... */

or

bind_mount (..., TRUE, ...);    /* what is being set to true here? */

const char *src,
const char *dest,
bind_option_t options)
Expand All @@ -391,10 +391,12 @@ bind_mount (int proc_fd,
cleanup_free char *kernel_case_combination = NULL;
cleanup_fd int dest_fd = -1;
int i;

int current_propagation = 0;
if (p_priv == 1)
current_propagation = MS_PRIVATE;
if (src)
{
if (mount (src, dest, NULL, MS_SILENT | MS_BIND | (recursive ? MS_REC : 0), NULL) != 0)
if (mount (src, dest, NULL, MS_SILENT | MS_BIND | current_propagation | (recursive ? MS_REC : 0), NULL) != 0)
return BIND_MOUNT_ERROR_MOUNT;
}

Expand Down Expand Up @@ -436,7 +438,7 @@ bind_mount (int proc_fd,
new_flags = current_flags | (devices ? 0 : MS_NODEV) | MS_NOSUID | (readonly ? MS_RDONLY : 0);
if (new_flags != current_flags &&
mount ("none", resolved_dest,
NULL, MS_SILENT | MS_BIND | MS_REMOUNT | new_flags, NULL) != 0)
NULL, MS_SILENT | MS_BIND | MS_REMOUNT | new_flags | current_propagation, NULL) != 0)
return BIND_MOUNT_ERROR_REMOUNT_DEST;

/* We need to work around the fact that a bind mount does not apply the flags, so we need to manually
Expand All @@ -451,7 +453,7 @@ bind_mount (int proc_fd,
new_flags = current_flags | (devices ? 0 : MS_NODEV) | MS_NOSUID | (readonly ? MS_RDONLY : 0);
if (new_flags != current_flags &&
mount ("none", mount_tab[i].mountpoint,
NULL, MS_SILENT | MS_BIND | MS_REMOUNT | new_flags, NULL) != 0)
NULL, MS_SILENT | MS_BIND | MS_REMOUNT | new_flags | current_propagation, NULL) != 0)
{
/* If we can't read the mountpoint we can't remount it, but that should
be safe to ignore because its not something the user can access. */
Expand Down
1 change: 1 addition & 0 deletions bind-mount.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ typedef enum
} bind_mount_result;

bind_mount_result bind_mount (int proc_fd,
int p_priv,
const char *src,
const char *dest,
bind_option_t options);
Expand Down
15 changes: 12 additions & 3 deletions bubblewrap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ int opt_userns_block_fd = -1;
int opt_info_fd = -1;
int opt_json_status_fd = -1;
int opt_seccomp_fd = -1;
int opt_propagation = 0;
const char *opt_sandbox_hostname = NULL;
char *opt_args_data = NULL; /* owned */
int opt_userns_fd = -1;
Expand Down Expand Up @@ -331,6 +332,7 @@ usage (int ecode, FILE *out)
" --symlink SRC DEST Create symlink at DEST with target SRC\n"
" --seccomp FD Load and use seccomp rules from FD (not repeatable)\n"
" --add-seccomp-fd FD Load and use seccomp rules from FD (repeatable)\n"
" --private Set mount propagation to private\n"
Copy link
Collaborator

@smcv smcv Jul 25, 2022

Choose a reason for hiding this comment

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

What happens if --private is halfway through the list of arguments? It's not at all obvious whether it affects all mounts (I think this is actually what happens) or whether it only affects mounts that appear after --private.

" --block-fd FD Block on FD until some data to read is available\n"
" --userns-block-fd FD Block on FD until the user namespace is ready\n"
" --info-fd FD Write information about the running container to FD\n"
Expand Down Expand Up @@ -1070,7 +1072,7 @@ privileged_op (int privileged_op_socket,
break;

case PRIV_SEP_OP_REMOUNT_RO_NO_RECURSIVE:
bind_result = bind_mount (proc_fd, NULL, arg2, BIND_READONLY);
bind_result = bind_mount (proc_fd, opt_propagation, NULL, arg2, BIND_READONLY);

if (bind_result != BIND_MOUNT_SUCCESS)
die_with_bind_result (bind_result, errno,
Expand All @@ -1081,7 +1083,7 @@ privileged_op (int privileged_op_socket,
case PRIV_SEP_OP_BIND_MOUNT:
/* We always bind directories recursively, otherwise this would let us
access files that are otherwise covered on the host */
bind_result = bind_mount (proc_fd, arg1, arg2, BIND_RECURSIVE | flags);
bind_result = bind_mount (proc_fd, opt_propagation, arg1, arg2, BIND_RECURSIVE | flags);

if (bind_result != BIND_MOUNT_SUCCESS)
die_with_bind_result (bind_result, errno,
Expand Down Expand Up @@ -2149,6 +2151,8 @@ parse_args_recurse (int *argcp,
argv += 1;
argc -= 1;
}
else if (strcmp (arg, "--private") == 0)
opt_propagation = 1;
else if (strcmp (arg, "--add-seccomp-fd") == 0)
{
int the_fd;
Expand Down Expand Up @@ -2956,7 +2960,12 @@ main (int argc,
/* Mark everything as slave, so that we still
* receive mounts from the real root, but don't
* propagate mounts to the real root. */
if (mount (NULL, "/", NULL, MS_SILENT | MS_SLAVE | MS_REC, NULL) < 0)
int current_propagation;
if (opt_propagation == 0)
current_propagation = MS_SLAVE;
else
current_propagation = MS_PRIVATE;
if (mount (NULL, "/", NULL, MS_SILENT | current_propagation | MS_REC, NULL) < 0)
die_with_error ("Failed to make / slave");

/* Create a tmpfs which we will use as / in the namespace */
Expand Down
4 changes: 4 additions & 0 deletions bwrap.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,10 @@
<term><option>--hostname <arg choice="plain">HOSTNAME</arg></option></term>
<listitem><para>Use a custom hostname in the sandbox (requires <option>--unshare-uts</option>)</para></listitem>
</varlistentry>
<varlistentry>
<term><option>--private</arg></option></term>
<listitem><para>Use private propagation instead of slave one (prevents host mounts from propagating to the sandbox, but may cause some problems, for example, by keeping removable media mounted in the sandbox).</para></listitem>
</varlistentry>
</variablelist>
<para>Options about environment setup:</para>
<variablelist>
Expand Down
1 change: 1 addition & 0 deletions completions/bash/bwrap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ _bwrap() {
--clearenv
--help
--new-session
--private
--unshare-all
--unshare-cgroup
--unshare-cgroup-try
Expand Down
1 change: 1 addition & 0 deletions completions/zsh/_bwrap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ _bwrap_args=(
'--new-session[Create a new terminal session]'
'--perms[Set permissions for next action argument]: :_guard "[0-7]#" "permissions in octal": :->after_perms'
'--pidns[Use this user namespace (as parent namespace if using --unshare-pid)]: :'
'--private[Use private propagation instead of slave]'
'--proc[Mount new procfs on DEST]:mount point for procfs:_files -/'
'--remount-ro[Remount DEST as readonly; does not recursively remount]:mount point to remount read-only:_files'
'--ro-bind-try[Equal to --ro-bind but ignores non-existent SRC]:source:_files:destination:_files'
Expand Down