(In reply to fabian@ritter-vogt.de from comment #0)
> kcm_sddm got some more features which needed some additional methods in its kauth helper.

Okay we will will schedule the review.

> https://build.opensuse.org/package/live_build_log/KDE:Unstable:Frameworks/kcm_sddm/openSUSE_Factory/x86_64
> 
> I submitted a patch upstream to make that auth_admin already: https://phabricator.kde.org/D23078

Is there some special reason for that? auth_admin_keep is not a bad choice by
default. It only allows the authenticated process to repeat the polkit action
at a later time. A different process will need to authenticate again. It is
usually chosen for situations like e.g. wifi scanning, an operation that is
often used multiple times repeatedly.

I didn't look into these specific polkit actions yet, that's why I'm asking.

I'll start working on this review.

Well this kauth helper is written in a style we've seen before with other KDE
auth components. Meaning that it largely oblivious to the implications of
accessing user owned files as root.

I have the following remarks:

a) if no sddm user is existing in the system then the result of
  `KUser("sddm").homeDir()` is an empty string. This error condition is not
  checked. This results in the service creating a directory "/.config" in this
  situation e.g. when calling the `syncSettingsChanged()` method.

  The client application passes the parameter "sddmUserConfig" to the backend,
  supposedly pointing to `$SDDM_HOME/.config` here. I think this evaluation of
  the sddm home directory should be solely made on the backend side and the
  error condition needs to be catched.

b) the auth helper creates the directory pointed to by the "sddmUserConfig"
  parameter, if it is not existing. This means the "sddm" home directory might
  be created by this auth helper using the default umask (i.e. it will be world
  readable). Is it really this auth helpers job to create this directory? And
  if so, shouldn't it be better protected?

c) SdmAutHelper::save() and SdmAuthHelper::installtheme() both access
  potentially user owned files (background files or theme archive files)
  without necessary protection. For example if a user downloads an image to
  /tmp then there is a danger that another user might play tricks with
  symlinks. Without `auth_admin` this would be completely unsafe.

  The only safe way to implement this would again be to drop privileges to the
  client user and safely copy the file away to some private location only
  accessible to root.

d) The README file in the repository is quite meaningless (it seems to contain
  build instructions). Installing it as part of the package makes no sense.
  Somebody could try to help upstream to add some actual documentation in
  there. This is a common theme with KDE packages that the documentation is
  lacking and this is sad.

For whitelisting this at least a) needs to be fixed. b) also looks worthy of a
fix to me. c) can be skipped given that this is only for auth_admin but it
still leaves a bad taste and at some stage KDE upstream needs to be able to
come up with more secure kauth helper implementations by default. d) is not
security related.

I forgot one more remark:

e) The API for SddmAuthHelper::uninstalltheme is unsuitably complex. It takes
  a full path to the theme to uninstall and then the implementation tries hard
  to make sure this full path exactly points to a theme folder below
  /usr/share/sddm/themes. So the sane thing to do here would be to only expect
  the theme name to be removed, not a path in the first place.

I've implemented our new audit bug lifecycle [1] in this audit. This bug here
now remains a tracker for the findings that resulted from the audit. I only
selected the findings actually blocking the whitelisting but you should
consider the other remarks, too.

[1]: https://en.opensuse.org/openSUSE:Package_security_guidelines#Audit_Bug_Lifecycle

Both linked bugs got fixed in v5.17.0 a while ago, so can this be whitelisted now?

The hard requirements are fulfilled. Remarks c/d/e remain unadressed. It would
have been nice to see some more efforts to come to a clean solution. But so be
it then.

I will submit a whitelisting shortly.

(In reply to Matthias Gerstner from comment #7)
> The hard requirements are fulfilled. Remarks c/d/e remain unadressed. It
> would
> have been nice to see some more efforts to come to a clean solution. But so
> be
> it then.

Those would be changes more suited for the next major release, not a simple bugfix one. Upstream is aware of those issues.

This is an autogenerated message for OBS integration:
This bug (1145182) was mentioned in
https://build.opensuse.org/request/show/749492 Factory / polkit-default-privs

The Whitelisting is submitted to Factory. This should finish this bug.

(In reply to Matthias Gerstner from comment #10)
> The Whitelisting is submitted to Factory. This should finish this bug.

Please also submit it to Leap 15.2, kcm_sddm needs to be updated there as well.

(In reply to fvogt@suse.com from comment #11)
> Please also submit it to Leap 15.2, kcm_sddm needs to be updated there as well.

The whitelisting *should* travel from Factory to SLE-15-SP2:GA to Leap:15.2.
If you need it more quickly I can try to explicitly submit an update to
Leap:15.2. But we want to fork polkit-default-privs for Leap:15.2 and need to
coordinate this still, so it would be a bit confusing at the moment.