Bug 1217186

I'm now looking into this one.

It seems we actually never reviewed this fontinst service before. The code for
it is pretty old. I am not very happy with the D-Bus interface and the actions
implemented there.

To start off, the only exported D-Bus method, org.kde.fontinst.manage, is
actually a one-catches all method that multiplexes the following individual
function calls:

```
    QString method = args["method"].toString();

    // qDebug() << method;

    if ("install" == method) {
        result = install(args);
    } else if ("uninstall" == method) {
        result = uninstall(args);
    } else if ("move" == method) {
        result = move(args);
    } else if ("toggle" == method) {
        result = toggle(args);
    } else if ("removeFile" == method) {
        result = removeFile(args);
    } else if ("reconfigure" == method) {
        result = reconfigure();
    } else if ("saveDisabled" == method) {
        result = saveDisabled();
```

These should all be individual D-Bus methods with individual Polkit actions
associated with them. Only then will it be possible to actually benefit from
Polkit by changing authentication settings on a fine grained level.

The single manage action that we have now is set to `auth_admin_keep` for the
logged in user. Although this way only people that have admin access anyway
are able to call the method, there are a couple of worries here. A look at the
individual sub-actions:

- install(): this can be used to copy arbitrary file paths to arbitrary
  locations, the new files end up with mode 0644.
- uninstall(): this allows to remove arbitrary file paths, as long as their
  parent directories have a writable bit set.
- move(): this allows to move arbitrary paths complete with new owner uid and
  group gid to arbitrary new locations.
- toggle(): this takes raw XML that also seems to specify font paths that are
  to be enabled or disabled.
- removeFile(): does what is says on the label.
- configure(): saves modified font directories and invokes a small bash script
  `fontinst_x11` that prepares font directories and triggers a font refresh at
  the X server.

This range of freedom looks like the fontinst helper is actually the new kio
D-Bus layer that allows arbitrary file operations. It is way beyond the scope
of what a fontinst helper should do. If an administrator wants to allow
regular users to modify system fonts by lower the authentication requirements
for this Polkit action then these regular uses will easily be able to gain
full root privileges by using this D-Bus API.

I strongly recommend investing some efforts here for KDE6 with a major
interface change:

- provide separate D-Bus methods and Polkit actions for each "sub-method" we
  currently have.
- instead of providing arbitrary source paths, the D-Bus clients should pass on
  an already open file descriptor. This way it is made sure that the caller
  actually has permissions to access the file.
- instead of providing arbitrary destination paths, the helper should make sure
  that fonts are only installed in system font directories.
- instead of allowing arbitrary uid/gid ownership changes the font files
  should always be owned by root:root mode 0644 if the service is running on
  the system bus. If the service is running on the session bus then no
  ownership change will be possible anyway (since the service is running
  unprivileged).

Otherwise the code could certainly benefit from investing some love, in parts
it still uses C APIs (like `fopen()`) and it generally looks pretty
convoluted.

Here is one additional misc finding:

- in `Misc::setFilePerms()` there is a `umask()` save & restore call around a
  `chmod()` invocation. This doesn't make sense. `chmod()` doesn't even use
  the umask.

Filed https://invent.kde.org/plasma/plasma-workspace/-/issues/106

There was no movement in the upstream issue for nearly three months.

The package to be submitted is now found in KDE:Frameworks/plasma6-workspace.
The version found there is v5.93.0.

There have been a couple of changes since the review, mostly adapting to Qt6
and some new C++ features.

!!! I will whitelist this only on the grounds that it was whitelisted before
!!! (but never reviewed).

This D-Bus helper is a legacy that should no longer be part of a production
environment.

The whitelisting is in Factory now. Closing as fixed. If you want a bug to discuss the improvement of this service then please split-off a separate one.