-
Notifications
You must be signed in to change notification settings - Fork 272
Add an option to use private (instead of slave) propagation for bind mounts. #522
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Азалия Смарагдова <charming.flurry@yandex.ru>
0b680e8 to
3885143
Compare
rusty-snake
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manpage and shell completion missing.
The manpage should also explain consequences like busy removable media.
…iles. Signed-off-by: Азалия Смарагдова <charming.flurry@yandex.ru>
Done. |
It can also make sandbox security worse, by having files that you thought you had unmounted (outside the sandbox) remain accessible inside the sandbox, or by having files that you thought you had hidden (by mounting something else over them) remain visible inside the sandbox. |
smcv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is your use-case for wanting this feature?
| int max_id; | ||
| unsigned int n_lines; | ||
| int root; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't adjust whitespace in locations that you're not otherwise editing.
This blank line is intentional: bubblewrap is mostly written in a C89 style where variable declarations must come before statements, and when writing in that style it's common to have an empty line between the last variable declaration and the first statement.
|
|
||
| bind_mount_result | ||
| bind_mount (int proc_fd, | ||
| int p_priv, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should be a BIND_PRIVATE flag in bind_option_t options. It's a lot clearer what the flag means if you see
bind_mount (..., BIND_PRIVATE);
rather than
bind_mount (..., 1, ...); /* what does 1 mean? is it a boolean? is it a length? ... */
or
bind_mount (..., TRUE, ...); /* what is being set to true here? */
| " --symlink SRC DEST Create symlink at DEST with target SRC\n" | ||
| " --seccomp FD Load and use seccomp rules from FD (not repeatable)\n" | ||
| " --add-seccomp-fd FD Load and use seccomp rules from FD (repeatable)\n" | ||
| " --private Set mount propagation to private\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if --private is halfway through the list of arguments? It's not at all obvious whether it affects all mounts (I think this is actually what happens) or whether it only affects mounts that appear after --private.
|
I don't think we can allow this in situations where bwrap is privileged (setuid root), because it would give users the ability to do something that the sysadmin thought they had prevented:
|
Hello everyone! I propose using a private propagation instead of slave one for bind mounts (this is optional and done by --private option). It can theoretically enhance sandbox security as bind-mounting protected paths inside accessible paths won't expose anything to the sandbox.