https://bugzilla.suse.com/show_bug.cgi?id=1176742 https://bugzilla.suse.com/show_bug.cgi?id=1176742#c4 --- Comment #4 from Fabian Vogt <fvogt@suse.com> --- (In reply to Matthias Gerstner from comment #3)
(In reply to fvogt@suse.com from comment #1)
I had a quick look at the helper and while it prevents arbitrary arguments to be passed (the absolute path has to be below /dev), this can probably be circumvented by sending a file descriptor over dbus and passing /dev/fd/X (should be predictable) as argument.
Yes this would be possible, but it requires the attacker to open the file in advance (i.e. he needs to have certain access rights anyway). It could be combined with a symlink attack in the path that /dev/fd/X is pointing to.
Shouldn't even be necessary. open with O_PATH doesn't require privileges like that and smartctl seems to dereference the passed path anyway...
Furthermore there is a TOCTOU race condition regarding the call to `info.absoluteFilePath()` and the time the path is actually accessed by smartctl. Since /dev/shm is a world writable sticky bit directory an attacker can:
- first place a regular file in /dev/shm/myfile - call the org.kde.kded.smart.smartctl action for /dev/shm/myfile - try to win the race condition by replacing /dev/shm/myfile with a symlink at the right time - on success smartctl will dereference the symlink and arbitrary paths will become possible. - kernel symlink protection will prevent the worst
Mostly this issue could be used for file existence tests. Since smartctl only operates on block devices there shouldn't be much further damage involved.
Since we are seeing these types of weak file path checks again and again I still like to see improved code. We need to stop spreading such bad examples in OSS code.
Definitely. IIRC I've heard of a library which implements several of such primitives in a secure way, so if that's license compatible it could be used (or copied, as it's past dep freeze). I don't know where I found that anymore though, maybe you know something?
What can be done is something along the following lines:
- use realpath() to resolve all path components of the input path - require that the resolved path starts with /dev - split up the path into individual path components - open /dev with the O_PATH|O_NOFOLLOW flags and start descending down the resolved path components using openat(), O_PATH|O_NOFOLLOW.
for each path component:
- perform an fstat() and inspect the owner, file type and mode. - if the file type is other than block device or directory then an error needs to be returned. - if the file is owned by a non-root user then an error needs to be returned. - if the file is world-writable (sticky-bit directories) then an error needs to be returned.
- the final path component needs to be of the block device file type - smartctl needs to be pointed to the resolved path. No path components under attacker control should be involved.
-- You are receiving this mail because: You are on the CC list for the bug.