Bug 1170163 - AUDIT-FIND: enlightenment: enlightenment_system: `_store_device_verify()` limitations are insufficient
AUDIT-FIND: enlightenment: enlightenment_system: `_store_device_verify()` lim...
Status: RESOLVED FIXED
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: Security
Current
Other Other
: P5 - None : Normal (vote)
: ---
Assigned To: Simon Lees
E-mail List
:
Depends on:
Blocks: 1169238
  Show dependency treegraph
 
Reported: 2020-04-22 09:27 UTC by Matthias Gerstner
Modified: 2020-05-22 10:24 UTC (History)
2 users (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Gerstner 2020-04-22 09:27:13 UTC
+++ This bug was initially created as a clone of Bug #1169238


This function tries to make sure that the source device path argument for block
device operations is only within the confines of the /dev directory. To do so
a lot of special characters are rejected as well as relative path components
"/..". It fails to consider symlinks, however:

- The /dev/fd directory on Linux is a symlink to /proc/self/fd. Therefore an
  already open file descriptor can be used as device argument. Open files are
  inherited from a potential attacker's context into the setuid-root context,
  therefore this can be used to circumvent the limitation. A prerequisite is
  that the attacker needs to have necessary privileges to open a file
  descriptor for the source file/device.

- The /dev/shm directory on Linux is a world-writeable sticky-bit directory.
  Therefore an unprivileged user can place symlinks in this directory.
  `_store_device_verify()` will not reject such paths. Such a symlink attack
  only works if the kernel symlink protection is off, however. Or if the
  attacker wins a race condition, because `_store_device_verify()` performs a
  `stat()` on the path and only aborts execution if the file can't be accessed.
  So an attacker could first place a regular file in there and after the
  `stat()` is performed it can be replaced by a symlink. The setuid-root
  program will then pass the path to the symlink when invoking child programs
  e.g. an `eject /dev/shm/test` which points to /dev/sr0 worked for me.

To fix this, these two cases can be rejected by the function's logic. But in
general it's difficult to filter out bad paths like this. To make it safer the
path components would need to be opened step by step and each component would
need to be checked against symlinks or otherwise user controlled content.
This can be achieved by using system calls like `openat()` and `fstat()`
starting from the root path component.
Comment 1 Simon Lees 2020-04-22 10:52:02 UTC
Upstream: https://phab.enlightenment.org/T8672
Comment 2 Simon Lees 2020-04-23 07:31:16 UTC
raster added a comment

symlinks here would not be an issue as it is only files in /dev .... all owned by root ... or should be. this function is actually incomplete because i have to verify it's a block type device AND that it's not already mounted AND it's removable and... you get the idea. - i haven't added limits of which device nodes can be mounted, just that it's a device of some sort. i'll just have this always fail for now thus effectively stopping the whole storage thing from working. remember this was being built as a fallback from udisks... so bsd's etc. can work... it's a necessary evil. ultimately this will actually be ifdefed out on linux and i'll back it onto udisks. i just was going to implement a generic udisksless version first so it can work everywhere then use udisks if i can.

https://phab.enlightenment.org/rEd8130a741c819d96e5b1c25a5fc28842bed3d77b
Comment 3 Matthias Gerstner 2020-04-30 12:27:51 UTC
(In reply to simonf.lees@suse.com from comment #2)
> raster added a comment
> 
> symlinks here would not be an issue as it is only files in /dev .... all
> owned by root ... or should be.

This is only true if paths deeper below /dev would be rejected which is
currently not the case. Therefore what I said in the initial report remains
true: /dev/fd/... and /dev/shm/... are user controlled.

> remember this was being built as a fallback from udisks... so bsd's etc. can
> work... it's a necessary evil. ultimately this will actually be ifdefed out
> on linux and i'll back it onto udisks. i just was going to implement a
> generic udisksless version first so it can work everywhere then use udisks
> if i can.

It's fine if this is only a fallback. But in the current code on Linux the
logic is reachable and therefore an attack surface is present.

The upstream commit disables this so it's fine for the moment.
Comment 4 Matthias Gerstner 2020-05-22 10:24:31 UTC
I'm closing this bug for now. But please if you ever enable this storage
subsystem request another review with us. This mount manager logic is very
fragile.