Bugzilla – Bug 1170163
AUDIT-FIND: enlightenment: enlightenment_system: `_store_device_verify()` limitations are insufficient
Last modified: 2020-05-22 10:24:31 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.
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.
(In reply to email@example.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.
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