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

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

Open
hartwork opened this issue Feb 28, 2023 · 11 comments · Fixed by #564
Open

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

hartwork opened this issue Feb 28, 2023 · 11 comments · Fixed by #564

Comments

@hartwork
Copy link
Contributor

Hi!

Thanks for making bubblewrap and sharing it as Software Libre! 🙏

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
Copy link
Collaborator

smcv commented Feb 28, 2023

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
Copy link
Contributor Author

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
Copy link
Collaborator

smcv commented Mar 2, 2023

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
Copy link
Collaborator

smcv commented Mar 2, 2023

I've published GHSA-7gfv-rvfx-h87x to clarify the handling of CVE-2017-5226.

smcv added a commit to smcv/bubblewrap that referenced this issue Mar 2, 2023
…model

bubblewrap can provide a robust security boundary that severely limits
functionality, or it can provide full functionality without any attempt
at being a security boundary, or anything in between those extremes.
If a caller of bubblewrap chooses inappropriate command-line arguments
for their desired security model, then bubblewrap will not provide the
security model they are aiming for, but this is not a bubblewrap
vulnerability.

Apparently this isn't clear to everyone, so try to clarify.

The one place where bubblewrap *does* define some sort of security
policy for itself is when it's setuid root, in which case it's
responsible for preventing users from carrying out privilege escalation
attacks like CVE-2020-5291.

Resolves: containers#555
Signed-off-by: Simon McVittie <[email protected]>
@hartwork
Copy link
Contributor Author

hartwork commented Mar 4, 2023

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
Copy link
Collaborator

smcv commented Mar 5, 2023

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
Copy link
Contributor Author

hartwork commented Mar 6, 2023

@smcv it's a tough one!

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

The key ideas are:

  • Communicate that it's easy to misuse, and be loud about it.
  • "Unprivileged" communicates "use me!" while "Low-level" communicates "advanced user territory!".
  • "sandboxing tool" sounds cosy while "process sandbox" sounds more technical (which affects messaging about the target audience).

What do you think?

kaniini added a commit to chainguard-dev/melange that referenced this issue Mar 14, 2023
…5226

Without it, it is possible to escape the sandbox via TIOCSTI ioctls on the session
PTY.

Related: containers/bubblewrap#555
Related: containers/bubblewrap#142
Related: https://news.ycombinator.com/item?id=30825088
Signed-off-by: Ariadne Conill <[email protected]>
algitbot pushed a commit to alpinelinux/abuild that referenced this issue Mar 16, 2023
…-5226)

Bubblewrap has an under-documented option which helps to protect against abuse
of TIOCSTI ioctls against the session PTY to escape the build sandbox, the
--new-session option.

Related: containers/bubblewrap#555
Related: containers/bubblewrap#142
Related: https://news.ycombinator.com/item?id=30825088
Signed-off-by: Ariadne Conill <[email protected]>
alexlarsson pushed a commit that referenced this issue Mar 30, 2023
…model

bubblewrap can provide a robust security boundary that severely limits
functionality, or it can provide full functionality without any attempt
at being a security boundary, or anything in between those extremes.
If a caller of bubblewrap chooses inappropriate command-line arguments
for their desired security model, then bubblewrap will not provide the
security model they are aiming for, but this is not a bubblewrap
vulnerability.

Apparently this isn't clear to everyone, so try to clarify.

The one place where bubblewrap *does* define some sort of security
policy for itself is when it's setuid root, in which case it's
responsible for preventing users from carrying out privilege escalation
attacks like CVE-2020-5291.

Resolves: #555
Signed-off-by: Simon McVittie <[email protected]>
@hartwork
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Contributor

--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
Copy link
Contributor Author

hartwork commented Mar 30, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants