Comment # 6 on bug 1220190 from Harald Sitter

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