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

Create a new terminal session by default #571

Closed
wants to merge 1 commit into from
Closed

Create a new terminal session by default #571

wants to merge 1 commit into from

Conversation

kulkom
Copy link

@kulkom kulkom commented Apr 11, 2023

This pull request will make it so that Bubblewrap will create a new terminal session by default and will allow users to opt out of this with a new parameter "--no-new-session" (parameter "--new-session" will be deprecated), instead of having to opt in with "--new-session". This will, in my opinion, make Bubblewrap more secure for new users who aren't completely familiar with it. (Fixes #555)

…of this

with a new parameter "--no-new-session" (parameter "--new-session" will be
deprecated), instead of having to opt in with "--new-session". This will,
in my opinion, make `bwrap` more secure for new users who aren't completely
familiar with it.

Signed-off-by: Mikhail Kulko <[email protected]>
@kulkom
Copy link
Author

kulkom commented Apr 11, 2023

P.S. I understand that this change would break sandboxes that rely on not having a new terminal session, however:

  • I think there are a lot more cases where someone would want a new terminal session as opposed to where someone would not
  • Changing this back would be a simple find-and-replace (for example, in an sh script it would be as easy as sed -i 's/bwrap/bwrap --no-new-session/g' whatever.sh)
  • I'm not sure if Bubblewrap follows SemVer, however if it does then SemVer explicitly allows for anything to change at any time in versions 0.*.*

Alternatively, I would propose officially deprecating --new-session in the next release (0.8.1 or 0.9.0) and then merging this PR in the next release after that.

Ultimately, I think that merging this PR would make Bubblewrap more secure, particularly for new users.
Thank you for consideration.

@smcv
Copy link
Collaborator

smcv commented Apr 11, 2023

No, this is an incompatible change, and in particular would break command-line use of Flatpak. I know because doing this by default is exactly what I proposed in early 2017 (#143), and it caused regressions and was mostly reverted, adding --new-session instead (#154).

I would personally be in favour of adding a --no-new-session that doesn't do anything except for cancelling an earlier --new-session, allowing a caller to be explicit about not wanting a new terminal session (the same way that --share-net cancelling an earlier --unshare-net or the network part of --unshare-all), although I seem to remember that not all of the bubblewrap maintainers are in favour of adding options in --foo/--no-foo pairs.

If we had --no-new-session, then we could eventually start to show a warning when running bubblewrap without explicitly specifying either --new-session or --no-new-session; and after that warning had been shown for long enough and all significant callers had been updated, then we could consider flipping the default.

parameter "--new-session" will be deprecated

No, we should not do this. If you want "most" users of bwrap to be creating a new terminal session, then deprecating the option that they use to achieve that in a backwards-compatible way is exactly the wrong thing to do.

Copy link
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

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

Marking this as "changes requested" to ensure this isn't merged by mistake, but in this case it's less like "changes requested" and more like "please open a different PR instead".

@smcv
Copy link
Collaborator

smcv commented Apr 11, 2023

I'm fairly sure that the long-term answer to the various TIOCSTI attacks (and also TIOCLINUX which has some similar modes) is to disable it at kernel level, which if I understand correctly is already the default in some distributions like Fedora's development branch.

@kulkom
Copy link
Author

kulkom commented Apr 11, 2023

@smcv My apologies. Will open a different pull request that implements only --no-new-session. Thank you.

@kulkom kulkom closed this Apr 11, 2023
@rusty-snake
Copy link
Contributor

already the default in some distributions like Fedora's development branch.

Fedora disabled TIOCSTI by default even in current stable Fedora 37 (with Linux 6.2 and above). 🎉

@kulkom
Copy link
Author

kulkom commented Apr 11, 2023

Superseded by #572.

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.

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