Allow blacklisting some wayland interfaces
ClosedPublic

Authored by apol on Jul 19 2019, 9:02 PM.

Details

Summary

For some interfaces, we'll look at the X-KDE-Wayland-Interfaces property in the desktop file to see which interfaces are requested.
Requires D22570.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.Jul 19 2019, 9:02 PM
Restricted Application added a project: KWin. · View Herald TranscriptJul 19 2019, 9:02 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
apol requested review of this revision.Jul 19 2019, 9:02 PM
davidedmundson added inline comments.
wayland_server.cpp
225

This has the same bug as outlined in the task.

The exe will resolve the file path that we see in kwins namespace, not the exe being run within the clients mountnamespace. We're comparing a process with itself.

We need to compare to /proc/PID/root/ + client->executablePath()

apol updated this revision to Diff 62103.Jul 20 2019, 10:06 AM

Address david's comment

This is a wonderful patch - I love it!

Can we put some enforcement on the desktop file like requiring that it is root owned? (would obviously suck for developer setups but prevents from malicious applications installing to $HOME).

And of course if this patch lands I have no objections any more to the fake key event injection in the fake input protocol which is required for kdeconnect and remote access.

Can we put some enforcement on the desktop file like requiring that it is root owned?

Urgh, please no.

At most maybe we can check the location matches kwin's install_prefix.

wayland_server.cpp
221

Can you make us fail if both return an empty byte array.

229

This doesn't protect against the attack of just having a binary called /tmp/plasmashell without a container.

We'll match the executablePath.fileName

Then compare that /tmp/plasmashell matched /tmp/plasmashell

We want to compare /proc/PID/root/ + client->executablePath()
against QStandardPaths::findExecutable(servicesFound.exec())

?

apol marked 2 inline comments as done.Jul 20 2019, 4:05 PM
apol updated this revision to Diff 62131.Jul 20 2019, 4:27 PM

Address coding style, david's comments

apol updated this revision to Diff 62139.Jul 20 2019, 6:12 PM

improve debug

broulik added a subscriber: broulik.EditedJul 20 2019, 9:09 PM

I haven't tried but it looks like i can just place a .desktop file in ~/.local/share/applications which is user-writable and overide the system-wide installed one with one that has an X-KDE-Wayland-Interfaces with the interface I want to use in my nasty app.

EDIT: Nevermind, I should have read the discussion before blatantly commenting :)

zzag added a subscriber: zzag.Jul 21 2019, 9:15 AM

Some style nitpicks.

wayland_server.cpp
80–81

QCryptographicHash needs to go first in this block. KServiceTypeTrader include directory needs to be above this include block.

233

The debug output needs to be more verbose. At least we have some idea what origin of this message is.

though I think we can just drop this one.

257

Why a new scope is introduced?

apol edited the test plan for this revision. (Show Details)Jul 21 2019, 1:50 PM
apol updated this revision to Diff 62190.Jul 21 2019, 2:03 PM
apol marked an inline comment as done.

Address zzag's comments

apol marked an inline comment as done.Jul 21 2019, 2:03 PM
apol added inline comments.
wayland_server.cpp
257

Because it's checking different things.

fvogt added a subscriber: fvogt.Jul 24 2019, 10:27 AM

IMO there should be a clear warning or error message if and why a request was declined.

