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.
Details
- Reviewers
davidedmundson - Group Reviewers
Plasma KWin - Commits
- R108:f32477613734: Allow blacklisting some wayland interfaces
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.
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() |
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() ? |
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 :)
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? |
wayland_server.cpp | ||
---|---|---|
257 | Because it's checking different things. |
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 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.
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")) |
wayland_server.cpp | ||
---|---|---|
225 | Any news on this? |
wayland_server.cpp | ||
---|---|---|
225 | Ping? |
wayland_server.cpp | ||
---|---|---|
225 | No, what you wrote isn't what we are doing. We are going through /prog/pid/root/... |
wayland_server.cpp | ||
---|---|---|
225 |
Exactly that is the issue. Let me demonstrate: docker run --rm -it opensuse/tumbleweed /usr/bin/cat > 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
Ignoring the issue above that the executable itself does not need to be faked, here are two ways:
|
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.
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.
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. |
> 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.
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.