Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

[RFC] Add wlr-exec-secure-unstable-v1.xml #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ddevault
Copy link
Contributor

@ddevault ddevault commented Oct 8, 2018

No description provided.

@ddevault
Copy link
Contributor Author

ddevault commented Oct 8, 2018

My intention is to pair this with two more standards:

  • A standard file format for /etc which specifies the permissions a client is authorized for, to be installed with the package for client software
  • Another protocol which lets you create new wayland sockets authorized to use a subset of your permissions. The purpose of this would be for e.g. flatpak to be authorized to do whatever it wants and add its own permissions on top of that.

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"/>
Copy link
Member

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.
Copy link
Member

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.

Copy link
Contributor Author

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.
Copy link
Member

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">
Copy link
Member

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

Copy link
Contributor Author

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">
Copy link
Member

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
Copy link
Member

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.

Copy link
Contributor Author

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.
Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor

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.

@emersion
Copy link
Member

emersion commented Oct 9, 2018

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.

@ddevault
Copy link
Contributor Author

ddevault commented Oct 9, 2018

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.

@agx
Copy link
Contributor

agx commented Oct 10, 2018

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.

@ddevault
Copy link
Contributor Author

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.

@emersion
Copy link
Member

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

@ddevault
Copy link
Contributor Author

Another problem is that it doesn't work with e.g. /usr/bin/python3

@agx
Copy link
Contributor

agx commented Oct 14, 2018

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.
This means the processes that could just use that model would need to change their behaviour a since it needs access to a privileged protocol.
I don't have a better solution to the problem though.

@agx
Copy link
Contributor

agx commented Oct 17, 2018

...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)?

@ddevault
Copy link
Contributor Author

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.

...that said an apparmor module for wayland protocol mediation

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.

Should we have a separate bug to handle privileged clients (since this protocol is (while possibly part of the solution) about something else)?

What are you thinking?

@nyorain
Copy link

nyorain commented Oct 19, 2018

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?
That's not a point against this protocol though, its role would be to allow application launcher clients or some cross-compositor wl-exec (instead of only sways exec)

@ddevault
Copy link
Contributor Author

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.

@dcz-purism
Copy link
Contributor

why clients (like a screenshot or video capture program or clipboard manager) have to work when not invoked by the compositor?

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.

@ddevault
Copy link
Contributor Author

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?

@dcz-purism
Copy link
Contributor

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,
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair.

@emersion
Copy link
Member

How does a protocol know that it needs to use this protocol to get elevated permissions?

  1. The process gets exec'ed
  2. It uses this protocol to be exec'ed by the compositor
  3. goto 1

Relying on WAYLAND_SOCKET would be very fragile.

@ddevault
Copy link
Contributor Author

Maybe we should add an event to this protocol which indicates if your process was already spawned securely.

@emersion
Copy link
Member

emersion commented Nov 8, 2018

I wonder if this can work with Flatpak and/or D-Bus activation. Not that I use these myself.

@emersion
Copy link
Member

emersion commented Apr 7, 2019

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.

@emersion
Copy link
Member

emersion commented Jun 9, 2019

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.

@emersion
Copy link
Member

Just to clarify a little bit more: this protocol provides a very convenient way to escape process limits, chroots, containers and jails.

@brandsimon
Copy link

@emersion Why would your idea provide a convenient way to escape chroots/containers?
If I can connect to the sway socket I can always spawn processes with the privileges of sway, why would the above make things worse, what am I missing?

@brandsimon
Copy link

brandsimon commented Nov 19, 2019

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)
My only solution was a shared fd, because everything else can be readable by other processes (like env, parameters, ...).
If sway has to open the fd, sway could do the authentication itself, so it would open swaysock and set the privileges for the new child.

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.
This also has the benefit that if the fd is not provided, the child can exec itself through sway swaymsg exec-with-fd /path/to/myself params and sway will add the fd (either through a replacement like {}, or at the end).

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.
Any thoughts on this?

@emersion
Copy link
Member

emersion commented Nov 1, 2021

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants