Skip to content
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

Implement --compat #572

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+112 −9
Open

Implement --compat #572

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
58fb6d3
implement --compat and flip some defaults
Jun 17, 2023
64ca7d1
document changes in bwrap.xml
Jun 17, 2023
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
84 changes: 75 additions & 9 deletions bubblewrap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@
* 2^64 - 2^12. */
#define MAX_TMPFS_BYTES ((size_t) (SIZE_MAX >> 1))

#define LATEST_COMPAT_LEVEL 1

/* Globals to avoid having to use getuid(), since the uid/gid changes during runtime */
static uid_t real_uid;
static gid_t real_gid;
Expand All @@ -75,6 +77,7 @@ static bool opt_as_pid_1;
static const char *opt_chdir_path = NULL;
static bool opt_assert_userns_disabled = FALSE;
static bool opt_disable_userns = FALSE;
static bool opt_disable_userns_set = FALSE;
static bool opt_unshare_user = FALSE;
static bool opt_unshare_user_try = FALSE;
static bool opt_unshare_pid = FALSE;
Expand All @@ -85,6 +88,7 @@ static bool opt_unshare_cgroup = FALSE;
static bool opt_unshare_cgroup_try = FALSE;
static bool opt_needs_devpts = FALSE;
static bool opt_new_session = FALSE;
static bool opt_new_session_set = FALSE;
static bool opt_die_with_parent = FALSE;
static uid_t opt_sandbox_uid = -1;
static gid_t opt_sandbox_gid = -1;
Expand All @@ -99,6 +103,7 @@ static char *opt_args_data = NULL; /* owned */
static int opt_userns_fd = -1;
static int opt_userns2_fd = -1;
static int opt_pidns_fd = -1;
static int opt_compat_level = 0;
static int next_perms = -1;
static size_t next_size_arg = 0;

Expand Down Expand Up @@ -308,6 +313,7 @@ usage (int ecode, FILE *out)
fprintf (out,
" --help Print this help\n"
" --version Print version\n"
" --compat Set compatability level (negative value means latest)\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
" --compat Set compatability level (negative value means latest)\n"
" --compat Set compatibility level (negative value means latest)\n"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think a negative value to mean "latest" is a good idea. The whole point of having a compat level is that it's an opt-in to say "I've changed my code to cope with incompatible changes to the meaning of the command-line", but it can't possibly be correct to say that if you don't know what new compat levels have been introduced since you wrote your code, because if that's the case, you can't know what incompatible changes they would have included.

" --args FD Parse NUL-separated args from FD\n"
" --unshare-all Unshare every namespace we support by default\n"
" --share-net Retain the network namespace (can only combine with --unshare-all)\n"
Expand All @@ -321,7 +327,7 @@ usage (int ecode, FILE *out)
" --unshare-cgroup-try Create new cgroup namespace if possible else continue by skipping it\n"
" --userns FD Use this user namespace (cannot combine with --unshare-user)\n"
" --userns2 FD After setup switch to this user namespace, only useful with --userns\n"
" --disable-userns Disable further use of user namespaces inside sandbox\n"
"%s" /* --(disable/allow)-userns */
Comment on lines -324 to +330
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer these options to always be allowed, which means the --help can go back to being static. You could say something like

           "    --disable-userns             Disable further use of user namespaces inside sandbox (default in compat >= 1)\n"
           "    --allow-userns               Allow further use of user namespaces inside sandbox (default in compat < 1)\n"

" --assert-userns-disabled Fail unless further use of user namespace inside sandbox is disabled\n"
" --pidns FD Use this pid namespace (as parent namespace if using --unshare-pid)\n"
" --uid UID Custom uid in the sandbox (requires --unshare-user or --userns)\n"
Expand Down Expand Up @@ -357,15 +363,20 @@ usage (int ecode, FILE *out)
" --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"
" --json-status-fd FD Write container status to FD as multiple JSON documents\n"
" --new-session Create a new terminal session\n"
"%s" /* -(-no)-new-session */
" --die-with-parent Kills with SIGKILL child process (COMMAND) when bwrap or bwrap's parent dies.\n"
" --as-pid-1 Do not install a reaper process with PID=1\n"
" --cap-add CAP Add cap CAP when running as privileged user\n"
" --cap-drop CAP Drop cap CAP when running as privileged user\n"
" --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"
);
" --chmod OCTAL PATH Change permissions of PATH (must already exist)\n",
opt_compat_level == 0 ?
" --disable-userns Disable further use of user namespaces inside sandbox\n" :
" --allow-userns Allow further use of user namespaces inside sandbox\n",
opt_compat_level == 0 ?
" --new-session Create a new terminal session\n" :
" --no-new-session Don't create a new terminal session\n");
exit (ecode);
}

