Implement --compat #572

kulkom · 2023-04-11T17:08:06Z

I hope everyone likes this pull request better than the last one.

meson.build

bubblewrap.c

bwrap.xml

completions/zsh/_bwrap

meson.build

meson_options.txt

configure.ac

smcv

Please rebase these changes and squash them into some smaller number of commits, so that each commit is individually a correct change. Adding --no-new-session is probably fine to do in a single commit.

As a general comment, I think you are more likely to get --no-new-session merged if you change this PR so that it only adds --no-new-session, with other changes moved to a separate PR.

If you are making changes that have dependencies between them, it's OK to have a series of commits like for example:

add infrastructure that will let us do foo more easily
factor out existing code that will become repetitive when we do foo (or code that is already repetitive)
do foo
add tests for foo

However, if the changes you're making don't have any dependencies in either direction, that's probably a sign that they should be separate PRs. I don't see any obvious reason why adding --no-new-session would require expanding the summary in the build system, or why expanding the summary in the build system would require adding --no-new-session, which probably means these are really two unrelated changes; that means they should be separate commits, and preferably separate PRs, so that each one doesn't block the other.

bubblewrap.c

configure.ac

meson.build

kulkom · 2023-05-24T17:08:41Z

Also, I created this PR (and the previous terrible one) because I wanted bubblewrap to be more secure, especially after I found out that anyone can escape from my container because I didn't specify --new-session. However, now I understand that in that case the problem was not bubblewrap itself, but how I used it. So now, I think that it would be better for me to instead make a PR implementing an option with a name like --default-secure, which would flip most, if not all, of the defaults. (for example, with this option the parameter --unshare-all would become the default and one would need to specify --share-all to disable it, and --bind would have the behaviour of --ro-bind and one would have to specify --rw-bind to bind something with write permissions). I think that such a feature would be significantly better than adding --no-new-session and flipping the default would ever be.

smcv · 2023-05-30T10:49:37Z

an option with a name like --default-secure

As I think I've explained several times already, it is not possible to implement such an option without making it deeply misleading, because this situation will inevitably happen:

the default behaviour of clone() in the Linux kernel is to share all state between parent and child, except for state that we explicitly unshare (this is a consequence of the kernel's strong backwards-compatibility rules)
you implement bwrap --default-secure which unshares all known namespaces
the kernel invents a new namespace, CLONE_NEWFOO, which unshares the new Foo namespace between parent and child
for maximum security (minimum sharing) we would ideally want --default-secure to unshare Foo too
but we now have two choices, both of them bad:
- make --default-secure leave Foo shared, meaning it is less secure than it could be, and now we have to invent a --default-more-secure;
- or make --default-secure unshare Foo too, which means its behaviour has incompatibly changed, and applications that relied on Foo being shared with parent processes will stop working

The way to do a similar thing without either adding a misleading option or randomly breaking apps is #455.

Signed-off-by: Mikhail Kulko <mkulko@mkulko.me>

kulkom · 2023-06-17T21:59:17Z

I have implemented --compat. Currently with this PR --compat [anything but 0] will do these things:

use 0644 as the default permissions instead of 0666 for --file
die when more than one --seccomp option is specified
remove --disable-userns and make it's behaviour the default, create a new option --allow-userns
remove --new-session and make it's behaviour the default, create a new option --no-new-session

I'm not sure about my decision to remove --new-session and --disable-userns from compatability level 1 and to implement --no-new-session and --allow-userns only in compatability level 1 or later, so if that's a bad idea I will fix it. Also, I would still like to implement something like --default-secure, but now I'm not sure whether that would be a good idea.

Signed-off-by: Mikhail Kulko <mkulko@mkulko.me>

smcv

Thanks for working on this.

I think if we want to get this merged as soon as possible, with as little doubt as possible about the semantics, it might be a good idea to drop the effect on --allow-userns/--disable-userns from this PR and move it to a follow-up PR. That would get you the benefit of the bits that are uncontroversial - the --file default permissions, and --new-session by default - without being blocked by figuring out exactly how --allow-userns and friends should interact with compat levels.

As a matter of philosophy, I think we should always have one compat level that is documented as "subject to change", which we use as a staging area for new changes to the defaults. Initially, compat level 1 will be that. When we're happy with compat level 1, we can document it as stable, and document compat level 2 as subject to change (even if it is initially the same as compat level 1); and so on.

smcv · 2023-09-28T11:19:55Z

bubblewrap.c

@@ -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"


Suggested change

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

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

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.

smcv · 2023-09-28T11:23:04Z

bubblewrap.c

        }
      else if (strcmp (arg, "--version") == 0)
        {
          print_version_and_exit ();
        }
+      else if (strcmp (arg, "--compat") == 0)
+        {
+          int the_compat_level;


Suggested change

int the_compat_level;

long the_compat_level;

strtol returns a long.

smcv · 2023-09-28T11:23:54Z

bubblewrap.c

+          if (the_compat_level < 0)
+            the_compat_level = LATEST_COMPAT_LEVEL;


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

smcv · 2023-09-28T11:26:21Z

bubblewrap.c

-      else if (strcmp (arg, "--disable-userns") == 0)
+      else if (opt_compat_level == 0 && strcmp (arg, "--disable-userns") == 0)


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.

smcv · 2023-09-28T11:29:13Z

bubblewrap.c

        }
      else if (strcmp (arg, "--version") == 0)
        {
          print_version_and_exit ();
        }
