Comment # 4 on bug 1220190 from Matthias Gerstner

Drkonqi doesn't rely on Kauth for this new D-Bus service. One of the reasons
likely is that it uses a QDBusUnixFileDescriptor argument in the only D-Bus
method "exvacateFromToDirFd()". Kauth doesn't support that currently.

This D-Bus method calls `coredumpctl` to fetch an arbitrary coredump and store
it in a user provided directory. Doing this requires Polkit `auth_admin`
privileges.

The positive aspect about the non-kauth route is that the target directory is
represented by said QDBusUnixFileDescriptor and thus a lot of attack scenarios
don't even come into play. The unprivileged user has to open the target
directory on its own in the unprivileged context.

The negative aspect is that the Polkit authentication has to be implemented in
a custom manner. It is only about 25 lines of code though and it is correct:
It uses the SystemBusName subject; the evaluation of the result is also
correct. A positive aspect is also that the source and target parameters are
even displayed in the Polkit authentication message, something that we wanted
to get into the ktexteditor authentication prompt for years.

A few things in the code are still strange though. The helper's code is found
in src/coredump/polkit/main.cpp:

- `tmpDir`, a `QTemporaryDir` is allocated on the heap for seemingly no reason,
  could also be placed on the stack I guess.
- `QTemporaryDir` is created before `isAuthorized()` is checked which doesn't
  make much sense. If authorization fails then ideally the helper should do as
  little as possible, also not create temporary directories that aren't
  needed.
- QTemporaryDir in Qt6 docs doesn't specify which permissions the temporary
  directory will get. This is a shortcoming in Qt docs. One _should_ expect
  that it receives safe 0700 permissions as is guaranteed by `mkdtemp()` from
  libc. We cannot really know without formal documentation though.

  The code contains this:

      std::filesystem::permissions(targetDir.toStdString(),
std::filesystem::perms::owner_all, std::filesystem::perm_options::replace);

  Ideally this should be unnecessary, but if it is necessary, then there would
  be a race condition, a time window within which other users in the system
  could enter the temporary directory and possibly read the core dump that
  will later be placed there.
- The `coreFile = COREDUMP_PATH + coreName` takes an arbitrary `coreName`
  input string which may also contain directory separators. With this
  arbitrary paths can be constructed. Luckily this isn't really a path but
  rather a property match for `coredumpctl`, it will be passed as
  `COREDUMP_FILENAME=%1` to it on the command line. So it looks like nothing
  else but coredumps can be targeted using this. It would still be a good idea
  to limit this `coreName` input argument to make sure it doesn't contain "/"
or
  "/.." elements.
- The helper uses `renameat()` to move a coredump - that was previously written
  by `coredumpctl` into a temporary directory - into the user supplied
  `targetDirFd`. This only works if the temporary directory and the user
  supplied directory reside on the same file system. It's not uncommon to have
  e.g. /tmp on a separate file system. In this case the operation will likely
  fail.
- the lambda function found in `exvacateDirFromToDirFd()`  the `exitCode` is
  only evaluated at the end, when moving the coredump file has already been
  attempted. This doesn't make sense, if the exitCode is non-zero then nothing
  in that direction should be attempted in the first place.
- the name of the function `excavateFromToDirFD()` sounds strange, doesn't
  really convey the meaning of what it does very well.