https://bugzilla.suse.com/show_bug.cgi?id=1220190 https://bugzilla.suse.com/show_bug.cgi?id=1220190#c6 Harald Sitter <sitter@kde.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sitter@kde.org --- Comment #6 from Harald Sitter <sitter@kde.org> --- (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! -- You are receiving this mail because: You are on the CC list for the bug.