> On Nov. 19, 2015, 3:08 p.m., Matthias Klumpp wrote:
> > Did you consider running the whole script with `env -i`, or (likely the
> > better idea) run KWin with `env -i`?
> > 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.
>
> Martin Gräßlin wrote:
> No I didn't consider that, because I wasn't aware that this exists.
>
> Martin Gräßlin wrote:
> 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.
>
> 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.
>
> Martin Gräßlin wrote:
> Just gave a try - the command looks horrible, but I got the session
> started and env variables are properly filtered.
>
> 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
>
> Matthias Klumpp wrote:
> 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).
>
> Xuetian Weng wrote:
> Woundn't this break user's workflow? Since startplasma is started by
> kwin, the environment variable for the desktop will be derived from that.
>
> 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.
>
> Martin Gräßlin wrote:
> > Woundn't this break user's workflow?
>
> Yes, but what do you prefer? Breaking user's workflows or a secure system?
>
> 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.
>
> Alex Richardson wrote:
> I don't see how this adds any security if you still keep $PATH. what if
> the user prepends `$HOME/my-evil-binaries/` to $PATH?
> Maybe it makes more sense to restrict which scripts are executed before
> kwin launches?
>
> Matthias Klumpp wrote:
> 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 `systemd --user` 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
>
> Matthias Klumpp wrote:
> @Alex: KWin is started with an absolute path now, which makes $PATH and
> aliases irrelevant. So that particular issue is solved :)
>
> Alex Richardson wrote:
> 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.
>
> 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.
>
> Martin Gräßlin wrote:
> @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.
>
> Xuetian Weng wrote:
> For security you need to have some assumption in the first place,
> otherwise it won't go anywhere.
>
> 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.
>
> For things like pam_env, one should advise distribution to have secure
> pam configuration in the first place.
> Otherwise it is still easy to hack the system with malicious environment
> (XDG_xx, DBUS_SESSION_BUS_ADDRESS) variable
Yes, each variable needs to be evaluated whether there is a risk for KWin.
> XDG_DATA_DIRS can already make kwin load arbitary qml file
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).
> For things like pam_env, one should advise distribution to have secure pam
> configuration in the first place.
Yes, I absolutely agree!
- Martin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126115/#review88597
-----------------------------------------------------------
On Nov. 19, 2015, 1:22 p.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126115/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2015, 1:22 p.m.)
>
>
> Review request for Plasma, David Edmundson and Matthias Klumpp.
>
>
> Repository: plasma-workspace
>
>
> Description
> -------
>
> 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.
>
>
> Diffs
> -----
>
> startkde/startplasmacompositor.cmake
> 1e46e5be0a0d733fb01e1a87a34ee3c73a06bf8c
>
> Diff: https://git.reviewboard.kde.org/r/126115/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Martin Gräßlin
>
>
_______________________________________________
Plasma-devel mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/plasma-devel
