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

README, SECURITY: Clarify that bubblewrap does not define a security model #564

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

smcv
Copy link
Collaborator

@smcv smcv commented Mar 2, 2023

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


@hartwork: does this, perhaps combined with #560, provide the information that you thought was missing in a place where you would have found it?

…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]>
@brandsimon
Copy link

@smcv
I think it is also valuable information for users of this tool that

  • everything passed to the sandboxed can be used to break out of it. A informative example would be a dbus-socket, which could be used to execute something via systemd. This may be obvious for advanced users, but makes newcomers aware of possible mistakes.
  • a sandboxed application could be less able to enforce a security boundary within itself (e.g. webbrowsers).

Comment on lines +54 to +55
bubblewrap is not a complete, ready-made sandbox with a specific security
policy.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good, but somewhere in the middle of the readme, with nothing pointing here, not bold, not all-uppercase etc. It will be easier to miss than notice.

the host system is entirely determined by the arguments passed to
bubblewrap.

Whatever program constructs the command-line arguments for bubblewrap
Copy link
Contributor

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

This assumes that it's a program rather than a human. My vote for either changing this to include the human case or to clearly state that bwrap is not supposed to be executed directly by a human, if that's the case.

Comment on lines +7 to +9
If bubblewrap is setuid root, then the goal is that it does not allow
a malicious local user to do anything that would not have been possible
on a kernel that allows unprivileged users to create new user namespaces.
Copy link
Contributor

Choose a reason for hiding this comment

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

The cognitive load from all the negations is higher here than probably necessary. Could this be rephrased or simplified for graspability some way?

@hartwork
Copy link
Contributor

hartwork commented Mar 4, 2023

@hartwork: does this, perhaps combined with #560, provide the information that you thought was missing in a place where you would have found it?

@smcv before I reply let me emphasize that I have no grief about my own reading the bubblewrap project wrong in some sense initially. I'm okay with that past, I'm happy about the exchange, my concern is the users after me. With them in mind, I like this direction but feel it may be still way to easy to overlook/miss.

PS: I keep extending the threat model documentation of sandwine. If anyone coming here feels like reviewing that documentation too (not here though to stay on topic), please feel invited. I hope that's okay to say.

@hartwork
Copy link
Contributor

hartwork commented Mar 4, 2023

@smcv PS: this is the kind out hard(er)-to-overlook loudness that I have in mind, just to give an idea:

threat_model_Screenshot_20230304_061418

@hartwork
Copy link
Contributor

hartwork commented Mar 5, 2023

PS: I just found that what @brandsimon proposed above has near-100% overlap with pull request #370.

@smcv
Copy link
Collaborator Author

smcv commented Mar 6, 2023

everything passed to the sandboxed can be used to break out of it. A informative example would be a dbus-socket, which could be used to execute something via systemd. This may be obvious for advanced users, but makes newcomers aware of possible mistakes.

I'm sorry, but I cannot spend an unlimited amount of time on giving newcomers hints about this project, or on discussing the best possible wording for a change. It's helpful that you are pointing out that things are not obvious, but I cannot afford to spend all my time on making them more obvious - particularly if we end up making so much noise about one important point that it ends up hiding a different important point.

I'm doing my best to keep this project alive and secure while its authors are busy with other things, but every hour I spend discussing its security model is an hour I am not spending on bug fixes or feature enhancements in this project or in the many other projects I'm responsible for.

If my best is not good enough, then I'm not going to move forward with this or related merge requests until I get either an opinion or an alternative from my co-maintainers.

a sandboxed application could be less able to enforce a security boundary within itself (e.g. webbrowsers)

That's #370 and I think we should treat it as out of scope here.

@hartwork
Copy link
Contributor

hartwork commented Mar 6, 2023

@smcv I will back down for now then. I hope we can collaborate more again later. Thanks for your work on bubblewrap 🙏

@alexlarsson
Copy link
Collaborator

This sounds good to me, and hopefully it can limit some of the weird ideas about what bwrap is and is not. (Its really just a tool to make it easy to use some kernel features, with the setuid fallback for when user namespaces are not supported.)

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?
4 participants