Expand Down Expand Up @@ -1652,18 +1663,43 @@ parse_args_recurse (int *argcp,
if (*total_parsed_argc_p > MAX_ARGS)
die ("Exceeded maximum number of arguments %u", MAX_ARGS);

bool print_help = FALSE;
while (argc > 0)
{
const char *arg = argv[0];

if (strcmp (arg, "--help") == 0)
{
usage (EXIT_SUCCESS, stdout);
/* Defer printing help as it now varies depending on the compat level. */
print_help = TRUE;
}
else if (strcmp (arg, "--version") == 0)
{
print_version_and_exit ();
}
else if (strcmp (arg, "--compat") == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because --compat changes the meaning of subsequent arguments, we should probably enforce that it must be the very first command-line argument? More like this, perhaps:

bwrap [--compat 0|1] [OPTIONS...] COMMAND [ARGS...]

The easiest way to check this is probably to check total_parsed_argc_p.

{
int the_compat_level;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
int the_compat_level;
long the_compat_level;

strtol returns a long.

char *endptr;

if (argc < 2)
die ("--compat takes an argument");

the_compat_level = strtol (argv[1], &endptr, 10);
if (argv[1][0] == 0 || endptr[0] != 0)
die ("Invalid compat level: %s", argv[1]);

if (the_compat_level > LATEST_COMPAT_LEVEL)
die ("Compat level %d is not suported by this version (latest supported is %d)", the_compat_level, LATEST_COMPAT_LEVEL);

if (the_compat_level < 0)
the_compat_level = LATEST_COMPAT_LEVEL;
Comment on lines +1695 to +1696
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, please remove that. Instead, specifying a negative compat level should be an error.


opt_compat_level = the_compat_level;

argv += 1;
argc -= 1;
}
else if (strcmp (arg, "--args") == 0)
{
int the_fd;
Expand Down Expand Up @@ -1789,13 +1825,20 @@ parse_args_recurse (int *argcp,
argv++;
argc--;
}
else if (strcmp (arg, "--disable-userns") == 0)
else if (opt_compat_level == 0 && strcmp (arg, "--disable-userns") == 0)
Comment on lines -1792 to +1828
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think --disable-userns should always be allowed, even in compat level >= 1: it just shouldn't have any practical result, because it's the default.

{
opt_disable_userns = TRUE;
opt_disable_userns_set = TRUE;
}
else if (strcmp (arg, "--assert-userns-disabled") == 0)
{
opt_assert_userns_disabled = TRUE;
opt_disable_userns_set = TRUE;
}
else if (opt_compat_level > 0 && strcmp (arg, "--allow-userns") == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly if we're going to add --allow-userns it should always be allowed, even in compat level 0: it just wouldn't have any practical effect in compat level 0, because it was the default.

{
opt_disable_userns = FALSE;
opt_disable_userns_set = TRUE;
}
else if (strcmp (arg, "--remount-ro") == 0)
{
Expand Down Expand Up @@ -1980,7 +2023,10 @@ parse_args_recurse (int *argcp,
if (next_perms >= 0)
op->perms = next_perms;
else
op->perms = 0666;
if (opt_compat_level == 0)
op->perms = 0666;
else
op->perms = 0644;
Comment on lines +2026 to +2029
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a matter of coding style I'd prefer to write this as

Suggested change
if (opt_compat_level == 0)
op->perms = 0666;
else
op->perms = 0644;
if (opt_compat_level >= 1)
op->perms = 0644;
else
op->perms = 0666;

to get contributors into the habit of always writing if (opt_compat_level >= n) for some n (which will be important when we get to compat level 2).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would 0600 be a better new default for --file than 0644, in the spirit of "failing safe" if the file might contain secrets? That would make it consistent with --[ro-]bind-data in compat level >= 1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you do that, then the documentation for --[ro-]bind-data should change to something like "note that this is not the same as --file in compatibility levels older than 1".


next_perms = -1;
argv += 2;
Expand Down Expand Up @@ -2176,7 +2222,12 @@ parse_args_recurse (int *argcp,
die ("--seccomp cannot be combined with --add-seccomp-fd");

if (opt_seccomp_fd != -1)
warn_only_last_option ("--seccomp");
{
if (opt_compat_level == 0)
warn_only_last_option ("--seccomp");
else
die ("More than one --seccomp options specified, use --add-seccomp-fd instead.");
Comment on lines +2226 to +2229
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly

Suggested change
if (opt_compat_level == 0)
warn_only_last_option ("--seccomp");
else
die ("More than one --seccomp options specified, use --add-seccomp-fd instead.");
if (opt_compat_level >= 1)
die ("More than one --seccomp option specified, use --add-seccomp-fd instead.");
else
warn_only_last_option ("--seccomp");

}

the_fd = strtol (argv[1], &endptr, 10);
if (argv[1][0] == 0 || endptr[0] != 0 || the_fd < 0)
Expand Down Expand Up @@ -2349,9 +2400,15 @@ parse_args_recurse (int *argcp,
argv += 1;
argc -= 1;
}
else if (strcmp (arg, "--new-session") == 0)
else if (opt_compat_level == 0 && strcmp (arg, "--new-session") == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to above, --new-session should always be allowed, even if it's the default.

{
opt_new_session = TRUE;
opt_new_session_set = TRUE;
}
else if (opt_compat_level > 0 && strcmp (arg, "--no-new-session") == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to above, --no-new-session should always be allowed, even if it's the default for the compat level.

{
opt_new_session = FALSE;
opt_new_session_set = TRUE;
}
else if (strcmp (arg, "--die-with-parent") == 0)
{
Expand Down Expand Up @@ -2526,6 +2583,15 @@ parse_args_recurse (int *argcp,
argc--;
}

if (print_help)
usage (EXIT_SUCCESS, stdout);

if (opt_compat_level > 0 && opt_unshare_user && !opt_disable_userns_set)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer compatibility level comparisons to always be written with >= or <, so in this case:

Suggested change
if (opt_compat_level > 0 && opt_unshare_user && !opt_disable_userns_set)
if (opt_compat_level >= 1 && opt_unshare_user && !opt_disable_userns_set)

Copy link
Collaborator

Choose a reason for hiding this comment

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

--disable-userns is documented as not working when bubblewrap is setuid root. Does this mean that bwrap --compat 1 ... is just not going to work for a setuid bubblewrap, unless you explicitly --allow-userns? That seems a bit odd.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even in compat level 1, --disable-userns likely shouldn't be the default if --assert-userns-disabled was given, because they're mutually-exclusive.

The main use-case for --assert-userns-disabled is that we're attaching a Flatpak sub-sandbox to a namespace that was already created, which if I remember correctly means that we can't disable creation of further user namespaces - we no longer have the necessary capabilities to do so. However, what we can do is to check that the original creator of the namespace did the equivalent of bwrap --disable-userns.

opt_disable_userns = TRUE;

if (opt_compat_level > 0 && !opt_new_session_set)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly here

Suggested change
if (opt_compat_level > 0 && !opt_new_session_set)
if (opt_compat_level >= 1 && !opt_new_session_set)

opt_new_session = TRUE;

*argcp = argc;
*argvp = argv;
}
Expand Down
31 changes: 31 additions & 0 deletions bwrap.xml
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

The documentation for --file should now say that the default permissions are 0644 in compatibility level 1 or later, or 0666 in older compatibility levels.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Or 0600 if we change the default to that instead, as I suggested in another thread)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to see a new section "Compatibility levels" in bwrap.xml, perhaps just before "Options", perhaps with text something like this:

Compatibility levels

bubblewrap has the concept of a compatibility level, similar to those used in debhelper(7). The compatibility level changes bubblewrap's behaviour and defaults in ways that have an impact on compatibility, and therefore cannot be done universally without breaking applications. It can be set by passing --compat LEVEL as the first command-line option. If not specified, the default compatibility level is 0.

Compatibility level 1

This compatibility level is not yet stable and its full effect is subject to change.

  • The --file operation defaults to creating a file with permissions 0644, instead of the 0666 that was historically the default. This can be overridden with --perms.
  • By default the sandbox will create a new terminal session, to avoid sandboxed processes being able to inject input into the controlling terminal (if any) with the TIOCSTI ioctl. This can be requested in older compat levels with --new-session, or reverted with --no-new-session.
  • If a new user namespace is being created, and none of the --allow-userns, --disable-userns or --assert-userns-disabled options are used, the default is to behave as though --disable-userns had been given. This can be requested in older compat levels with --disable-userns, or reverted with --allow-userns.

Compatibility level 0

This is the default compatibility level if no --compat option is specified. With this compatibility level, bubblewrap has its historical (backwards-compatible) behaviour.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also mention --seccomp here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The documentation for --seccomp should say something like:

In compatibility level 1 or later, this option can only be used once.
In older compatibility levels, if this option is given more than once, only the last one is used: this should be considered deprecated.
Use --add-seccomp-fd if multiple seccomp programs are needed.

Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@
<term><option>--version</option></term>
<listitem><para>Print version</para></listitem>
</varlistentry>
<varlistentry>
<term><option>--compat <arg choice="plain">compatability level</arg></option></term>
<listitem><para>Set compatability level (negative value means latest)</para></listitem>
Comment on lines +88 to +89
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<term><option>--compat <arg choice="plain">compatability level</arg></option></term>
<listitem><para>Set compatability level (negative value means latest)</para></listitem>
<term><option>--compat <arg choice="plain">LEVEL</arg></option></term>
<listitem><para>Set compatibility level to LEVEL. If used, this must be the first option. It changes the defaults and behaviour for subsequent options. The default compatibility level is 0.</para></listitem>

</varlistentry>
<varlistentry>
<term><option>--args <arg choice="plain">FD</arg></option></term>
<listitem><para>
Expand Down Expand Up @@ -145,6 +149,15 @@
<listitem><para>After setting up the new namespace, switch into the specified namespace. For this to work the specified namespace must be a descendant of the user namespace used for the setup, so this is only useful in combination with --userns.</para>
<para>This is useful because sometimes bubblewrap itself creates nested user namespaces (to work around some kernel issues) and --userns2 can be used to enter these.</para></listitem>
</varlistentry>
<varlistentry>
<term><option>--allow-userns</option></term>
<listitem><para>
Allow the process in the sandbox to create further user namespaces,
so that it can rearrange the filesystem namespace or do other more
complex namespace modification.
This option is only available in compatability level 1 or later.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This option is only available in compatability level 1 or later.
This is the default in compatibility levels older than 1.

</para></listitem>
</varlistentry>
<varlistentry>
<term><option>--disable-userns</option></term>
<listitem><para>
Expand All @@ -157,6 +170,7 @@
in the outer namespace.
This option requires <option>--unshare-user</option>, and doesn't work
in the setuid version of bubblewrap.
This option is not available in compatability level 1 or later.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This option is not available in compatability level 1 or later.
This is the default if using `--unshare-user` in compatibility level 1 or later.

</para></listitem>
</varlistentry>
<varlistentry>
Expand Down Expand Up @@ -455,12 +469,29 @@
ignore members and objects that they do not understand.
</para></listitem>
</varlistentry>
<varlistentry>
<term><option>--no-new-session</option></term>
<listitem><para>
Don't create a new terminal session for the sandbox (don't call
setsid()). This doesn't disconnect the sandbox from the controlling
terminal which means the sandbox can for instance inject input into
the terminal. This option is only available in compatability level 1
or later.
Comment on lines +478 to +479
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
the terminal. This option is only available in compatability level 1
or later.
the terminal. This is the default before compatibility level 1.

</para><para>
Note: In a general sandbox, if you use --no-new-session, it is
recommended to use seccomp to disallow the TIOCSTI ioctl, otherwise
the application can feed keyboard input to the terminal
which can e.g. lead to out-of-sandbox command execution
(see CVE-2017-5226).
</para></listitem>
</varlistentry>
<varlistentry>
<term><option>--new-session</option></term>
<listitem><para>
Create a new terminal session for the sandbox (calls setsid()). This
disconnects the sandbox from the controlling terminal which means
the sandbox can't for instance inject input into the terminal.
This option is not available in compatability level 1 or later.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This option is not available in compatability level 1 or later.
This is the default in compatibility level 1 or later.

</para><para>
Note: In a general sandbox, if you don't use --new-session, it is
recommended to use seccomp to disallow the TIOCSTI ioctl, otherwise
Expand Down
3 changes: 3 additions & 0 deletions completions/bash/bwrap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@ _bwrap() {

# Please keep sorted in LC_ALL=C order
local boolean_options="
--allow-userns
--as-pid-1
--assert-userns-disabled
--clearenv
--disable-userns
--help
--new-session
--no-new-session
--unshare-all
--unshare-cgroup
--unshare-cgroup-try
Expand All @@ -39,6 +41,7 @@ _bwrap() {
--cap-drop
--chdir
--chmod
--compat
--dev
--dev-bind
--die-with-parent
Expand Down
3 changes: 3 additions & 0 deletions completions/zsh/_bwrap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ _bwrap_args=(

# Please sort alphabetically (in LC_ALL=C order) by option name
'--add-seccomp-fd[Load and use seccomp rules from FD]: :_guard "[0-9]#" "file descriptor to read seccomp rules from"'
'--allow-userns[Allow further use of user namespaces inside sandbox]'
'--assert-userns-disabled[Fail unless further use of user namespace inside sandbox is disabled]'
'--args[Parse NUL-separated args from FD]: :_guard "[0-9]#" "file descriptor with NUL-separated arguments"'
'--as-pid-1[Do not install a reaper process with PID=1]'
Expand All @@ -38,6 +39,7 @@ _bwrap_args=(
'--chdir[Change directory to DIR]:working directory for sandbox: _files -/'
'--chmod[Set permissions]: :_guard "[0-7]#" "permissions in octal":path to set permissions:_files'
'--clearenv[Unset all environment variables]'
'--compat[Set compatability level (negative value means latest)]:compatability level:'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably this should use the same syntax as file descriptors:

Suggested change
'--compat[Set compatability level (negative value means latest)]:compatability level:'
'--compat[Set compatibility level]: :_guard "[0-9]#" "level"'

'--dev-bind-try[Equal to --dev-bind but ignores non-existent SRC]:source:_files:destination:_files'
'--dev-bind[Bind mount the host path SRC on DEST, allowing device access]:source:_files:destination:_files'
'--dev[Mount new dev on DEST]:mount point for /dev:_files -/'
Expand All @@ -53,6 +55,7 @@ _bwrap_args=(
'--lock-file[Take a lock on DEST while sandbox is running]:lock file:_files'
'--mqueue[Mount new mqueue on DEST]:mount point for mqueue:_files -/'
'--new-session[Create a new terminal session]'
"--no-new-session[Don't 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)]: :'
'--proc[Mount new procfs on DEST]:mount point for procfs:_files -/'
Expand Down