I will start the review.

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

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.

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.

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

(In reply to Fabian Vogt from comment #4)
> 
> 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?
> 

You may be thinking of https://github.com/openSUSE/libpathrs

(In reply to fvogt@suse.com from comment #4)
> (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...

This is only partly valid. For opening with O_PATH you still need execute
permission on the involved directories.

> 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?

Yes the library that Malte mentioned probably. I'm not sure if it is
production ready yet, though.

Since the KDE seems to have a lot of uses cases of this kind it could also be
a solution to provide this path security check algorithm in some suitable
framework library. Most of the time what is needed is simply a path that is
known not to be under control of anybody else but root.

Thinking some more about this, a simpler approach could be to implement a
whitelist of paths that are accepted. I could imagine something like:

- block devices directly in /dev
- anything below /dev/disk

Virtual block devices like from LVM or dmcrypt should not be needed here,
since they shouldn't deliver SMART data anyways.

We have contacted upstream. I will forward a mail to the security team with the developer in CC, with also a potential patch on their side to address the issue.

Downgrading to AUDIT-1. Waiting for new code drop. There's a separate
discussion about the fix with upstream running by e-mail.

Upstream has merged the change (https://invent.kde.org/plasma/plasma-disks/-/merge_requests/5) and we ship it in the packages which are under review at this point.

Is there anything else left to do, or can this be whitelisted now? Thanks.

(In reply to lbeltrame@kde.org from comment #10)
> Is there anything else left to do, or can this be whitelisted now? Thanks.

I started the whitelisting process. Thanks for working on the issue.

Is there a SR or PR for tracking progress?
Some users asked why plasma5-disks is missing in TW and I could only link this report.

(In reply to fabian@ritter-vogt.de from comment #12)
> Is there a SR or PR for tracking progress?
> Some users asked why plasma5-disks is missing in TW and I could only link this report.

Hm for some reason the submission bot didn't post a comment about sr#841162 /
sr#841629 regarding the D-Bus whitelisting in rpmlint.

For some other reason I seem to have forgotten to actually update
polkit-default-privs which I did just now, is on the way to Factory via
sr#842553.

This is an autogenerated message for OBS integration:
This bug (1176742) was mentioned in
https://build.opensuse.org/request/show/842553 Factory / polkit-default-privs

The whitelisting should be through by now.