I'll looking into this as well.

The shape of this service hasn't improved much over the last larger audit we
did in bug 1145182.

security aspects
----------------

- this service runs as root but at least in the `sync()` and `reset()`
  functions acts on behalf of the `sddm` service user. These functions place
  files into the `sddm` controlled home directory. A compromised `sddm` user
  might be able to use this for symlink attacks or DoS attacks. The helper
  should drop privileges to `sddm` for the functions that require this.

- `openConfig()` likely follows symlinks in the path provided by the D-Bus
  client. This could allow to make arbitrary paths in the system
  world-readable.

- `save()` removes a client provided "background path", remove logic that is
  also existing in some other spots. In this case it could be used to remove
  arbitrary paths in the system.

- `installtheme()` accepts a path to a tar or zip archive and extracts it into
  the system theme dir. Since archives can contain relative pathnames it is no
  unlinkely that this could be used to create arbitrary files in arbitrary
  locations.

minor findings
--------------

- the `// initial check for sddm user; abort if user not present` is duplicate
  in the `sync()` and `reset()` functions. It should be moved into a small
  helper function.

summary
-------

we have two major problem classes here:

- the helper service running as `root` operates in `sddm`s home directory,
  thus a compromised sddm user account could exploit this to achieve DoS,
  integrity violation or even privilege escalation.

- the helper service accepts all kind of path names from D-Bus clients. All
  current actions need `auth_admin` authentication so it could be argued that
  they are equivalent to root. It still means that lowering these
  authentication requirements for any of these actions would be dangerous and
  create security issues. This would not be necessary in a cleanly designed
  helper. One major improvement would be not accepting path names but
  passed-on file descriptors from clients. But I'm not even sure if this is
  currently possible with the KAuth design. Or rather I'm pretty sure it's not
  possible. I will mention this in the kauth upstream ticket for kauth as
  well.

Overall this kauth helper is in a bad shape security wise and should receive
some improvements for the KDE6 release.

Christophe, I'm done with this package as well. See comment 2. This is one of
the cases where some improvements by upstream would be very welcome.

