[kwayland] Security filter
Open, Needs TriagePublic

Description

Several interfaces in KWayland (Server) need to be security aware. That is not every application should be allowed to access it.

An idea is that applications are able to install a config file into a well-defined configuration directory. The absolute file path to the binary is the group, the key is the interface name, the value being Allowed, Denied or Undecided. E.g.:

    [/usr/bin/plasmashell]
    PlasmaShell=Allowed
    Screenshot=Denied

KWayland gains a security interceptor which is invoked whenever a client tries to bind an interface. The security interceptor invokes a chain of registered interceptors which evaluate whether the client is allowed to bind the interface. If the current interceptor returns undecided the next is invoked. On allowed or denied the interception is ended.

KWayland would provide three default interceptors:

  • allow all
  • deny all
  • configuration directory based

The user of the library can build up it's own security interceptor chain including the default ones and custom ones.

sitter added a subscriber: sitter.

Different proposal.

  1. We use wl_global_filter rather than doing our own thing inside bind.
  1. We do the following:

Clients send a DBus message before their first QApplication. Something like:

org.kde.KWin /restrictedProtocols/shellInterface org.kde.kwin.RestrictedProtocol enable

with no parameters.
We return nothing or an error.

We extract the PID (which is reliable) and store in a hash.
We have than that available for our filter.

What stops everyone just calling that?

From KWin's POV it becomes someone else's problem.
Kwin can't filter file access or anything else essential - so for sandboxing you need that software that's already working at the right layer.

  • Standard sandboxed apps don't have access to the session bus (or if they do, it's already the job of the sandboxing tech to limit/prompt)
  • Current AppArmor/SELinux can match a path easily and there they can do whatever.

Filtering screenshots and filtering moving windows is in the same place. Cleaner to sysadmin/debug.

Also it means the overall system has one point of failure, rather than building our own thing and making two.

davidedmundson added a comment.EditedJun 21 2018, 10:21 PM

Found a secuity hole in kwin's exe detection.

We follow the symlink of /proc/PID/exe
but that path is based on the user namespace of the parent process, which could have completely separate mountpoints to kwin.
I can just in my separate namespace mount something else as /usr/bin

So ironically the original suggested approach will work for everything *except* sandboxed applications,

Example Dockerfile

FROM python:3.6
LABEL maintainer="David Edmundson <david@davidedmundson.co.uk>"

RUN echo "echo I am some exploit ; sleep 1000" > /some_exploit
RUN mkdir -p /opt/kde5/bin
RUN ln /bin/dash /opt/kde5/bin/plasmashell
CMD /opt/kde5/bin/plasmashell /some_exploit

/proc/pid_of_the_new_shell_script/exe points to our real /usr/bin/plasmashell

You can probably do some clever solution parsing /proc/PID/root but then we're into the world of hacks.

ngraham added a subscriber: ngraham.EditedJul 10 2019, 7:10 PM

One thing I'd like to ensure is that any additional security measures provided by Wayland are not detrimental to the user experience, because security that burdens and annoys the user will eventually irritate them into finding a way to bypass it, or just make them stop using the software out of frustration (and if they have to use it because it's employer or school provided, they will grow to hate it).

For example, the current "click to acknowledge that you want to take a screenshot" UX on Wayland is a major gripe of mine, and it make Spectacle less useful and elegant than it is on X11. There are even use cases where it would actively destroy the user experience, such as taking a full-screen screenshot to a file while playing an first-person shooter game. In this case, when you hit the screenshot key, you could get killed while the screen is showing the screenshot, and you could fire your gun by accident when you click to acknowledge it, potentially killing a teammate or giving away your position.

For this, a better UX might be provided by taking inspiration from mobile, where the user is generally asked once to allow the current app some kind of blanket permission (access the camera, access contacts, etc). If we did that, then the user would authorize Spectacle to be able to take screenshots once, and never have to think about it again.

This would in fact improve security because with fewer permissions requests, the user is more likely to actually read and evaluate each one individually. By contrast, when the user sees them all the time, they're likely to mindlessly click through to get back to what they're doing, like they do on Windows with its UAC spam.

Recent relevant patches: D22209, D22033

A relevant link on future work on app identification.

https://bugs.freedesktop.org/show_bug.cgi?id=100344

For this, a better UX might be provided by taking inspiration from mobile, where the user is generally asked once to allow the current app some kind of blanket permission (access the camera, access contacts, etc). If we did that, then the user would authorize Spectacle to be able to take screenshots once, and never have to think about it again.

To do this properly there would have to be a capability security system like sandstorm.io or fuscia

I've been using setcap in a work project, and it's worth mentioning.

Effectively it's just an xattr but with the key in the form of "security.blah" which can only be set by root, regardless of the file permissions - but it's easily extensible to check user.blah for a more portable (less safe) version.

You don't have symlink issues as we're looking at the executable itself.

It's what other kernel processes use and my main objection was for us to avoid re-inventing a wheel.

romangg closed this task as Resolved.Sep 16 2019, 12:04 PM
romangg claimed this task.
romangg removed romangg as the assignee of this task.Sep 16 2019, 12:07 PM
romangg added a subscriber: romangg.
fvogt reopened this task as Open.EditedWed, Nov 6, 12:25 PM
fvogt added a subscriber: fvogt.

This got implemented in https://phabricator.kde.org/D22571, which is flawed (see the various comments there).

My suggestion:

If the goal is to just not allow processes running inside a sandbox to use certain interfaces, just check whether /proc/$pid/ns/mnt is the same (compare the string returned by readlink) as /proc/self/ns/mnt.
That way .desktop files aren't required either.

What the goal is is not really agreed upon.