Page MenuHomePhabricator

enlightenment_system: _store_mount_verify() follows symlinks in /media/$user
Closed, ResolvedPublic

Description

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 programs.
  • 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.

http://bugzilla.suse.com/show_bug.cgi?id=1170161

simotek created this task.Apr 22 2020, 3:48 AM
simotek triaged this task as High priority.
simotek updated the task description. (Show Details)Apr 22 2020, 4:01 AM

now we get to code i was working on to replace the old mount wrapper code we had in enlightenment_sys ... but i found i can't use it without some rewrites of stuff. i was actually going off an old legacy structure and all these dirs should be root owned. as i hadn't used this yet it was left untested/finished. in fact it should all be root.root so i've changed that. also checked for any shell meta chars in the path. we should use /run/media actually... but for now it doesn't matter.

also moving to root only no parts of the path should be user owned unless root has messed with the system and made /media or /media/user or /media/user/dir owned by a user.

Here is another patch from the SUSE Security team that will help make things better again (sorry arc isn't setup here)

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

diff --git a/src/bin/system/e_system_storage.c b/src/bin/system/e_system_storage.c
index c243d276e..7fd67bf6b 100644
--- a/src/bin/system/e_system_storage.c
+++ b/src/bin/system/e_system_storage.c
@@ -78,13 +78,19 @@ _mkdir(const char *path, uid_t u, gid_t g)
           {
              struct stat st;
 
-             if (stat(path, &st) == 0)
+             if (lstat(path, &st) == 0)
                {
                   if (!S_ISDIR(st.st_mode))
                     {
                        ERR("Path is not a dir [%s]\n", path);
                        return EINA_FALSE;
                     }
+                  else if (st.st_uid != u || st.st_gid != g)
+                    {
+                       ERR("Path already exists with unsafe permissions [%s]\n", path);
+                       return EINA_FALSE;
+                    }
+
                }
           }
      }

ummm did you see the comments in _store_mount_verify()?

// /media <- owned by root
p = strchr(tmnt + 1, '/');
if (!p) goto malformed;
*p = '\0';
if (!_mkdir(tmnt, 0, 0)) goto err;
*p = '/';

// /media/username <- owned by root
p = strchr(p + 1, '/');
if (!p) goto malformed;
*p = '\0';
pp = strrchr(tmnt, '/');
if (!pp) goto err;
// check if dir name is name of user...
if (strcmp(p + 1, user_name)) goto err;
if (!_mkdir(tmnt, 0, 0)) goto err;
*p = '/';

// /media/username/dirname <- owned by root
if (!_mkdir(tmnt, 0, 0)) goto err;

so /media, /media/user and /media/user/dirname are all root owned so the user can't do what you say they can... :) these paths should not already exist on the system or should already be owned by root so we're not walking into a pre-broken system... unless pre-broken systems are out there?