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

[WIP] Wayland protocol security #3088

Closed
wants to merge 4 commits into from
Closed

Conversation

emersion
Copy link
Member

@emersion emersion commented Nov 8, 2018

This is just a basic patch that filters globals on a per-client basis.

The wl_client → command path conversion is done with the PID. I chose to do this because:

(Also, it's not racy -- if the PID has been re-allocated for another process then the connection has been closed anyway)

More elaborate solutions like making the compositor start processes or making clients ask the compositor to do so can be added on top of this later.

TODO:

  • Check /proc/<pid>/root
  • Decide on a security file format. We thought of designing a standard file format so that clients can work on other compositors too. The idea was to have security files stored in e.g. /etc/wayland-security.d/ that would be installed with clients. Maybe this should be done after we've proved this system works.
  • -Dinsecure
  • Docs

@bl33pbl0p
Copy link

(Also, it's not racy -- if the PID has been re-allocated for another process then the connection has been closed anyway)

What if the connection is just kept open by also passing the file descriptor to some other process, and then the PID reuse happens?

@emersion
Copy link
Member Author

emersion commented Nov 10, 2018

What if the connection is just kept open by also passing the file descriptor to some other process, and then the PID reuse happens?

The process which owns the file descriptor is responsible for keeping it safe.

@emersion emersion requested a review from ddevault November 11, 2018 22:50
@emersion
Copy link
Member Author

Ping

@ddevault
Copy link
Contributor

I'm skeptical about FreeBSD. I'd like to see a libwayland patch first.

@valpackett
Copy link
Contributor

I'm not sure how a libwayland patch would be possible. That link goes to my kernel patch that makes it actually store the PID.

@ddevault
Copy link
Contributor

I don't mind if a kernel patch is necessary first.

@emersion
Copy link
Member Author

I don't think a kernel patch is necessary. Have you seen that cmsgcred.cmcred_pid exists? Just need to send this message as ancillary data when connecting.

@valpackett
Copy link
Contributor

Oh! Making libwayland explicitly send the credentials, that would work. (Though there are alternative Wayland implementations such as wayland-rs already, they have to be changed as well.)

@emersion emersion closed this Jan 18, 2019
@emersion emersion deleted the security branch January 18, 2019 13:07
@emersion emersion restored the security branch January 20, 2019 01:37
@emersion emersion reopened this Jan 20, 2019
@emersion
Copy link
Member Author

Turns out this approach is a bad idea. Not a very bad idea because it's hard to exploit (Flatpak is vulnerable because it's reading /proc/$pid), but it's still bad. Thanks Simon McVittie for explaining why. Full discussion: https://lists.freedesktop.org/archives/wayland-devel/2019-January/039866.html

There are two mechanisms to check a peer's credentials: SO_PEERCRED and SCM_CRDENTIALS. These are Linux-specific but there are BSD counterparts.

SO_PEERCRED is a way to get the PID/GID/UID when the socket is opened. I thought this was safe, because even if a PID has been re-used between wl_client_get_credentials and read("/proc/$pid/exe"), this would mean the old process has been killed hence the connection has been closed. However this isn't true:

  1. A malicious process opens a Wayland connection, has PID x
  2. It forks, and then the parent process exits
  3. A privileged binary is launched with PID x
  4. The child process has escalated privileges

SCM_CREDENTIALS allows one process to send its PID/GID/UID to another via an ancillary message on a Unix socket. The kernel will check that the credentials are correct (or that the sending process has permission to fake them). Using this for this use-case is exploitable:

  1. A malicious process opens a Wayland connection
  2. Unset the CLOEXEC flag on the connection's FD
  3. Buffer a SCM_CREDENTIALS message on the socket with the PID of a privileged process
  4. Execute a suid binary
  5. The buffered message is sent, the kernel accepts the fake PID since the process is suid

@emersion
Copy link
Member Author

emersion commented Jan 21, 2019

TBH I wonder if we should do protocol security at all. Does it even make sense to try to restrict access to some Wayland global when processes can set $PATH globally?

Without containers/jails/VMs/whatever, trying to restrict Wayland interfaces will not prevent anything bad. It can still be nice to have to prevent everybody from using all possible privileged protocols, but then this solution is good enough (by far). Though, we shouldn't lie to our users and say it's secure.

The better solution would be to use a container software like e.g. Flatpak, implement container-specific access control interfaces (for Flatpak, it's an xdg-desktop-portal backend), and forbid apps running under containers from accessing our privileged globals.

@ddevault
Copy link
Contributor

I've long suspected that pid-based security was a dead end. Why don't we return to working on ideas based on WAYLAND_SOCKET, e.g. authenticating a specific connection and only for clients launched directly by the compositor?

@emersion
Copy link
Member Author

Why don't we return to working on ideas based on WAYLAND_SOCKET, e.g. authenticating a specific connection and only for clients launched directly by the compositor?

See my second post.

@ddevault
Copy link
Contributor

Sorry, what part of it?

@emersion
Copy link
Member Author

I don't think implementing strong Wayland protocol security is useful for regular un-containerized clients. Those clients don't need Wayland interfaces to do evil things, and we're not the ones responsible for stopping them.

@ddevault
Copy link
Contributor

A simple chroot should be sufficient to prevent many of those bad things. Still, how do you intend to determine if a process is containerized? How will you grant that process privileges if it ought to have some?

@ddevault
Copy link
Contributor

Oh, like this:

The better solution would be to use a container software like e.g. Flatpak, implement container-specific access control interfaces (for Flatpak, it's an xdg-desktop-portal backend), and forbid apps running under containers from accessing our privileged globals.

Bleh. I'm not opposed to using this method to give flatpak processes et al their permissions, but I'm not convinced we can't also secure processes which are isolated more simply, e.g. chrooted.

@emersion
Copy link
Member Author

emersion commented Jan 21, 2019

A simple chroot should be sufficient to prevent many of those bad things.

I don't think chroot is well-suited for running desktop apps. You need to give access to various things like the DRM render node and /dev/shm. Maybe something like OpenBSD's unveil would be better.

Even so, if the Wayland compositor exposes an interface that allows to start new processes, then it's easy to escape the chroot.

@ddevault
Copy link
Contributor

Providing those things is easy, though. Mounting /dev into the chroot is trivial and required for many, many applications.

Even so, if the Wayland compositor exposes an interface that allows to start new processes, then it's easy to escape the chroot.

So... don't provide that?

@emersion
Copy link
Member Author

emersion commented Jan 21, 2019

So... don't provide that?

Well, you just said:

Why don't we return to working on ideas based on WAYLAND_SOCKET, e.g. authenticating a specific connection and only for clients launched directly by the compositor?

@ddevault
Copy link
Contributor

What part of that implies a protocol which lets clients escape a chroot by running arbitrary processes?

@emersion
Copy link
Member Author

I remember this was the result of last time's attempt at making the compositor launch processes: swaywm/wlr-protocols#27

@ddevault
Copy link
Contributor

That's unrelated to the present discussion. If it doesn't work we can just ditch that protocol and the rest is still secure (or I think it is, but we won't know if we never get back on topic instead of hemming and hawing about unrelated crap).

@ddevault
Copy link
Contributor

Work based on the WAYLAND_SOCKET idea still exists here btw, in a dubiously mergable state #2375

@emersion
Copy link
Member Author

All right, fair enough. I'm not sure a lot of people will use this feature then, if it requires setting up per-app chroots.

I guess this can just be closed.

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

Successfully merging this pull request may close these issues.

4 participants