-
Notifications
You must be signed in to change notification settings - Fork 29
[RFC] Add wlr-exec-secure-unstable-v1.xml #27
base: master
Are you sure you want to change the base?
Conversation
My intention is to pair this with two more standards:
|
protocol error is issued. The command will be executed without a shell. | ||
</description> | ||
<arg name="handle" type="new_id" interface="zwlr_exec_handle_v1"/> | ||
<arg name="command" type="array" summary="the command to execute"/> |
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 is argv[0] handled?
Maybe the actual executable you call and the arguments (with argv[0]) should be separate like the execv calls.
protocols to accomplish its basic tasks, it will likely wish to use | ||
wlr_exec_secure_unstable_v1 to ask the compositor to execute itself again. | ||
It's at the client's discretion to exit once the compositor runs the second | ||
process or to wait for it to complete. |
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 think it's unlikely that is how it's going to pan out.
It'll probably end up being wayland-exec ./my-prog args
, where this hypothetical wayland-exec is just a small program that handles all of this. Or otherwise small interactive launcher programs which try to open the real program.
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 think it's more likely that if my-prog
wants to access Wayland protocols it knows require authentication, it will naturally include a means to authenticate. This would become a basic requirement of that software.
<description summary="creates a wlr_exec_handle_v1 resource"> | ||
Creates an exec_handle for the requested command. The compositor can | ||
choose to disallow this request for certain commands, in which case a | ||
protocol error is issued. The command will be executed without a shell. |
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.
It should probably be explicit that the arguments are passed unmodified to execv, instead of just saying "executed without a shell".
No $VAR or ~ expansion, etc.
summary="pid_t of the child process"/> | ||
</event> | ||
|
||
<event name="exit"> |
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 don't like the idea of this event. It basically asks the compositor to become a service manager, and track extra client state over normal wayland clients. It just seems unnecessary in my opinion.
It also doesn't address harder questions about processes that fork themselves (daemon-like) or otherwise hand their WAYLAND_SOCKET to another process. The compositor doesn't have a way to track this kind of state and shouldn't be expected to.
Also, there is an indefinite amount of time before this is called. How long do we really expect launcher programs to wait around doing nothing? It would just be cleaner if you fire off the client and don't hear about it again.
As far as I can see, there are two potential use cases for this extension:
(General) Application launchers
As stated above, there isn't really a robust way to track the state of the clients (unless you want to implement/depend on full-blown service manager), and application launchers wouldn't care anyway. They would ignore this event, and destroy the object.
Applications gaining privileges for themselves
Perhaps a program wants to provide a proper interactive interface and really do care about the state of the process they create. We don't need the exit event to be able to implement this.
The fd/enviroment arguments allows clients to implement anything special it wants.
You can create a socketpair/pipe in the launcher process, pass it to the child process, and communicate anything you want back via that, including noticing when the socket/pipe closes (e.g. on death).
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.
It also doesn't address harder questions about processes that fork themselves (daemon-like) or otherwise hand their WAYLAND_SOCKET to another process. The compositor doesn't have a way to track this kind of state and shouldn't be expected to.
It's not becoming a service manager because I don't expect it to do this, either. This is basically fork/exec/wait the protocol, nothing more and it doesn't need to handle the edge cases. Clients using this and launched by this will be designed to accomodate it, the edge cases are not important.
Perhaps a program wants to provide a proper interactive interface and really do care about the state of the process they create. We don't need the exit event to be able to implement this.
The fd/enviroment arguments allows clients to implement anything special it wants.
I disagree, pid is a natural thing to include in this design and its absence would be glaring. Making applications deal with some secondary exit hatch which sends an exit status down the drain first is dumb. The compositor will already have an event loop and handling SIGCHLD isn't a tall order.
</description> | ||
</request> | ||
|
||
<event name="pid"> |
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.
As discussed in my comment about "exit", I don't really think this is necessary or that helpful, and there are other ways that this could be found out.
request and await the exit event. | ||
|
||
Once the compositor receives an exec request, it should fork, close all | ||
file descriptors (except for the transferred ones), then dup2 them to the |
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.
close all file descriptors
Including stdin, stdout, and stderr? Should they be set to /dev/null?
Obviously if they're explicitly set with fd, they should be set to that.
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.
We should probably specify that the behavior of stdin/stdout/stderr is unspecified if the client doesn't transfer them.
<description summary="transfer a file descriptor to the process"> | ||
Transfers a file descriptor to the new process. The compositor will use | ||
dup2 to assign the specified file descriptor number, then close the | ||
danging fd. |
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.
dangling fd
<event name="pid"> | ||
<description summary="the process's pid is available"> | ||
The compositor sends this after it has forked, to inform the client of | ||
the new process's pid. |
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.
Is this after fork and before exec of the new process or is this deliberately unspecified?
<description summary="a protocol for executing clients with higher permissions"> | ||
This protocol allows unprivileged clients to ask the compositor to execute | ||
a program which can access protected Wayland protocols. | ||
|
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 found the description somewhat confusing (I read it as that the protocol is not about privileges but rather to make sure that the program is spawned in a compositor controlled environment). I'd simply drop the
which can access protected Wayland protocols
part since this is not part of the protocol or add a
which can access protected Wayland protocols via the <TBD> protocol.
I'd also drop the "with higher permissions" part from the short description since this is (as I read it) not what the protocol does.
This proposal is similar to DBus socket activation. It has similar issues. For instance, when launching a process from the command-line, the process will be forked in the background by the compositor. This is annoying. |
I don't think we can get away from making the compositor execute it. However, if the command line process transfers its stdin/stdout/stderr and environment, then waits for exit and exits with the same status code, the user will be none the wiser. |
The re-execution also leads to issues with user session management, be it via systemd user units or e.g.gnome-session. The client using this protocol would need to supervise it's own (compositor spawned) process and can't just rely on e.g. systemd's 'simple' service type for being respawned. |
I think that falls under the same model as before. The systemd-spawned process can wait on the compositor spawned process and exit with the same exit code when it does. Then systemd will spawn another and so on. |
@Ongy What was the issue with reading the executable name from the PID again? I can't remember why it's racy. (It's still only available on Linux and OpenBSD, not FreeBSD) |
Another problem is that it doesn't work with e.g. /usr/bin/python3 |
That's why I said "can't just rely on 'simple' service" and this is still true. The process needs to do a lot more tracking that is handled by the supervisor otherwise. |
...that said an apparmor module for wayland protocol mediation (similar to dbus mediation) would solve this in linux but it's not cross plattform. While searching for prior art I came across: http://www.mupuf.org/blog/2014/02/19/wayland-compositors-why-and-how-to-handle/ based on this XDC talk https://www.x.org/wiki/Events/XDC2014/XDC2014DodierPeresSecurity/xorg-talk.pdf . Should we have a separate bug to handle privileged clients (since this protocol is (while possibly part of the solution) about something else)? |
I still don't see this as overlapping with a supervisor. The initial process just becomes a dumb proxy of the new process. Any supervisor can just monitor the initial process normally.
I don't really want to get AppArmor involved here. I don't think it's necessary and most Linux desktops aren't using it. I want to try to get security into this with a model which will work for most users, and most users aren't going to set up AppArmor.
What are you thinking? |
I haven't really followed the security considerations for sway so far too much but while reading the discussion the question i mainly had was why clients (like a screenshot or video capture program or clipboard manager) have to work when not invoked by the compositor? Why can't we simply tell users that when they want to execute something that they trust enough to give those permission they have to execute it via the compositor? |
We can. This is just a convenience, and something useful for clients like application launchers which run with lower permissions than the clients they launch. |
One of the cases where it would not be straightforward to get started from the compositor is anything automated - running an application test suite, remotely requesting a VNC server, etc. |
But in those cases, this protocol can be used, right? |
I was worried that the spawned program would have to find a new way to share the data from the new interface back to the requester, but this seems to be taken care of in the fd request. So yeah, seems possible. |
|
||
Once the compositor receives an exec request, it should fork, close all | ||
file descriptors (except for the transferred ones), then dup2 them to the | ||
appropriate file descriptor number. It should then exec (with PATH lookup, |
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.
-1 to exec'ing with PATH lookup. To apply security rules we need the full executable path anyway.
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.
Fair.
How does a protocol know that it needs to use this protocol to get elevated permissions?
Relying on |
Maybe we should add an event to this protocol which indicates if your process was already spawned securely. |
I wonder if this can work with Flatpak and/or D-Bus activation. Not that I use these myself. |
Oh, by the way, just thought of something: instead of making the compositor a process manager, we could just use a FD which is closed when the spawned client exits. So the "spawner" client code becomes: int fds[2];
pipe(fds);
// Ask the compositor to spawn ourselves, with dup'ed
// stdin/stdout/sterr, and to close fds[1] on exit
close(fds[1]);
struct pollfd pfd = {
.fd = fds[0],
.events = POLLIN,
};
poll(&pfd, 1, -1); And the compositor could just do this: int exit_fd; // FD sent by client
if (fork() == 0) {
exec("/path/to/client");
}
close(exit_fd); More complicated things to get back the exit code are possible too. |
Thought about this a little bit more, and it doesn't seem like this is going to cut it. Replicating a process' environment is not easy: for instance we'd need to forward signals so that typing Ctrl+Z would make the process spawned by the compositor stop. Additionally, the process spawned by the compositor wouldn't inherit the whole environment: there are more portable things like umask, and system-specific things like Linux containers and BSD jails. |
Just to clarify a little bit more: this protocol provides a very convenient way to escape process limits, chroots, containers and jails. |
@emersion Why would your idea provide a convenient way to escape chroots/containers? |
First of all, this is just brainstorming, maybe someone gets inspired, also I dont know all the implementation details, probably my approach with swaysock is wrong, because there is communication outside of it. I thought of a way how sway and the child can share a common secret (randomly created), though the secret can be used to authenticate at swaysock. (I am swaylock, let me lock the screen) The fd number will be passed as a string to the child as paramter (exec), so the child does not need to guess/rely on deterministic behaviour, to use the correct fd. It is important, that sway is executing the child, so LD_PRELOAD and other stuff can not get inbetween and can not write to the privileged fd. |
wlr-protocols has migrated to gitlab.freedesktop.org. This pull request has been moved to: https://gitlab.freedesktop.org/wlroots/wlr-protocols/-/merge_requests/27 |
No description provided.