ScreenshotEffect: Use Service Property to authorize screenshot without confirmation
Needs ReviewPublic

Authored by meven on Mon, May 4, 1:43 PM.

Details

Reviewers
apol
davidedmundson
bport
zzag
Group Reviewers
KWin
Summary

Restrict to process with X-KDE-DBUS-Restricted-Interfaces=org_kde_kwin_effect-screenshot in their corresponding Service file,
to take screenshots.
Such a program can now take immediate screenshots.

Adds a utility file to group KService related logic.

Needed for D29408

Diff Detail

Repository
R108 KWin
Branch
arcpatch-D29407
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26957
Build 26975: arc lint + arc unit
meven created this revision.Mon, May 4, 1:43 PM
Restricted Application added a project: KWin. · View Herald TranscriptMon, May 4, 1:43 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
meven requested review of this revision.Mon, May 4, 1:43 PM
apol added inline comments.Mon, May 4, 1:54 PM
effects/screenshot/screenshot.cpp
79

But this is not a wayland interface, is it? :/

meven added inline comments.Mon, May 4, 1:56 PM
effects/screenshot/screenshot.cpp
79

I am abusing things here by using the same X-KDE-Wayland-Interfaces for authorized Wayland protocol and DBUS interface.
Do you think it is necessary to separate the two ?
Maybe just this variable name ?

meven edited the summary of this revision. (Show Details)Mon, May 4, 2:46 PM
ngraham added a subscriber: ngraham.EditedMon, May 4, 6:51 PM

Can you provide a bit more explanation on what this is doing?

meven edited the summary of this revision. (Show Details)Tue, May 5, 5:36 AM
meven edited the summary of this revision. (Show Details)
meven edited the summary of this revision. (Show Details)Tue, May 5, 5:38 AM
meven planned changes to this revision.Tue, May 5, 4:19 PM

TODO:
remove X-KDE-Original-Executable
use KApplicationTrader::query instead of KServiceTypeTrader
Probably add support for a X-KDE-DBus-Restricted-Interfaces in services_utils

meven edited the summary of this revision. (Show Details)Thu, May 7, 1:16 PM
meven updated this revision to Diff 82199.Thu, May 7, 1:28 PM

Use KApplicationTrader instead of KServiceTypeTrader, differentiate DBus and Wayland interface

meven updated this revision to Diff 82222.Thu, May 7, 4:33 PM

Improve include

meven added a comment.Mon, May 11, 8:50 AM

It has been reminded me that this solution to have some security rest entirely on the guarantees offered by $XDG_DATA_DIRS.
Same can be said about X-KDE-Wayland-Interfaces.

But currently I believe this does not constitutes a strong security model.
A malicious executable could manufacture a fake $XDG_DATA_DIRS, add an application folder in it and a desktop file for its executable, trigger kbuildsyscoca5 and then use any of the restricted interfaces.
We would need further to restrict path for which we would consider the desktop file, for instance, like only root owned path.

meven added a comment.Wed, May 13, 5:09 PM

I guess it is for 5.20.

meven added a reviewer: zzag.Fri, May 15, 8:59 AM
apol added a comment.Fri, May 15, 2:15 PM

Patch looks good to me overall.

service_utils.h
46

move implementations to service_utils.cpp?

67

fetchRequestedWaylandInterfaces?

meven updated this revision to Diff 82953.Fri, May 15, 2:55 PM
meven marked an inline comment as done.

Improve naming of fetchRequestedWaylandInterfaces

service_utils.h
46

I tried to do it, but I encountered errors.
Unless I am mistaken it is because this static function is shared with effects and effects are compiled to a separate shared lib.
So I would need to make a new build target for this one file to be able to share it with KWin::Core unless copied in both binaries.

meven added a comment.Fri, May 15, 2:58 PM

It has been reminded me that this solution to have some security rest entirely on the guarantees offered by $XDG_DATA_DIRS.
Same can be said about X-KDE-Wayland-Interfaces.

But currently I believe this does not constitutes a strong security model.
A malicious executable could manufacture a fake $XDG_DATA_DIRS, add an application folder in it and a desktop file for its executable, trigger kbuildsyscoca5 and then use any of the restricted interfaces.
We would need further to restrict path for which we would consider the desktop file, for instance, like only root owned path.

And if you have any suggestion regarding this.
In relation to this, wayland_server.cpp has a isTrustedOrigin function that checks using a hash if the executable matches the own in /proc/<pid>/exe

meven marked 2 inline comments as done.Wed, May 20, 10:00 AM

So this should be good for merging @apol after spectacle D29408 has been rebased and D29487 has landed