[effects/screenshot] Disable screenshot to file on Wayland
Needs RevisionPublic

Authored by graesslin on Jul 2 2019, 8:48 AM.

Details

Reviewers
ngraham
davidre
Group Reviewers
KWin
Spectacle
Summary

One of the biggest security issues on X11 was that any application could
take screenshots. On Wayland we addressed this by making the screenshot
process interactive. The user has to confirm explicitly that a
screenshot is taken. But our API still allows to create a screenshot and
save it directly to a temporary file. It shows a notification, but
nevertheless it is a small security issue.

Thus this change addresses it by sending a dbus error in case the
methods are called on Wayland.

Test Plan

Run nested session before and after change and used qdbusviewer
to call screenshot. Before path was returned, after change no path was returned.

Diff Detail

Repository
R108 KWin
Branch
screenshot-unsupported-wayland
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 13510
Build 13528: arc lint + arc unit
graesslin created this revision.Jul 2 2019, 8:48 AM
Restricted Application added a project: KWin. · View Herald TranscriptJul 2 2019, 8:48 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
graesslin requested review of this revision.Jul 2 2019, 8:48 AM

I don't really agree, for the same reasons that I would have happily accepted David R's patch

X11 was a problem because we need to give sandboxed apps a raw X11 connection.
On wayland we have to give sandboxed apps a raw wayland connection.

It's really important to make sure there's no data leaks in the wayland connection.

It's not our responsibility to do anything else. It achieves only security theater at the cost of potentially breaking things.

I won't block this patch, as xdg-desktop-portal and spectacle use the other version, but I do want to state I truly dislike the trend.

I accept your general criticism, but I disagree. When we started Wayland and added the interactive protocol flatpak did not exist (practically) and we still had to assume that everything runs trusted. Even right now it's still a nice theory only that all apps will be sandboxed (on my system they aren't yet). Given that we should also look at the now and present issues. Having these dbus call open was a mistake when adding the interactive mode. Given that I see it as a bug fix to that.

Also I want to have our base Wayland compositor secure without having to rely on flatpak. If flatpak adds additional security that is nice, but we should be secure by default.

ngraham requested changes to this revision.Jul 5 2019, 12:14 PM
ngraham added reviewers: Spectacle, davidre.
ngraham added subscribers: davidre, ngraham.

I understand the security implications here, and I don't approve of the change.

In general, security that burdens and annoys the user is at best useless and at worst harmful, because the user will eventually be annoyed into finding a way to bypass it, or stop using the software. The whole "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.

I would prefer that we step back and re-examine how we provide authorization to take screenshots on Wayland, because there clearly are real security implications at play here. Instead of demanding authorization via click before every screenshot, perhaps we should allow apps to request blanket screenshot permissions once, not unlike how it's done on modern mobile phones. Then you would authorize Spectacle 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.

I'm adding Spectacle and @davidre so we can broaden the conversation and come up with something that works for everyone, results in a pleasant user experience, and retains security.

This revision now requires changes to proceed.Jul 5 2019, 12:14 PM

Yes, I briefly thought about something like a whitelist myself. But of course having an idea and realizing it are two different things. And this solution would probably also come with its own difficulties. But it would be awesome if we could do something similar.

Lets not discuss that here. We get a scattering of notes randomly distributed across various review requests.

Relevant task thread: https://phabricator.kde.org/T4437

Though the first task before we start to propose any more solutions is to have some sort of general agreement of what our goals are.

Thanks, I'll add my comment there.

@ngraham I do not understand why you request changes. This diff has no implications on spectacle or any other application. The api call is neither intended to be used by an X11 application nor by a Wayland application and predates the interaction with ksnapshot.

nor by a Wayland application

Then how is a Wayland application supposed to take a screenshot to a file? Some other method?

Who currently uses it? Nobody?