+      else if (strcmp (arg, "--compat") == 0)


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.

smcv · 2023-09-28T11:43:24Z

completions/zsh/_bwrap

@@ -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:'


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"'

smcv · 2023-09-28T11:44:30Z

bubblewrap.c

-           "    --disable-userns             Disable further use of user namespaces inside sandbox\n"
+           "%s" /* --(disable/allow)-userns */


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"

smcv · 2023-09-28T11:50:40Z

bubblewrap.c

+  if (print_help)
+    usage (EXIT_SUCCESS, stdout);
+
+  if (opt_compat_level > 0 && opt_unshare_user && !opt_disable_userns_set)


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.

smcv · 2023-09-28T11:52:41Z

bwrap.xml

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.

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

smcv · 2023-09-28T12:05:44Z

bwrap.xml

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.

Also mention --seccomp here.

smcv

Should --compat 1 make it an error for any of the non-repeatable options that call warn_only_last_option() to be repeated? That would affect: --chdir, --exec-label, --file-label, --block-fd, --sync-fd, --userns-block-fd, --info-fd, --json-status-fd, --userns, --userns2, --pidns, --uid, --gid, --hostname.

I think probably it should. The easiest way to achieve that would be to change warn_only_last_option() into something like this:

static void
warn_only_last_option (const char *name)
{
  if (compat_level >= 1)
    die ("Repeating the %s option is not allowed in compatibility level >= 1", name);
  else
    warn ("Only the last %s option will take effect", name);
}

smcv · 2023-09-28T12:13:06Z

bwrap.xml

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.

smcv · 2023-09-28T12:13:40Z

bwrap.xml

Also mention --seccomp here.

smcv · 2023-09-28T12:19:49Z

bubblewrap.c

+            if (opt_compat_level == 0)
+              op->perms = 0666;
+            else
+              op->perms = 0644;


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.

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".

smcv · 2023-09-28T12:30:30Z

This is also missing any test coverage: tests/test-run.sh is using compat level 0 everywhere.

rusty-snake reviewed Apr 11, 2023

View reviewed changes

meson.build Outdated Show resolved Hide resolved

kulkom mentioned this pull request Apr 11, 2023

Create a new terminal session by default #571

Closed

smcv reviewed Apr 11, 2023

View reviewed changes

kulkom force-pushed the main branch from 7fff133 to 83bf842 Compare April 12, 2023 10:40

kulkom changed the title ~~Implement --no-new-session and an option to warn against relying on bubblewrap to set --no-new-session implicitly (disabled by default)~~ Implement --no-new-session and improve build files Apr 12, 2023

smcv requested changes May 4, 2023

View reviewed changes

bubblewrap.c Outdated Show resolved Hide resolved

bubblewrap.c Outdated Show resolved Hide resolved

configure.ac Outdated Show resolved Hide resolved

meson.build Outdated Show resolved Hide resolved

kulkom force-pushed the main branch from 1275699 to c774bca Compare May 24, 2023 10:59

kulkom changed the title ~~Implement --no-new-session and improve build files~~ Implement --no-new-session May 24, 2023

kulkom closed this Jun 17, 2023

kulkom force-pushed the main branch from 73023cb to ad76c2d Compare June 17, 2023 21:01

implement --compat and flip some defaults

58fb6d3

Signed-off-by: Mikhail Kulko <mkulko@mkulko.me>

kulkom reopened this Jun 17, 2023

kulkom changed the title ~~Implement --no-new-session~~ Implement --compat Jun 17, 2023

document changes in bwrap.xml

64ca7d1

Signed-off-by: Mikhail Kulko <mkulko@mkulko.me>

kulkom force-pushed the main branch from 0bcf8c6 to 64ca7d1 Compare June 17, 2023 22:01

smcv requested changes Sep 28, 2023

View reviewed changes

smcv reviewed Sep 28, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement --compat #572

Implement --compat #572

kulkom commented Apr 11, 2023

smcv left a comment

kulkom commented May 24, 2023

smcv commented May 30, 2023

kulkom commented Jun 17, 2023 •

edited

Loading

smcv left a comment

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv left a comment

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv Sep 28, 2023

smcv commented Sep 28, 2023

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

		if (the_compat_level < 0)
		the_compat_level = LATEST_COMPAT_LEVEL;

		else if (strcmp (arg, "--disable-userns") == 0)
		else if (opt_compat_level == 0 && strcmp (arg, "--disable-userns") == 0)

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

		" --disable-userns Disable further use of user namespaces inside sandbox\n"
		"%s" /* --(disable/allow)-userns */

Implement --compat #572

Are you sure you want to change the base?

Implement --compat #572

Conversation

kulkom commented Apr 11, 2023

smcv left a comment

Choose a reason for hiding this comment

kulkom commented May 24, 2023

smcv commented May 30, 2023

kulkom commented Jun 17, 2023 • edited Loading

smcv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Compatibility levels

Compatibility level 1

Compatibility level 0

Choose a reason for hiding this comment

smcv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smcv commented Sep 28, 2023

kulkom commented Jun 17, 2023 •

edited

Loading