Comment # 4 on bug 1176742 from Fabian Vogt

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