(In reply to Matthias Gerstner from comment #3)
> Christophe, I'm done with this package as well. See comment 2. This is one of
> the cases where some improvements by upstream would be very welcome.

Thanks, email sent to security@kde.org for this one as it deserves more attention.

I think that if we want to make this actionable, we need to narrow down the issues we have with this.

WRT openConfig for example, it's always opening files that are well-known at compile-time. There's some cases that it's true that the path is passed through dbus but it's reasonably addressable.

WRT lowering privileges to sddm user, it would lose the ability to read files from the user directory which seems to be big part of what we are doing here.

On installtheme, how would that happen? is the concern that the zip/tar files would escape its extraction directory? It could make sense to address this at KArchive level.

Also if you know of a similar helper that does these properly, it would be good for context.

Hi,

(In reply to aleixpol@kde.org from comment #5)
> I think that if we want to make this actionable, we need to narrow down the issues we have with this.

I tried to do this in comment 2.

> WRT openConfig for example, it's always opening files that are well-known at compile-time. There's some cases that it's true that the path is passed through dbus but it's reasonably addressable.

The D-Bus interface takes client provided paths to open using `openConfig()`.
This is happening in the following places:

- `SddmAuthHelper::save()` in sddmauthhelper.cpp line 272
- `SddmAuthHelper::sync()` in sddmauthhelper.cpp line 177 and line 178
- `SddmAuthHelper::reset()` in sddmauthhelper.cpp line 231 and line 232

The `openConfig()` function performs the equivalent of a `chown 644` on the
passed path, if it already exists and has a different mode. This means if a
client passes e.g. /etc/shadow as a path then it will be world-readable
afterwards.

It even creates new directories and files, if necessary, so I could pass
/etc/sudoers.d/my-sudoers and this would come into existence. Not writable for
non-root but it is still very unexpected.

This is operating outside of the promise these D-Bus functions should give:
that they restrict themselves to operate on the sddm configuration and
nothing else.

I also think it is addressable, but these things can be tricky to get right if
not done carefully.

> WRT lowering privileges to sddm user, it would lose the ability to read files from the user directory which seems to be big part of what we are doing here.

The service doesn't need to permanently lower privileges. It can temporarily
lower privileges to both, the sddm user and the calling (client) user to
safely access user provided paths or write to user-owned directories. This can
be done using `setregid()` & friends.

For client provided paths the elegant way would be to pass already open file
descriptors, though, as I outlined in previous comments. It seems that the
KAuth framework is currently a limitation here, so maybe you can chime in (see
review bug 1217178) and try to find a design for kauth that makes this
possible in the future.

> On installtheme, how would that happen? is the concern that the zip/tar files would escape its extraction directory? It could make sense to address this at KArchive level.

I didn't test it but it is usually trivial to do that unless a lot of
precautions are made. It could be addressed at the KArchive level, but such
libraries aren't usually designed for situations where untrusted data is
processed by root.

I'm not sure what the simplest route would be to make this robust. Often
programs resort to isolation techniques like mount namespaces to add a
foolproof layer of security. But this is rather complex, naturally, and also
platform specific.

The current code in `SddmAuthHelper::installtheme()` only checks the root
level entries of the archive, if they are directories and if they contain
"metadata.desktop" files. That's not much with regards to verification of the
structure that's passed here.

> Also if you know of a similar helper that does these properly, it would be good for context.

I cannot think of any shining example for this level of complexity just at the
moment. There is at least a rather recent example in Passim:

    https://github.com/hughsie/passim/blob/main/src/passim-server.c#L1125

This spot shows how file descriptors are passed from client to service to
avoid all the trouble with opening client provided paths.

There seem to be no news about the state of this.

The to-be-submitted package is now in KDE:Frameworks/sddm-kcm6 and uses
version v.5.93.0.

Nothing in the code logic actually changed since I did the review in early
December.

So what are your plans to continue with this?

I can reluctantly whitelist this as a legacy, but I'd like to see some path
forward to get this problematic code out of the way in the future.

We are looking to find someone who can spend time on these issues.

This is the last KDE6 component that is waiting for a whitelisting. I'll
whitelist it although it is pretty ugly given that there's an sddm to root
attack vector in there ... Strictly spoked this is CVE material, a change of
security scope.

I'm submitting the whitelisting. I've split off a separate bug 1221041 to
track the security concerns.

I assume the software to be whitelisted is now found in KDE:Frameworks/sddm-kcm6

This is an autogenerated message for OBS integration:
This bug (1217188) was mentioned in
https://build.opensuse.org/request/show/1155534 Factory / rpmlint

This is an autogenerated message for OBS integration:
This bug (1217188) was mentioned in
https://build.opensuse.org/request/show/1156346 Factory / rpmlint

This is an autogenerated message for OBS integration:
This bug (1217188) was mentioned in
https://build.opensuse.org/request/show/1156898 Factory / rpmlint

This is an autogenerated message for OBS integration:
This bug (1217188) was mentioned in
https://build.opensuse.org/request/show/1156938 Factory / rpmlint

*** Bug 1221396 has been marked as a duplicate of this bug. ***

the whitelisting is in Factory now

*** Bug 1221474 has been marked as a duplicate of this bug. ***

(In reply to Matthias Gerstner from comment #9)
> This is the last KDE6 component that is waiting for a whitelisting. I'll
> whitelist it although it is pretty ugly given that there's an sddm to root
> attack vector in there ... Strictly spoked this is CVE material, a change of
> security scope.

FTR, the plan we had for 6.0 was to not submit it to TW for now until the affected functionality was rewritten or the issues addressed some other way, but for $reasons it ended up in TW anyway.

For now I made a patch to disable third-party theme installation, which means that all actions only work on data explicitly supplied by the user: https://build.opensuse.org/request/show/1159795