<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/126115/">https://git.reviewboard.kde.org/r/126115/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 19th, 2015, 3:08 p.m. CET, <b>Matthias Klumpp</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Did you consider running the whole script with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">env -i</code>, or (likely the better idea) run KWin with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">env -i</code>?
That should sanitize the environment (unset all env vars, except for shell-defaults). You could then set exactly the variables you need, to the exact values you want, so we don't miss unsetting anything.</p></pre>
</blockquote>
<p>On November 19th, 2015, 3:11 p.m. CET, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No I didn't consider that, because I wasn't aware that this exists.</p></pre>
</blockquote>
<p>On November 19th, 2015, 3:20 p.m. CET, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Just tried with changing directly in the wayland session file. That doesn't work at all. I think the main problem is that I lose important env variables related to the logind session/dbus, etc.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So only way would be for the command to start kwin_wayland. But that as well would require to set quite an amount of env variables, but worth a try.</p></pre>
</blockquote>
<p>On November 19th, 2015, 3:55 p.m. CET, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Just gave a try - the command looks horrible, but I got the session started and env variables are properly filtered.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Command looks like this now:
/usr/bin/env -i KDE_FULL_SESSION=true KDE_SESSION_VERSION=5 KDE_SESSION_UID=${KDE_SESSION_UID} XDG_CURRENT_DESKTOP=KDE QT_QPA_PLATFORM=wayland PAM_KWALLET5_LOGIN=${PAM_KWALLET5_LOGIN} USER=${USER} LANGUAGE=${LANGUAGE} XDG_SEAT=${XDG_SEAT} XDG_SESSION_TYPE=${XDG_SESSION_TYPE} XCURSOR_SIZE=${XCURSOR_SIZE} HOME=${HOME} DESKTOP_SESSION=${DESKTOP_SESSION} XDG_SEAT_PATH=${XDG_SEAT_PATH} DBUS_SESSION_BUS_ADDRESS=${DBUS_SESSION_BUS_ADDRESS} LOGNAME=${LOGNAME} XDG_SESSION_CLASS=${XDG_SESSION_CLASS} XDG_SESSION_ID=${XDG_SESSION_ID} PATH=${PATH} XDG_SESSION_PATH=${XDG_SESSION_PATH} XDG_RUNTIME_DIR=${XDG_RUNTIME_DIR} XCURSOR_THEME=${XCURSOR_THEME} LANG=${LANG} XDG_SESSION_DESKTOP=${XDG_SESSION_DESKTOP} XCURSOR_PATH=${XCURSOR_PATH} XDG_VTNR=${XDG_VTNR} PWD=${PWD} XDG_DATA_DIRS=${XDG_DATA_DIRS} XDG_CONFIG_DIRS=${XDG_CONFIG_DIRS} @KWIN_WAYLAND_BIN_PATH@ --xwayland --libinput --exit-with-session=@CMAKE_INSTALL_FULL_LIBEXECDIR@/startplasma</p></pre>
</blockquote>
<p>On November 19th, 2015, 4:01 p.m. CET, <b>Matthias Klumpp</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Urgh, terrible! But I think this might just be the best workaround we can come up with, given the circumstances. It at least protects against someone adding new env vars which load bad code into the compositor. It might be an issue as soon as kwin starts to require another env var which isn't in the list, but that's an issue we can solve.
I wonder if we can simplify that command somehow, though, e.g. by placing the allowed variables in a file or new variable, to make this easier to read.
Apart from that, +1 from me (I'll take a look at other DEs though, maybe someone has come up with a bettr solution to this issue).</p></pre>
</blockquote>
<p>On November 19th, 2015, 4:56 p.m. CET, <b>Xuetian Weng</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Woundn't this break user's workflow? Since startplasma is started by kwin, the environment variable for the desktop will be derived from that.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If distribution has some global configuration under /etc/profile.d (which is really common, e.g. openjdk, mozilla plugin path), they will not be set.</p></pre>
</blockquote>
<p>On November 19th, 2015, 5:06 p.m. CET, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Woundn't this break user's workflow?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, but what do you prefer? Breaking user's workflows or a secure system?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There is nothing wrong of course with sourcing the env scripts in startplasma again and distributions using things like /etc/profile.d would be advised to do exactly that.</p></pre>
</blockquote>
<p>On November 19th, 2015, 5:37 p.m. CET, <b>Alex Richardson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't see how this adds any security if you still keep $PATH. what if the user prepends <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">$HOME/my-evil-binaries/</code> to $PATH?
Maybe it makes more sense to restrict which scripts are executed before kwin launches?</p></pre>
</blockquote>
<p>On November 19th, 2015, 5:37 p.m. CET, <b>Matthias Klumpp</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Maybe starting a session manager as the first thing makes sense. That one can then spawn KWin and KWin can tell the service to start plasmashell when it's ready.
Reminds me that I wanted to redesign the KDE startup process a while ago, getting rid of most of the scripts. A <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">systemd --user</code> based startup could do the exact same, btw.
Unsetting most vars will break assumptions, but since this affects only Wayland, it's probably okay for a little bit of breakage to exist, although if we can avoid that we should try to avoid breaking things.
Generally I agree with Martin: Security >> Breaking Workflows >> Backwards-Compatibility</p></pre>
</blockquote>
<p>On November 19th, 2015, 5:39 p.m. CET, <b>Matthias Klumpp</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Alex: KWin is started with an absolute path now, which makes $PATH and aliases irrelevant. So that particular issue is solved :)</p></pre>
</blockquote>
<p>On November 19th, 2015, 5:44 p.m. CET, <b>Alex Richardson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I know it doesn't make a difference for kwin. But I thought kwin will then run startplasma which will start all sorts of other stuff.
I'm just worried that a terminal (or any other program) that I start from a plasma session will not have all the required environment variables set if everything gets unset by kwin.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If this only affects the kwin process then I'm fine with it. Although if kwin doesn't launch start any binaries by name $PATH would also not be required. As it apparently does a user could replace thos with a malicious binary.</p></pre>
</blockquote>
<p>On November 19th, 2015, 5:50 p.m. CET, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Alex: we have two different problems to solve here: one is how to get KWin secure (and only KWin, keyloggers can only be installed in KWin), the other is how to keep the convenience for the Plasma session. Let's first make it secure and then look into the other problem.</p></pre>
</blockquote>
<p>On November 19th, 2015, 6:33 p.m. CET, <b>Xuetian Weng</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For security you need to have some assumption in the first place, otherwise it won't go anywhere.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For this specific case, you need to trust that everything set when start_wayland get started is legit. Otherwise it is still easy to hack the system with malicious environment (XDG_xx, DBUS_SESSION_BUS_ADDRESS) variable. For example, XDG_DATA_DIRS can already make kwin load arbitary qml file.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For things like pam_env, one should advise distribution to have secure pam configuration in the first place.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Otherwise it is still easy to hack the system with malicious environment (XDG_xx, DBUS_SESSION_BUS_ADDRESS) variable</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, each variable needs to be evaluated whether there is a risk for KWin.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">XDG_DATA_DIRS can already make kwin load arbitary qml file</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, but that's not a problem. For one QML doesn't expose the problem of logging keys and also we can remove those again before we load any QML. With QML it's relatively easy: we have code to tell Qt to load it, we can sanitize before (and should).</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For things like pam_env, one should advise distribution to have secure pam configuration in the first place.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, I absolutely agree!</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On November 19th, 2015, 1:22 p.m. CET, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma, David Edmundson and Matthias Klumpp.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Nov. 19, 2015, 1:22 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Any environment variable which can be used to specify a path to a
binary object to be loaded in the KWin process bears the risk of
being abused to add code to KWin to perform as a key logger.
E.g. an env variable pointing QT_PLUGIN_PATH to a location in $HOME
and adjusting QT_STYLE_OVERRIDE to load a specific QStyle plugin from
that location would allow to easily log all keys without KWin noticing.
As env variables can be specified in scripts sourced before the session
starts there is not much KWin can do about that to protect itself.
This affects all the LD_* variables and any library KWin uses and
loads plugins.
The list here is based on what I could find:
* LD_* variables as specified in the man page
* LIBGL_* and EGL_* as specified on mesa page
* QT_* variables based on "git grep qgetenv" in qtbase and qtdeclarative
combined with Qt's documentation
* "git grep getenv" in various KDE frameworks based on ldd output of KWin
Unfortunately the list is unlikely to be complete. If one env variable is
missed, there is a risk. Even more each change in any library might
introduce new variables.
The approach is futile, but needed till Linux has a secure way to start
the session without sourcing env variable scripts from user owned
locations.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>startkde/startplasmacompositor.cmake <span style="color: grey">(1e46e5be0a0d733fb01e1a87a34ee3c73a06bf8c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126115/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>