[Bug 1220190] New: AUDIT: drkonqi6
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190 Bug ID: 1220190 Summary: AUDIT: drkonqi6 Classification: openSUSE Product: openSUSE Tumbleweed Version: Current Hardware: Other OS: Other Status: NEW Severity: Normal Priority: P5 - None Component: Security Assignee: security-team@suse.de Reporter: christophe@krop.fr QA Contact: qa-bugs@suse.de Target Milestone: --- Found By: --- Blocker: --- drkonqi is the crash handler for KDE applications. drkonqi6 will coexist in openSUSE for a while and needs to be reviewed: [ 93s] drkonqi6.x86_64: E: dbus-file-unauthorized (Badness: 10000) /usr/share/dbus-1/system.d/org.kde.drkonqi.conf (sha256 file digest default filter:233b3b98de2fc6b780c1eb9c3ab9b9f99b1949dfab951cababd7c5a717b8ee16 shell filter:233b3b98de2fc6b780c1eb9c3ab9b9f99b1949dfab951cababd7c5a717b8ee16 xml filter:3326c5271c6321c5b3a703d187443eef260d54aa7347bc8f05d9090a153a04bd) [ 93s] drkonqi6.x86_64: E: dbus-file-unauthorized (Badness: 10000) /usr/share/dbus-1/system-services/org.kde.drkonqi.service (sha256 file digest default filter:b99a6dc5943df0e641616dc4be3122fdd1d1b1c418df298c56008b50c5fb2b08 shell filter:b6f2a505903e2d8874d7da96a197944ab4a11f351f3db80ffeeec86d9eefd79a xml filter:<failed-to-calculate>) Package sources: https://build.opensuse.org/package/show/KDE:Frameworks/drkonqi6 -- You are receiving this mail because: You are on the CC list for the bug.
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
Christophe Marin
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c1
--- Comment #1 from Christophe Marin
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
Wolfgang Frisch
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c2
Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c3
--- Comment #3 from Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c4
--- Comment #4 from Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c5
Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c6
Harald Sitter
- `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.
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c7
--- Comment #7 from Matthias Gerstner
- `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. -- You are receiving this mail because: You are on the CC list for the bug.
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c8
--- Comment #8 from Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c9
--- Comment #9 from Christophe Marin
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 -- You are receiving this mail because: You are on the CC list for the bug.
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c10
--- Comment #10 from Matthias Gerstner
(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. -- You are receiving this mail because: You are on the CC list for the bug.
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c11
--- Comment #11 from Christophe Marin
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c13
--- Comment #13 from Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c17
--- Comment #17 from Christophe Marin
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c19
--- Comment #19 from Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c20
--- Comment #20 from Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c21
Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c22
--- Comment #22 from Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c23
--- Comment #23 from Christophe Marin
Any news here about the state of our packaging?
The changes are still not merged upstream -- You are receiving this mail because: You are on the CC list for the bug.
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c24
--- Comment #24 from Christophe Marin
(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. -- You are receiving this mail because: You are on the CC list for the bug.
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c25
--- Comment #25 from Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c26
Fabian Vogt
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 -- You are receiving this mail because: You are on the CC list for the bug.
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c27
--- Comment #27 from Matthias Gerstner
(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. -- You are receiving this mail because: You are on the CC list for the bug.
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c29
--- Comment #29 from Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c30
Matthias Gerstner
![](https://seccdn.libravatar.org/avatar/a895f78a81a109471893519443e4d933.jpg?s=120&d=mm&r=g)
https://bugzilla.suse.com/show_bug.cgi?id=1220190
https://bugzilla.suse.com/show_bug.cgi?id=1220190#c31
--- Comment #31 from Matthias Gerstner
participants (1)
-
bugzilla_noreply@suse.com