Bug 1220190

The package also installs a polkit file:

[   93s] drkonqi6.x86_64: E: polkit-untracked-privilege (Badness: 10000) org.kde.drkonqi.excavateFromToDirFd (no:no:auth_admin)

I will look into it. This should be the last component for KDE6 then.

Drkonqi now contains new components compared to drkonqi5. It didn't have D-Bus
or polkit components before. This needs a more careful review.

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.

So none of the findings in comment 4 is a hard blocker for whitelisting this.
Taking it all together it feels pretty shaky still though. You should
forward this to upstream so that they have a chance to improve on this.

(In reply to Matthias Gerstner from comment #4)
> - `tmpDir`, a `QTemporaryDir` is allocated on the heap for seemingly no
> reason,
>   could also be placed on the stack I guess.

Was on the heap because the lambda needs to consume it, but since it has a move constructor anyway I've switched it to the stack.

> - `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.

Agreed. However, in this case we forward the involved paths to the auth UI though, so we need them before authorization.

> - 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.

It's 0700, being a cross platform toolkit I suspect they mean that when the documentation says "QTemporaryDir is used to create unique temporary directories **safely**"

>   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 code indeed is useless. One question for my understanding though: What does a callchain look like there?

attacker: opendir()
helper: chmod()
attacker: readdir() still succeeds?

Also, do you think it makes sense to assert the permissions are indeed 0700?

> - 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.

Do you have a recommendation what to fall back to? sendfile()?

> - 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.

Suggestions?

Thanks for the review!

(In reply to sitter@kde.org from comment #6)
> > - `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.
> 
> Agreed. However, in this case we forward the involved paths to the auth UI though, so we need them before authorization.

Since the temporary directory is under full control of the privileged helper
service and actually an implementation detail, I wouldn't even display it to
the user. I don't see how this information should influence the decision to
authorize this or not.

The coredump name requested from coredumpctl is relevant though, it should
match the one that the interactive user requested, and this is something a
user can base its decision on.

> > - 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.
> 
> It's 0700, being a cross platform toolkit I suspect they mean that when the documentation says "QTemporaryDir is used to create unique temporary directories **safely**"

I would rather say this refers to the process of the creation of the
directory, because it is uniquely named and won't clash with other directory
in a shared /tmp. But that is refers to the permissions the directory ends up
with is a bit far fetched I'd say.

> >   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 code indeed is useless. One question for my understanding though: What does a callchain look like there?
> 
> attacker: opendir()
> helper: chmod()
> attacker: readdir() still succeeds?

On the fly I'm not sure if readdir() on an already opened directory FD will
still succeed when the underlying permissions change. I believe it will.
What will likely not succeed is opening a file within the directory using
openat() on the FD after the permissions changed. So it's likely not really an
attack vector. But it's also not really clean.

> Also, do you think it makes sense to assert the permissions are indeed 0700?

If we'd be using `mkdtemp()` here then I'd say we shouldn't start mistrusting
the C library. Since Qt does not clearly state what it does such an assertion
could be justified. It's a bit over the top maybe,

I just find it sad that Qt is so underspecified in its documentation. These
uncertainties can pile up. In some corners it is even a lack of safe APIs that
have lead to security issues. That is why I'm wary about it.

> > - 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.
> 
> Do you have a recommendation what to fall back to? sendfile()?

If `rename()` is not possible then an explicit copy has to happen in some way.
So the target file has to be explicitly created and the data has to be
transferred.

`sendfile()` is an efficient method to do the transferring part, yes. The most
flexible system call in this area on Linux is `copy_file_range()` I believe.
`sendfile()` has some limitations.

If you implement this then please be careful about the `openat()` part to
create the new file beneath the user supplied dirfd. You need to pass flags
such that symlinks are not followed and alike.

If I'm not mistaken then the Qt library offers some facility to transparently
try a rename() and fall back to something like `sendfile()` if that doesn't
work. But this is just one of these areas where the library doesn't offer
sufficient control over the symlink following behaviour so I wouldn't
recommend it for use in the privileged helper.

> > - the name of the function `excavateFromToDirFD()` sounds strange, doesn't
> >   really convey the meaning of what it does very well.
> 
> Suggestions?

Why not name it with the D-Bus API in mind. What does the D-Bus caller want?
It wants to save a core dump somewhere. So something along the lines of
`saveCoreToDir()` would be fitting I guess.

> Thanks for the review!

You're welcome.

Will you provide another version for review or should I whitelisting the
existing package already? A possible change of the Polkit action name would
require a follow-up whitelisting.

(In reply to Matthias Gerstner from comment #8)
> Will you provide another version for review or should I whitelisting the
> existing package already? A possible change of the Polkit action name would
> require a follow-up whitelisting.

I don't see any change upstream that would lead to a different checksum

(In reply to christophe@krop.fr from comment #9)
> (In reply to Matthias Gerstner from comment #8)
> > Will you provide another version for review or should I whitelisting the
> > existing package already? A possible change of the Polkit action name would
> > require a follow-up whitelisting.
> 
> I don't see any change upstream that would lead to a different checksum

I comment 7 we discussed a possible rename of the function
`excavateFromToDirFD()`. The Polkit action is named after this function. If
the polkit action is renamed then we need an adjusted polkit-default-privs
whitelisting.

The D-Bus parts likely won't change that is true.

According to https://invent.kde.org/plasma/drkonqi/-/merge_requests/217 the change won't be cherry-picked.

The current name will stay until Plasma 6.1 (released in 3 months)

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

The situation is still not fully clear, from the discussion in the upstream
PR#:

https://invent.kde.org/plasma/drkonqi/-/merge_requests/217

Do you intend to cherry-pick this one commit into the package? I can do it
either way. When we whitelist the old action name then an additional
whitelisting maintenance / change will be necessary when the change actually
hits the packaging.

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

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

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

We're still waiting for the upstream change to land before backporting the fixes to our drkonqi6 package

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

I just noticed that the current implementation of this service still has an
issue: the renameat() is performed as root, but the unprivileged user can also
pass on file descriptors for directories it doesn't own like /etc. Thus the
caller could cause a "core" dump file to be placed anywhere in the system it
has read access for.

I just wrote this in the upstream MR#, it should be addressed before
whitelisting the Polkit action.

The upstream MR# now contains safe code without using the directory file
descriptor anymore. Basically this version can be whitelisted once the MR# is
through and the packaging updated.

I believe the code could still be simplified a lot, however, as I stated in
the MR# just now.

After another iteration in the upstream MR# we finally arrived at simple and
robust code. Once you have that merged upstream and packaged for openSUSE we
can go ahead with the whitelisting.

Any news here about the state of our packaging?

(In reply to Matthias Gerstner from comment #22)
> Any news here about the state of our packaging?

The changes are still not merged upstream

(In reply to Christophe Marin from comment #23)
> (In reply to Matthias Gerstner from comment #22)
> > Any news here about the state of our packaging?
> 
> The changes are still not merged upstream

The MR is now merged. Our unstable package has the changes:
https://build.opensuse.org/package/show/KDE:Unstable:Frameworks/drkonqi6

They'll arrive in KDE:Frameworks/drkonqi6 with a future plasma release.

Good to hear. Please ping us once more when the stable devel package has got
the proper version. Then we'll finish the whitelisting.

(In reply to Matthias Gerstner from comment #25)
> Good to hear. Please ping us once more when the stable devel package has got
> the proper version. Then we'll finish the whitelisting.

Not in the devel prj just yet, but a home: branch has it: https://build.opensuse.org/package/show/home:Vogtinator:plasma6/drkonqi6

(In reply to fvogt@suse.com from comment #26)
> (In reply to Matthias Gerstner from comment #25)
> > Good to hear. Please ping us once more when the stable devel package has got
> > the proper version. Then we'll finish the whitelisting.
> 
> Not in the devel prj just yet, but a home: branch has it: https://build.opensuse.org/package/show/home:Vogtinator:plasma6/drkonqi6

I'll get back to it once it is in devel.

(In reply to Matthias Gerstner from comment #27)
> (In reply to fvogt@suse.com from comment #26)
> > (In reply to Matthias Gerstner from comment #25)
> > > Good to hear. Please ping us once more when the stable devel package has got
> > > the proper version. Then we'll finish the whitelisting.
> > 
> > Not in the devel prj just yet, but a home: branch has it: https://build.opensuse.org/package/show/home:Vogtinator:plasma6/drkonqi6
> 
> I'll get back to it once it is in devel.

It is, got accepted yesterday.

I just stumbled over the /usr/bin/installdbgsymbols.sh script which is also
installed by the drkonqi package. It uses an unsafe predictable /tmp path for
a FIFO in an unsafe way.

I created the following upstream issues for this:

https://invent.kde.org/plasma/drkonqi/-/issues/5

The polkit helper code that landed in the devel project looks good now. I will
finish the whitelisting for drkonqi.

The D-Bus whitelisting was already done with the KDE6 batch and still seems to
be valid. Thus only the Polkit whitelisting remains. I started the process.

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

the new drkonqi6 has now made it into Factory, closing this bug as fixed