containers / bubblewrap

Low-level unprivileged sandboxing tool used by Flatpak and similar projects
Other
3.96k stars 237 forks source link

"--new-session" underadvertised and CVE-2017-5226 still a thing in 2023 by default? #555

Open hartwork opened 1 year ago

hartwork commented 1 year ago

Hi!

Thanks for making bubblewrap and sharing it as Software Libre! :pray:

Someone pointed out the importance of --new-session on Hacker News and I'm in debt to them for speaking up about it both personally and with sandwine. They go on saying:

[..] Bubblewrap is aware of this, yet their documentation gives no indication that this flag is necessary to produce a secure sandbox. In --help, the documentation of --new-session is simply "Create a new terminal session," which severely understates its importance. [..]

I checked the main readme for mentions of --new-session and found no matches and checked the help output and it doesn't mention security implecations. The man page has something but why check the man page if --help seemed to answer the questions you new you had. So there really was no place other than Hacker News educating me prior to first usage and I could not even heard of --new-session still, realistically. After seeing the CVE-2017-5226 demo from #142 (thanks!) work on my own terminal (scary!), including stealth mode with echo off, I agree that --new-session needs more user attention and/or become default. Maybe it needs a counter-part --same-session also so that --new-session can become the default at some point in the future if #150 is not being implemented — the latest reply there is off 2017. The warning idea from #162 — also of 2017 — will help with user education (which is good) but would come too late if bubblewrap is run behind the scenes rather than manually by the user.

Any chance for motion in that direction?

Thanks and best, Sebastian

smcv commented 1 year ago

bubblewrap is more of a tool for building sandboxes than a sandbox itself. All of its options map fairly directly to lower-level kernel features, and it's up to the authors of larger frameworks that use bubblewrap (like Flatpak) to provide a coherent security model using those features, which makes CVE-2017-5226 really more like a Flatpak vulnerability than a bubblewrap vulnerability.

Because bwrap needs to be setuid on some systems, and has historically been setuid, any new code that we add to it has to weigh up the benefits of that new code path against the additional security exposure of having that code in a setuid executable, which is a good reason to avoid compiling seccomp filters in bubblewrap itself, making #150 unappealing.

Also, because bubblewrap is a toolkit for building sandboxes more than a sandbox, it isn't in a great position to assess whether the options it is given are "safe" or "unsafe" (particularly tricky when some use-cases for it don't even intend to be a security boundary).

If you have concrete suggestions for things that can be done better, please open merge requests - but if those merge requests add significant code to the security-sensitive critical path, or if they break backwards compatibility, then we won't necessarily be able to merge them.

hartwork commented 1 year ago

Hi @smcv , thanks for your reply!

I understand that bubblewrap is useful to build sandboxes, but the visible marketing — in particular the GitHub repository main description "Unprivileged sandboxing tool" — are in some conflict with what you say about mostly setuid and not being an end user tool, not sure how subjective my reading is there. People building sandboxes on top of bubblewrap need a realistic chance to get it right and it was only luck that I heard about --new-session elsewhere for me. Maybe this stretch — being a standalone tool and not being a standalone tool at the same time — is the key thing that needs to be addresses somehow then? Maybe the future and the past are two different buckets on that front though.

Best, Sebastian

smcv commented 1 year ago

the visible marketing — in particular the GitHub repository main description "Unprivileged sandboxing tool"

It is an unprivileged sandboxing tool - that's accurate. That's not the same thing as being a complete, ready-to-use sandbox with its own coherent security model.

It is not my intention to be doing any marketing (and please see the license text for details of the extent to which there is no warranty).

Where would you have expected to find details of the care required when constructing bubblewrap command-lines?

Maybe this stretch — being a standalone tool and not being a standalone tool at the same time — is the key thing that needs to be addresses somehow then?

Honestly, I don't see bubblewrap as a standalone thing, more of an implementation detail of higher-level layers like Flatpak, libgnome-desktop and sandwine. It is possible to construct sandboxes with it, which might or might not comply with your security policy - we can't know whether they do or not, because we don't know your security policy (and the way you tell bubblewrap your security policy is by constructing its command-line arguments, hopefully carefully).

smcv commented 1 year ago

I've published https://github.com/flatpak/flatpak/security/advisories/GHSA-7gfv-rvfx-h87x to clarify the handling of CVE-2017-5226.

hartwork commented 1 year ago

Hi @smcv I had a chance to digest on this a bit more and to play with flatpak a bit to see its invocation of bubblewrap… On your two latest replies above:

Regarding marketing I should have been more precise, sorry. I understand that technically "Unprivileged sandboxing tool" is not wrong, but I would argue that's too easy to read as "end user sandboxing tool" while you seem to imply it's a lot closer to "plumbing-only sandboxing tool for senior sandbox developers only". Wouldn't it be great to resolve some of that ambiguity for more realistic user expectations? Btw if there is an important language level difference between sandbox and sandboxing then it may get lost in translation, I cannot tell the difference.

Regarding GHSA-7gfv-rvfx-h87x I think clarification is great but name CVE-2017-5226 is already hard-glued to bubblewrap to the Internet so I think this may create more confusion then less. Maybe just requesting a new CVE from Mitre with similar metadata would be better? Maybe I do not yet understand your idea fully.

smcv commented 1 year ago

I understand that technically "Unprivileged sandboxing tool" is not wrong, but I would argue that's too easy to read as "end user sandboxing tool" while you seem to imply it's a lot closer to "plumbing-only sandboxing tool for senior sandbox developers only".

Do you have any better/clearer suggestions?

hartwork commented 1 year ago

@smcv it's a tough one!

Maybe "Low-level process sandbox, USE WITH CARE"?

The key ideas are:

What do you think?

hartwork commented 1 year ago

@alexlarsson I would like to note that the issue is now marked as closed while merging #564 did nothing to improve on the --new-session being underadvertised situation: it doesn't mention --new-session anywhere. Regarding --new-session, there are two open pull requests #560 and #567. Please consider re-opening this ticket, thank you!

alexlarsson commented 1 year ago

I can open this and then we can close it when the manpage adds more verbiage, but its not a default we can change without breaking existing users.

WhyNotHugo commented 7 months ago

--new-session mostly works around security issues that happen as a result of TIOCSTI (which lets a terminal application inject keystrokes into the parent terminal).

Unless TIOCSTI is strictly needed, it should likely be disabled by setting LEGACY_TIOCSTI=n in your distribution's kernel configuration.

You can also disable it at runtime with sysctl. It seems to be disabled by default of Alpine, unsure about other distributions. You can check with sysctl:

> sysctl dev.tty.legacy_tiocsti
dev.tty.legacy_tiocsti = 0
hartwork commented 7 months ago

@WhyNotHugo a lot has happened in this space since I first created this ticket and it is clear by now that --new-session does not work well for interactive applications like htop. Personally I believe that the true fix is adding PTY support to bwrap but at the same time it's complex and hard to implement correctly. A sort-of snapshot of where I am with the TIOCSTI topic regarding related CVEs, options for implementations etc can be found in the readme of https://github.com/hartwork/antijack .