Page MenuHomePhabricator

enlightenment_system: `_store_device_verify()` limitations are insufficient
Closed, ResolvedPublic


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.

simotek created this task.Apr 22 2020, 3:51 AM
simotek triaged this task as High priority.
raster reopened this task as Open.Apr 22 2020, 6:28 AM

oops - i tagged the wrong task with this.

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.