Bug 1170161 - AUDIT-FIND: enlightenment: enlightenment_system: _store_mount_verify() follows symlinks in /media/$user
AUDIT-FIND: enlightenment: enlightenment_system: _store_mount_verify() follow...
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: Security
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:22 UTC by Matthias Gerstner
Modified: 2022-07-05 10:56 UTC (History)
2 users (show)

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

patch to harden _mkdir() (882 bytes, text/x-diff)
2020-05-22 09:51 UTC, Matthias Gerstner

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

This function rejects relative path components in the target mount path,
unlike its sibbling function described in b). However, it is unaware of
symlinks. Furthermore it makes sure that /media/$user and /media/$user/sub are
existing and are owned by the $uid:$gid of the unprivileged user.

- by placing a symlink in /media/$user/sub the setuid-root binary can be
  tricked to create attacker owned directories in arbitrary locations.
  This can quite likely lead to full root access by creating user owned
  directories e.g. beneath /etc that are then used by other privileged
- if the attacker wins a race condition he can also cause the setuid-root
  binary to pass ownership of arbitrary existing directories to himself. The
  `_store_mount_verify()` functions performs a single `stat()` call on the
  target mount path. Only if it exists and is not owned by the unprivileged
  user, the execution is aborted. Therefore if the attacker places a suitable
  symlink in the target path just after this `stat()` is performed by the
  setuid-root binary, the following `_mkdir()` invocation will `mkdir()` and
  `chown()` the path components nonetheless. This allows full root system
  access by gaining ownership of e.g. /etc or /root.

To fix this I suggest not to pass ownership of /media/$user or of any
sub-directories to the unprivileged user. If /media/$user is user controlled
then the mount operation should be rejected.
Comment 1 Simon Lees 2020-04-22 10:49:18 UTC
Upstream: https://phab.enlightenment.org/T8670
Comment 2 Simon Lees 2020-04-23 07:20:51 UTC
Upstream Fix: https://phab.enlightenment.org/rE0c79c6317b92d9a5dd2548bb34625c6ed03018d7 

enlightenment is not actually using this codepath yet.
Comment 3 Matthias Gerstner 2020-04-30 10:09:36 UTC
The upstream fix should deal with the problem at hand. I still need to check
what happens with symlinks.

Now the code performs a chown to 0:0 for the /media and /media/$user
directory. It stills seems a bit forced, could potentially clash with other
mount managers. But there's a note that they think about using /run/media
Comment 4 Matthias Gerstner 2020-05-22 09:51:10 UTC
As I said before this logic looks still a bit shaky. The `_mkdir()` function
still performs a `chown()`, where a `lchown()` would be more on the safe side.

Theoretically, if another mount manager creates user owned /media/$USER
directories, then the symlink attack would still basically work at least one
time. A user could then place /media/$USER/link -> /etc. enlightenment_system
would perform:

mkdir /media; chown 0:0 /media
mkdir /media/$USER; chown 0:0 /media/$USER
mkdir /media/$USER/link; chown 0:0 /media/$USER/link

Since the chown is now towards 0:0 the attack isn't that useful any more.

But a mount over a symlink could still cause trouble.
Comment 5 Matthias Gerstner 2020-05-22 09:51:45 UTC
Created attachment 838069 [details]
patch to harden _mkdir()
Comment 6 Matthias Gerstner 2020-05-22 09:53:42 UTC
In attachment 838069 [details] you can find a suggested patch that would make me sleep
better regarding this. Can you approach upstream with this?
Comment 7 Matthias Gerstner 2020-05-22 09:54:33 UTC
Sorry I didn't mean to close this bug just yet.
Comment 8 Matthias Gerstner 2020-07-08 11:49:15 UTC
Could I please get an update about this? Can you add the patch?
Comment 10 Matthias Gerstner 2022-07-05 10:56:13 UTC
Thanks for addressing this. With the code removed the mkdir patch is not
necessary. Closing as fixed.