Comment # 7 on bug 1220190 from Matthias Gerstner

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