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.

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

Improve readme with regard to --new-session security (related to #555) #560

Merged
merged 3 commits into from
Apr 3, 2023

Conversation

hartwork
Copy link
Contributor

@hartwork hartwork commented Mar 2, 2023

Hi!

This is the first piece aimed at improving on the #555 situation. I'm looking forward to your review 🍻

PS: The addition of the tip on brace expansion is unrelated. I can drop it or extract a new pull request for it as needed, as you like.

README.md Outdated
Comment on lines 120 to 122
`--new-session` is in there to project against out-of-sandbox
command execution through `TIOCSTI` commands
(see [CVE-2017-5226](https://github.com/containers/bubblewrap/issues/142)).
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 are aiming to make that example a complete, useful sandbox, then this is not enough: there are much larger sandbox holes in that simple command, like the fact that a process inside the sandbox can connect to an IPC socket (D-Bus, X11, that sort of thing) and ask processes outside the sandbox to take action on its behalf.

The command-line for a production-ready use of bubblewrap as a sandbox that is genuinely a security boundary is very long (and in practice it's very likely that it will be so long that you'll want to pass it in via --args). Look at the output of flatpak run -vv ... and you'll see what I mean!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smcv I was not aiming at making the example complete in terms of usability: the example still doesn't work out of the box (e.g. Bash is /bin/bash here which is not shared in the example) and the text still says "This is an incomplete example" explicitly. I believe we should consider though aiming for complete in terms of security. I should have a closer look at your pointer regarding flatpak (thanks!) before continuing this reply.

Copy link
Collaborator

Choose a reason for hiding this comment

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

like the fact that a process inside the sandbox can connect to an IPC socket (D-Bus, X11, that sort of thing)

On closer inspection, this command-line isn't sharing /run or /tmp or /, so maybe it prevents that sort of thing on typical systems? But I wouldn't want to claim that it's secure against any particular set of attacker capabilities.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For comparison, the argv output by flatpak run -vv org.gnome.Recipes are long enough that they don't fit on my screen with one argument per line, and the seccomp filter set up by Flatpak (which only represents 2 lines of those!) includes blocking things like access to the kernel keyring, creating a new user namespace inside the first one, tracing other processes, and accessing Bluetooth devices (unless the app has suitable permissions flags).

Strong sandboxing is not an easy thing to do.

Copy link
Contributor Author

@hartwork hartwork Mar 4, 2023

Choose a reason for hiding this comment

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

@smcv I had a chance at seeing that wall of a bubblewrap myself now. What I find interesting is how different the defaults are for the realms(?) that bubblewrap touches:

Realm Default
Mount stack opt-in (empty)
Session opt-out (shared)
Namespaces opt-out (shared)
Seccomp opt-out (unfiltered)
Hostname opt-out (shared)
Environment variables opt-out (shared)
POSIX Capabilities opt-out?

Of those, the mount stack is what users see first and are forced to tackle when they first start using the tool. Unless they go on a self-driven look out for all the other realms, it's easy to mis-assume that the rest of the tool will also be opt-in, when in reality most of the others are opt-out. For Seccomp, opt-in is probably not feasible, but for some or even most of the others, flipping the default around to opt-in should well be possible and get more users onto safe ground "for free".

Are there more realms like that in bubblewrap that I overlooked or forgot?

Maybe we can use a table like that to educate about the full list of realms that they'll need to make decisions for, and put the opt-out nature of most realms into the spotlight more. (The opt-in approach of the mount stack in bubblewrap was what made me go "nice!" first, in particular compared to --private of Firejail.)

PS: Since you mention "creating a new user namespace inside the first one" — the implications of allowing or denying that are not yet clear to me. Do you have any links in mind where I could learn more about the topic?

PPS: Which syscalls did you have in mind with "accessing Bluetooth devices"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smcv that is understood yes, so I guess these changes would need to be done combined into one big break in the future. The gain from mass-flipping to opt-in in security will be significant. If Seccomp was the only realm left to shoot yourself in the foot with, that would be a great improvement to baseline user security. Before we flip defaults I believe we can do two things in that direction:

  • we can introduce arguments that enforce current default behavior, e.g. --same-session and --keep-env, so that fools can start using these options to be safe from default flipping in the future (e.g. thinking of flatpak), and
  • we can put a table like the one above somewhere visible to raise awareness about the current de-facto bubblewrap user research todo list.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Much better approach IMHO is #455.

  • The problem with "current default arguments to be fail safe" is that a lot downstream project will only add them when they notice breakage - be it automated tests/qa or bug reports from users.

  • Default opt-out in sandboxing tools has the big problem of new realms unless you have combat levels (like targetSdk version in android). You can not add a new realm that is disallowed by default, when this kind of resource wasn't handle and therefore allowed before. Say for example we get sockets namespaces in the future and enable them by default, everything that uses sockets will fail now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Frankly, I'm not a fan of #455, I just haven't replied there yet.

Breaking compatibility is not something that should be done lightly. But if a switch from 99% opt-out to 99% opt-in is not worth breaking compatibility, then what is?

Personally, I'd consider renaming the binary to bwrap2, rename all other clashing files, no unexpected breaks, everyone starts out with both bwraps installed side by side, users can use the fixed version right away, every app using bubblewrap migrates from bwrap to bwrap2 and adjust dependencies, done. Nothing breaks, more work but about as trivial as it gets. Trivial is good for security 😃

Copy link
Collaborator

Choose a reason for hiding this comment

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

for some or even most of the others, flipping the default around to opt-in should well be possible and get more users onto safe ground "for free"

Sorry, that doesn't work. We have a strict policy of not doing impossible things.

I am at least 99% confident that the kernel is going to add more aspects of the execution environment that bubblewrap users might want to unshare. Let's assume that in 2024, the kernel will add a new Foobar namespace, CLONE_NEWFOOBAR. The default for that feature at the kernel level is going to be "shared with parent", because kernel developers take backwards compatibility very seriously, and the behaviour of clone() today is to inherit all aspects of the execution environment that are not specifically unshared.

Existing bubblewrap binaries and source releases have to accept the default for whether to unshare the Foobar namespace, because they have no alternative: they already exist, and simply do not contain the necessary code to flip that feature to "unshared". In the absence of a time machine, we cannot change the behaviour of those existing bubblewrap releases, however much we might want to, and neither can you.

If we followed the plan you are suggesting, by having a "bwrap2" where the controlling terminal and all namespaces that existed in February 2023 are unshared by default, then by 2024, bwrap2 would be back to having behaviour that you would describe as inconsistent: it would unshare the controlling terminal and the user, mount, pid, UTS, network and IPC namespaces, but it would leave the Foobar namespace unshared. Even if someone (maybe you) contributed an --unshare-foobar option, it would have to be an opt-in (default-allow), to avoid breaking existing users of bwrap or bwrap2. If we responded by adding a new bwrap3 entry point, with essentially the same code but different defaults, which distributions would have to package separately if they want it, then I hope you can see how poorly that could scale over time.

In some ways, we already have that problem: newer kernels already add cgroup and time namespaces which bwrap does not know how to unshare, and --unshare-all unshares all of the namespaces that bwrap implements, which is a subset of the list of namespaces that exist.

debhelper-style compat levels (#455) would mitigate this by letting us have a "ratchet" behaviour where callers can opt-in to newer defaults by setting a higher compat level, and each compat level would presumably have defaults closer to "unshare everything" than the previous compat level.

I completely agree that this stuff is difficult and non-obvious, but please don't assume that bubblewrap's maintainers haven't thought about it. (Or, if your assumption is that the maintainers of bubblewrap don't know what we're doing, then you probably shouldn't be trusting our codebase; there's nothing magic about bubblewrap, and you could implement something equivalent for yourself if you want to. Please don't use a name that would be interpreted as any sort of official successor, like bwrap2 or bwrap-ng: that would be misleading.)

The time that I have available for bubblewrap is limited, and I cannot afford to spend an unlimited amount of time on explaining this or arguing about it. So, the more you say "but why can't you just?", the more likely it is that I will have to abandon an issue or pull request to work on other topics, and hope that my co-maintainers (who are even more busy than I am) will eventually pick it up.

Frankly, I'm not a fan of #455, I just haven't replied there yet.

Your opinion is noted, but we cannot please everyone, and it's the maintainers of bubblewrap (who are responsible for keeping this stuff working indefinitely) whose opinion counts most here.

Copy link
Contributor Author

@hartwork hartwork Mar 6, 2023

Choose a reason for hiding this comment

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

@smcv I will keep my reply short to save your time:

  • I have no reason to "assum[e] that the maintainers of bubblewrap don't know what we're doing". You guys know more about sandboxes than me.
  • I want to help your boat, not make a second boat. I have zero intentions of forking bubblewrap. Command bwrap2 would be original upstream bubblewrap.
  • I have no problem with having command bwrap3 in 12 months if a new namespace comes out. It doesn't scale well, but it's worth it in my personal book. We'd probably break a lot less than e.g. clang 16.
  • Compat levels do not help with the core issue, which is flipping defaults.
  • A good example for compat levels gone wrong are CMake policies. It could be execution rather than concept though, in theory.
  • If I'm effectively causing a problem here, I will back down a bit or multiple bits so that you feel less pressure again.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@smcv
Copy link
Collaborator

smcv commented Mar 2, 2023

I think what I would be inclined to do is: in the Usage section, include --new-session in the example command-line, but without explanation; and in the Sandboxing section, explain why. That would avoid having this information twice.

Signed-off-by: Sebastian Pipping <sebastian@pipping.org>
Signed-off-by: Sebastian Pipping <sebastian@pipping.org>
Signed-off-by: Sebastian Pipping <sebastian@pipping.org>
@hartwork
Copy link
Contributor Author

hartwork commented Mar 4, 2023

I think what I would be inclined to do is: in the Usage section, include --new-session in the example command-line, but without explanation; and in the Sandboxing section, explain why. That would avoid having this information twice.

@smcv having it twice will not win a price for purity, but repetition means higher chance to get noticed (and is likely an important tool in some approaches to education), it was a conscious decision. However, I have dropped it now as suggested. If you change your mind later, I can bring it back.

@alexlarsson alexlarsson merged commit 2f9ce90 into containers:main Apr 3, 2023
@hartwork hartwork deleted the improve-readme branch April 3, 2023 13:42
smcv added a commit to smcv/bubblewrap that referenced this pull request Feb 15, 2024
Signed-off-by: Simon Brand <simon.brand@postadigitale.de>
[smcv: Combine with containers#560, re-word]
Co-authored-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants