Differential D13008

Disallow running KWin/Wayland as root
ClosedPublic
Actions

Authored by graesslin on May 20 2018, 7:51 PM.

Details

Reviewers
davidedmundson
Group Reviewers
KWin
Plasma
Commits
R108:267b5a11222e: Disallow running KWin/Wayland as root
Summary

KWin is not designed to run as root. It is not hardened enough and
there is a great risk that applications could attack KWin through
X11 properties, Wayland protocol requests, etc. to trigger stack or
heap overflows and execute random code. As clients connected to KWin
could be remote there is a great risk in running KWin as root. Also
clients on the same system but started as a different user could
try to gain more privs by exploiting KWin.

Furthermore KWin is designed to not run as root. It interacts with
logind to get the devices opened which would require root. Whether
KWin would work as root at all is questionable.

We cannot guarantee that running KWin as root is secure, thus this
change disallows running KWin and thus a Wayland session as root.

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.
graesslin created this revision.May 20 2018, 7:51 PM
Restricted Application added a project: KWin. · View Herald TranscriptMay 20 2018, 7:51 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
graesslin requested review of this revision.May 20 2018, 7:51 PM
ngraham added a subscriber: ngraham.May 20 2018, 8:08 PM

Am I correct that this will prevent root GUI sessions in all distros that decide not to patch out this change? If so, could you at least provide a CMake flag to make it easy for distros to patch it out if they so choose?

davidedmundson added a subscriber: davidedmundson.May 20 2018, 8:12 PM

Why only main_wayland?

zzag added a subscriber: zzag.May 20 2018, 8:23 PM
bshah added a subscriber: bshah.May 21 2018, 6:46 AM

+1 to this change,

@ngraham I am kinda opposed to the idea of having cmake switch, we do already know that running as root is broken [1].. so specifying it early and just aborting makes sense.. there is no point in allowing broken behavior by cmake switch -DENANBLE_BROKEN_BEHAVIOR

[1] In early experiments of Plasma Mobile on imx6 based development board, Mathias Klump was hitting very weird screen corruption and other weird issues, it turned out they were broken because kwin_wayland was running as root. Running kwin as normal user fixed it.

graesslin added a comment.May 21 2018, 8:20 AM
In D13008#265655, @davidedmundson wrote:

Why only main_wayland?

Because on main_x11 the thread potential is different. If KWin_x11 is running as root, we can assume that other applications are also running as root and also that the X server is running as root. Especially KWin is not the windowing system in that case. I'm personally in favor of disallowing kwin_x11 as root, but I also don't want to break user's workflows needlessly.

In D13008#265650, @ngraham wrote:

Am I correct that this will prevent root GUI sessions in all distros that decide not to patch out this change? If so, could you at least provide a CMake flag to make it easy for distros to patch it out if they so choose?

Certainly not. I do not want to support this. Running as root most likely doesn't work and is a bad idea from KWin's security model (not designed for it, supporting that would require a rewrite from scratch). I'm certainly not going to encourage distributions to enable it by adding a cmake flag. I want to be able to close any bug report related to that as invalid as we don't support it. Also I don't want to have to consider whether a bug could be abused when running as root. If distros want to do that: fine. But I won't spend my time on supporting that.

davidedmundson accepted this revision.May 21 2018, 8:21 AM
This revision is now accepted and ready to land.May 21 2018, 8:21 AM
Closed by commit R108:267b5a11222e: Disallow running KWin/Wayland as root (authored by graesslin). · Explain WhyMay 21 2018, 2:59 PM
This revision was automatically updated to reflect the committed changes.
ngraham added a comment.May 21 2018, 3:05 PM

Ah ok. That makes sense. I guess root-on-Wayland isn't really a supported use case in any circumstance. Sorry for cluttering up the discussion.

Revision Contents
Changeset List

PathPackages
Mmain_wayland.cpp (4 lines)

Diff 34583

View Options

main_wayland.cpp

Loading...