-
Notifications
You must be signed in to change notification settings - Fork 273
Support scoping abstract unix sockets #704
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
|
Flag name is a bit long, but I couldn't think of a shorter one; open to suggestions. |
|
Ping |
|
Manpage should be updated too. |
948bcbb to
4b8a69e
Compare
|
I noticed some completions in the repo, so I've updated those as well. |
bwrap.xml
Outdated
| <varlistentry> | ||
| <term><option>--scope-abstract-unix-sockets</option></term> | ||
| <listitem><para> | ||
| Scope access to abstract unix sockets to within in the sandbox. |
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.
tabs vs. spaces
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.
How should it be indented then? I noticed that indentation, at least in my editor, in the file seems inconsistent, so I copied the --as-pid-1 section.
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.
you're right it is inconsistent. I just noticed strange indention in GH diff and --cap options looked good.
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 mimic the indentation of the better / more consistent options when adding new options. --cap-add and --cap-drop are better examples to follow.
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.
The commit message should briefly describe why this is here. Presumably the main use-case is to be able to prevent sandboxed processes from accessing your Xorg/Xwayland server's listening socket, as on #330?
Landlock is an optional LSM (although it seems to be on-by-default in at least some distributions, perhaps many distributions - unlike the "big" LSMs AppArmor, SELinux and Smack, which are currently still mutually exclusive, so each distro has to choose no more than one of them to be the default, and if sysadmins are allowed to choose a different "big" LSM then they have to turn off the distro's default). This means we need to consider what will happen if the distro or sysadmin has turned it off, or if the kernel is too old to offer this feature.
At the moment, --scope-abstract-unix-sockets would "fail closed", refusing to start any apps that have that option in their sandboxing parameters. But bwrap is primarily designed to be used by higher-level frameworks like Flatpak, WebKitGTK and Glycin, rather than being used directly by end users, and I suspect that the maintainers of those frameworks would prefer to have a way to "fail open" - having your app run normally, but relying on Xauth to prevent access to the abstract X11 socket, seems like it would be better than nothing.
So do we need an accompanying --scope-abstract-unix-sockets-try option, similar to the relationship between --unshare-cgroup and --unshare-cgroup-try?
bwrap.xml
Outdated
| <varlistentry> | ||
| <term><option>--scope-abstract-unix-sockets</option></term> | ||
| <listitem><para> | ||
| Scope access to abstract unix sockets to within in the sandbox. |
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 does it mean to "scope" abstract sockets, more precisely? It's OK for the option name and the --help to be brief, but the man page should really give a little more detail about what this means.
If a process outside the sandbox (most commonly Xorg or Xwayland) listens on an abstract Unix socket, does this option prevent processes inside the sandbox from connecting to it, even in the absence of --unshare-net?
Conversely, if a process inside the sandbox listens on an abstract Unix socket, does this option prevent processes outside the sandbox from connecting to it, even in the absence of --unshare-net?
It would probably also be helpful to mention This has the same behaviour as LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET: see <citerefentry><refentrytitle>landlock</refentrytitle><manvolnum>7</manvolnum></citerefentry> for details. but I think at least a brief summary of what this option does is also necessary here.
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.
I've included a brief summary of what it means to "scope", but not of what an abstract unix socket is. Would you like me to include that as well, or is it okay as is?
| #include <linux/sched.h> | ||
| #include <linux/seccomp.h> | ||
| #include <linux/filter.h> | ||
| #include <linux/landlock.h> |
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 kernel/glibc versions, if any, guarantee to have this header?
The build system should probably check for this header with cc.check_header('linux/landlock.h') and define HAVE_LANDLOCK_H, so that this can become
| #include <linux/landlock.h> | |
| #ifdef HAVE_LANDLOCK_H | |
| #include <linux/landlock.h> | |
| #endif |
and then the use of Landlock features in the code can be something like (pseudocode)
if (opt_scope_abs_unix_sockets)
{
#ifdef HAVE_LANDLOCK_H
do Landlock things;
#else
die ("Landlock not available at compile time, cannot implement --scope-abstract-unix-sockets");
#endif
}
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.
Not sure about the exact version, but the header was introduced by Linux in cb2c7d1a1776057c9a1f48ed1250d85e94d4850d on Thu Apr 22 17:41:17 2021. Regardless, I also agree that we should have this as a check and guard it, so I'll wire that up.
bubblewrap.c
Outdated
| " --perms OCTAL Set permissions of next argument (--bind-data, --file, etc.)\n" | ||
| " --size BYTES Set size of next argument (only for --tmpfs)\n" | ||
| " --chmod OCTAL PATH Change permissions of PATH (must already exist)\n" | ||
| " --scope-abstract-unix-sockets Scope access to abstract unix sockets to within in the sandbox\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.
For this initial review pass I've assumed that you didn't change the --help except for re-indenting. I'm leaving this comment as a reminder that someone still needs to check this.
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.
Should be a non-issue now that we've changed the option name to be shorter.
bwrap.xml
Outdated
| <varlistentry> | ||
| <term><option>--scope-abstract-unix-sockets</option></term> | ||
| <listitem><para> | ||
| Scope access to abstract unix sockets to within in the sandbox. |
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 mimic the indentation of the better / more consistent options when adding new options. --cap-add and --cap-drop are better examples to follow.
bubblewrap.c
Outdated
| " --perms OCTAL Set permissions of next argument (--bind-data, --file, etc.)\n" | ||
| " --size BYTES Set size of next argument (only for --tmpfs)\n" | ||
| " --chmod OCTAL PATH Change permissions of PATH (must already exist)\n" | ||
| " --scope-abstract-unix-sockets Scope access to abstract unix sockets to within in the sandbox\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.
This name is rather long, as you said. --scope-abstract-af-unix would be shorter, perhaps? Or --scope-abstract-sockets since there is no other type of socket with the abstract vs. path-based distinction?
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.
Personally I prefer --scope-abstract-af-unix (which seems reasonable to me). I'll change it to that unless the other option is preferred.
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.
.
42cd329 to
588ad8d
Compare
Closes: containers#330 Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
588ad8d to
d43692e
Compare
Closes: #330