Use seccomp for implementing a sandbox for kscreenlocker_greet

Authored by graesslin on Mar 12 2017, 4:17 PM.



This change introduces a new optional dependency on libseccomp.
Libseccomp allows to forbid syscalls. With that we can constrain the
user defined dynamically loaded QtQuick code from the look'n'feel
package and from the wallpaper package. The idea is to protect against
"malicious" packages the user manually installed.

With the installed seccomp filter we can ensure that the QtQuick code
cannot perform the following operations:

  • send password into Internet through forbidding the socket syscall
  • use KIO to send password into Internet through forbidding fork+exec
  • write password into a file through forbidding opening a file in write mode or creating a new file
  • send password to another process through forbidding pipe/pipe2

So far our QtQuick code was already constrained by disallowing network
access through injecting a QNetworkAccessManager which forbids internet
access. But this was easy to circumvent through e.g. KIO.

The seccomp filter cannot protect against a malicious process already
running on the system. The obvious way to get out of this sandbox is
DBus. DBus is allowed in the sandbox, thus it is possible for a malicious
look'n'feel package to communicate with a running malicious application
through DBus. To protect DBus we need to implement an additional apparmor

The seccomp filter gets only installed if the seccomp dependency is
available and kcheckpass is not setuid. This is ensured with a runtime
check. For kscreenlocker_greet the main change is that when seccomp is
enabled the delayed kcheckpass authentication method is used.

Test Plan

Manual testing and a new auto test which verifies the
restricted conditions.

Diff Detail

R133 KScreenLocker
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
graesslin created this revision.Mar 12 2017, 4:17 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 12 2017, 4:17 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript



set QNetworkRequest::FollowRedirectsAttribute or else Ben will get angry


Might lead to an unreachable code warning?

graesslin added inline comments.Mar 12 2017, 4:48 PM

The point of this test is that the call doesn't work. See line 101. The seccomp filter disallows network access. No matter whether I use FollowRedirectsAttribute or not: will never see the request.

And if not and Ben gets angry: even better, than we have a human auto test ;-)


why should it? The code looks quite reachable to me. If kcheckpass is setuid it uses the code from line 151.

broulik added inline comments.Mar 12 2017, 5:32 PM

It might fail because KDE CI infrastructure does a redirect we don't follow instead of failing because we actually restricted it.


Yeah but it will lead to code in the form of

return new Authenticator(Authenticator::AuthenticationMode::Delayed, this);
return new Authenticator(Authenticator::AuthenticationMode::Direct, this);

the latter being unreachable. Just being picky here, though, feel free to ignore.

graesslin added inline comments.Mar 12 2017, 7:43 PM

I'm pretty sure that it would fail properly for the IP variants in such a case.


That is totally fine. Having different return values in different paths is quite common. E.g. an early exit with a return nullptr.

romangg added a subscriber: romangg.Apr 1 2017, 5:33 PM
romangg added inline comments.

Why not create it explicitly? Calling supportsThreadedOpenGL to do it seems like a hack.

graesslin added inline comments.Apr 2 2017, 7:12 AM

True, it's a hack. But creating a working gl context is nothing I want to do here and is nothing I can do in a way which ensures that it will work in all cases for Qt.

This one method does exactly what we need. It creates a Gl context which works for Qt and destroys it again. Overall it means we can get rid of a few hundred lines of code.

So I personally think that this hack is justified in this case.

graesslin updated this revision to Diff 13189.Apr 7 2017, 2:01 PM

Rebased on latest changes

This revision was automatically updated to reflect the committed changes.