Currently this would break use of those interfaces from sandboxes and inside containers, others can do whatever they want to anyway (if they were "evil" it's already game-over).

wayland_server.cpp
252

Looking at /proc/pid/* requires ptrace permissions, which aren't always available and containerization technologies usually block those.

IMO there should be a clear warning or error message if and why a request was declined.

This can't be done here. We're blocking advertising the protocol, so it's before the client has requested anything.

If we want that sort of thing, we probably need the approach I suggested in the original task, but at this point I think we're probably better off merging something so we can unblock everything that depends on this and then we can build on it later.

IMO there should be a clear warning or error message if and why a request was declined.

This can't be done here. We're blocking advertising the protocol, so it's before the client has requested anything.

If we want that sort of thing, we probably need the approach I suggested in the original task, but at this point I think we're probably better off merging something so we can unblock everything that depends on this and then we can build on it later.

IMO a qWarning would be enough for a start.

apol updated this revision to Diff 62736.Jul 29 2019, 2:02 PM

add debug statements

apol added a comment.Jul 29 2019, 2:12 PM

IMO a qWarning would be enough for a start.

Added them, and it's still quite verbose. Note that all (Qt) applications will request all interfaces just in case, so it shows multiple unnecessary requests.

davidedmundson accepted this revision.Jul 29 2019, 10:46 PM
This revision is now accepted and ready to land.Jul 29 2019, 10:46 PM
This revision was automatically updated to reflect the committed changes.
fvogt added inline comments.Aug 1 2019, 12:43 PM
wayland_server.cpp
225

AFAICT, "We're comparing a process with itself." is still the case.

It would need to compare the content of client->executablePath() with /proc/pid/exe, but even that can be faked easily.

Note that procfs is unusual - sha256("/proc/pid/exe") != sha256(readlink("/proc/pid/exe"))

fvogt added inline comments.Sep 18 2019, 2:36 PM
wayland_server.cpp
225

Any news on this?

fvogt added inline comments.Sep 30 2019, 2:45 PM
wayland_server.cpp
225

Ping?

apol added inline comments.Oct 7 2019, 10:38 AM
wayland_server.cpp
225

No, what you wrote isn't what we are doing. We are going through /prog/pid/root/...
I don't really see how it would be faked for a remote process reliably.

fvogt added inline comments.Oct 28 2019, 4:03 PM
wayland_server.cpp
225

No, what you wrote isn't what we are doing. We are going through /prog/pid/root/...

Exactly that is the issue. Let me demonstrate:

docker run --rm -it opensuse/tumbleweed /usr/bin/cat
Now client->executablePath() is /usr/bin/cat and you compare /proc/$(pidof cat)/root/usr/bin/cat against /proc/$(pidof cat)/exe.
That's always identical:

> diff -s /proc/$(pidof cat)/exe /proc/$(pidof cat)/root/usr/bin/cat
Files /proc/21994/exe and /proc/21994/root/usr/bin/cat are identical
> diff -s /proc/$(pidof cat)/exe /usr/bin/cat
Binary files /proc/21994/exe and /usr/bin/cat differ

I don't really see how it would be faked for a remote process reliably.

Ignoring the issue above that the executable itself does not need to be faked, here are two ways:

  • By just not faking it: LD_PRELOAD=pwn.so /usr/bin/plasmashell
  • By changing the executable file between establishing the wayland connection and requesting interfaces

That's effectively the very same test I outlined, and I ended up concluding the opposite :/
Maybe I fell for this readlink trap. Sorry for not getting back to you on that.

Hopefully you're right as it'll simplify the code.

By just not faking it: LD_PRELOAD=pwn.so /usr/bin/plasmashell

Sure. But that's out of scope. We shoudln't pretend we can completely protect against unsandboxed code running as your user. You're somewhat screwed regardless.

fvogt added a comment.Oct 28 2019, 4:49 PM

By just not faking it: LD_PRELOAD=pwn.so /usr/bin/plasmashell

Sure. But that's out of scope. We shoudln't pretend we can completely protect against unsandboxed code running as your user. You're somewhat screwed regardless.

You missed the point there: That would work just fine for sandboxed code as well, even if the current code was fixed to compare the right file contents.

fvogt added inline comments.Nov 6 2019, 10:09 AM
wayland_server.cpp
225

Ping. This code seems to be active still while doing the exact opposite of what it should and I wonder why it hasn't been reverted.
It's also responsible for breaking xdg-desktop-portal-kde currently.

> diff -s /proc/$(pidof cat)/exe /proc/$(pidof cat)/root/usr/bin/cat
Files /proc/21994/exe and /proc/21994/root/usr/bin/cat are identical
> diff -s /proc/$(pidof cat)/exe /usr/bin/cat
Binary files /proc/21994/exe and /usr/bin/cat differ

I got the same results with docker.
Though /proc/someFlatPakApp/root/bin/cat is different to my system one which is where my logic came from.

But given you're right about readlink (exe) and read(exe) behaving differently, we should be able to simplify things and have it work.

I shall do that now.

It's also responsible for breaking xdg-desktop-portal-kde currently.

The bug outlined above is not responsible for that. It's something else. (Possibly the NoDisplay flag?)
If anything, it is a sign that the general filter does work though...

Lets track xdg-desktop-portal in a bug report, and let's make use of the task https://phabricator.kde.org/T4437#111632 for any new approaches. There were some other suggestions there.

fvogt added a comment.Nov 6 2019, 12:15 PM

But given you're right about readlink (exe) and read(exe) behaving differently, we should be able to simplify things and have it work.

I shall do that now.

The whole approach of comparing executables is just completely flawed and does not provide any security, only maybe "accidental" use of an interface by an application.

I'll reopen and comment on T4437.

It's also responsible for breaking xdg-desktop-portal-kde currently.

The bug outlined above is not responsible for that. It's something else. (Possibly the NoDisplay flag?)

I didn't mean that this bug was responsible, but rather that the blacklisting is. There is just no .desktop file at all in xdg-desktop-portal-kde 5.17, so it's completely blocked.