On Wed, 22 Apr 2020 11:02:46 +0200 Matthias Gerstner <[email protected]> said:
fixed. :) > Hi, > > the SUSE security team has been asked [1] to review the new > `enlightenment_system` setuid-root binary for inclusion into openSUSE > Tumbleweed. Therefore I looked into the snapshot that our packager > provided me with. It seems to correspond to git [2] commit > b5fa86e7f5301452f4156ba62bc073f27280c798, at least with regard to the > `enlightenment_system` code itself. > > [1]: https://bugzilla.suse.com/show_bug.cgi?id=1169238 > [2]: https://git.enlightenment.org/core/enlightenment.git > > # Security Issues > > After reviewing this setuid-root binary I don't deem the current state of it > fit for production use. I have found the following individual issues: > > ## a) `_store_mount_verify()` follows symlinks in /media/$user > > This function rejects relative path components in the target mount path. > It is unaware of symlinks, however. 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 into creating 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 him. The > `_store_mount_verify()` function performs a single `stat()` call on the > target mount path. The operation is only rejected if it exists and is > not owned by the unprivileged user. 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. > > ## b) `_store_umount_verify()` does not protect against shell > metacharacters and relative path components > > This function tries to make sure that the user can only unmount his own > mounts below /media/$user. It also rejects backslashes in the path. > However it does not reject relative path components or shell characters. > > - this allows a regular user to unmount arbitrary file systems by passing > paths like "/media/$user/../../tmp. > - since the unmount is performed by calling the `umount` utility via > "/bin/sh", shell metacharacters will be interpreted. Passing a path > like '/media/testuser/$(date)' will cause the setuid-root program to > execute the `date` program as root. This leads to full code execution > as root. The only requirement is that a directory of the same name > exists. Spaces are also allowed in the path, therefore even complex > commands can be executed as root. > > I recommend to reject relative path components and shell metacharacters in > this function to fix the issue. > > ## c) `_store_device_verify()` limitations are insufficient > > This function tries to make sure that the source device path argument > for block device operations is 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. > > - The /dev/shm directory on Linux is a world-writable 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 feature > is off, however. Or if the attacker wins a race condition, because > `_store_device_verify()` performs a `stat()` on the path and only > rejects the operation 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 replaced the file 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()` using O_PATH and `fstat()` starting from the root path > component. > > ## d) `_cb_l2ping_ping()` performs an unbounded `sscanf()` on untrusted > input data, allowing a stack buffer overflow > > A `sscanf()` call in this function passes a `%s` format for the `params` > input parameter. The target buffer has a length of 1024 bytes. Thus if a > client passes a very long device name the setuid-root binary's stack > will be overwritten. The parsing by `sscanf()` stops at whitespace > characters thus the stack overflow data cannot be chosen arbitrarily. > Still it is a pretty dangerous security issue. > > ## e) `_cb_l2ping_ping()` SEGFAULTs when no parameter is passed. > > This function unconditionally dereferences the `params` string in a > `sscanf()` call, which causes a SEGFAULT by dereferencing a NULL pointer > when a user is not passing any parameter data. > > ## f) /etc/enlightenment/sysactions.conf limitations are ineffective > > There is a security mechanism implemented based on the file > /etc/enlightenment/sysactions.conf. This file defines which users/groups > are allowed to execute the `enlightenment_system` binary in the first > place. > > The `_etc_enlightenment_system_conf()` function parses this file. > However there seems to be a `while` or `for` loop body missing. Instead > only the first line of the file is ever parsed, which happens to be a > comment line by default. The logic in the function defaults to "allow > everything" if nothing else was determined. Thus this security mechanism > is currently ineffective and all users in the system can use the full > functionality of the setuid-root program. > > I suggest to deny access by default and correct the algorithm to > correctly parse the configuration file. > > ## g) setcwd() is missing and a check for "libcurl.so.5" allows arbitrary > file existence tests > > The setuid-root program does not reset the current working directory to > a safe path like "/". Therefore whichever directory the unprivileged > user sits in when invoking the setuid-root program will affect relative > path lookups performed in the setuid-root context. > > One such instance is found in the context of the call to > `ecore_file_download_init()´ which later down the path tries to load the > shared library "libcurl.so.5" via `eina_module_load()`. If the > `dlopen()` fails the latter function performs a `stat()` on the library > basename "to determine" whether the library existed or not. If it thinks > the library existed but couldn't be loaded then it prints out an error > message. > > An attacker can place a symlink named "libcurl.so.5" in the current > working directory and enlightenment_system will follow the link and > behave differently depending on whether the link target exists or not. > Therefore an attacker can test for existence of arbitrary files also in > paths usually not accessible to him. Example: > > ``` > # symlink pointing to non-existing file > user /home/user $ ln -s /root/.notexisting libcurl.so.5 > user /home/user $ /usr/lib64/enlightenment/utils/enlightenment_system > ddc-list^C > > # symlink pointing to existing file > user /home/user $ ln -s /root/.bashrc libcurl.so.5 > user /home/user $ /usr/lib64/enlightenment/utils/enlightenment_system > ERR<4165>:eina_module ../src/lib/eina/eina_module.c:319 eina_module_load() > could not dlopen("libcurl.so.5", libcurl.so.5: cannot open shared object > file: No such file or directory): RTLD_NOW [...] ``` > > I suggest to set the CWD to a defined and safe default like "/". > > Furthermore I'd adjust the code in `eina_module_load()` to use > `dlerror()` instead of guessing reasons for failed library loads. > > ## h) `_cb_stdio_in_read()`: potentially large memory allocation based > on untrusted user data > > The line `buf = malloc(head.size)` in this function uses the untrusted > size specification provided by the unprivileged user to allocate a > potentially large chunk of memory on the heap. On Linux this is mostly > uncritical, because the kernel by default overcommits memory and only > really allocates memory once it's accessed. On other OSs this could be > used to hog memory in a root process, however. > > I suggest to implement a reasonable maximum message size and reject > everything else. > > ## i) `ecore_file_app_installed()` can be tricked into returning bogus results > > Various calls to `ecore_file_app_installed()` are performed in the > context of the setuid-root binary. This function performs a direct check > for the existence of the given filename before checking the directories > found in the PATH environment variable. > > Since the CWD is controlled by a potential attacker (see g)), the > attacker can place arbitrary files named like the searched binaries in > the CWD. As a result the `ecore_file_app_installed()` will returns bogus > results. I couldn't find any way to exploit this fact in the context of > the setuid-root binary, however. > > Like in g) I suggest to set the CWD to a safe default value. > Furthermore I suggest *not* to check the CWD in > `ecore_file_app_installed()` installed. If the CWD should be checked > then the PATH environment variable should contain an entry for "." > instead. > > # Security Vulnerability Process > > I'm posting these findings here publicly since the Enlightenment project > does not document any preferred vulnerability report procedure and does > not offer a means of coordinated disclosure. I asked on the > Enlightenment freenode IRC channel about the best way to report security > issues and I was pointed towards the mailing lists and the issue > tracker. > > From my point of view at least items a), b) and d) deserve a CVE > assignment due to the severity of the issues. Even if to my knowledge > the code in question wasn't yet part of an official release yet it might > help the community to identify risks in their systems. Please tell me > whether you want to assign CVEs on your end or whether I should do this. > > # The general design of the `enlightenment_system` setuid-root program > > I would like to share my thoughts on the general design of this > setuid-root program: > > - the fact that all of the child processes are created through "/bin/sh" > is difficult for security and complicates the code a lot. By using a > mechanism based on the `execve()` system call all worries about shell > metacharacters would be gone in the first place. > - the various "filter" logic that tries to detect bad paths and alike is > error prone in nature. It is very difficult to operate as root on user > controlled paths. Lower level system calls like `openat()` are a must > to avoid symlink attacks and other surprises. Child programs like > `mount` can then be pointed to the already open files in > /proc/self/fd/, for example. This is valid at least for Linux. > - allowing to mount arbitrary block devices as is the case with the > "store-mount" command seems a bad idea to me. This should be limited > to removable block devices at best (this property can be determined > via sysfs on Linux). > - even though the setuid-root specific code is "only" about 3.000 lines > of code in size, the plethora of framework libraries pulled in make > this a surprisingly complex program for a setuid-root utility. These > libraries are largely unaware that they're operating in a security > sensitive context which can create additional security issues (like > issue g) and i)). > - the design of the program is already asynchronous in nature based on > the main loop. Furthermore there is a rather complex communication > channel established between the caller and the program based on a > special purpose binary protocol. Given this I wonder why a setuid-root > program has been chosen to realize these features in the first place. > With this level of complexity and feature set a D-Bus service with > polkit authentication on top would be the cleaner solution. > > Cheers > > Matthias > > -- > Matthias Gerstner <[email protected]> > Dipl.-Wirtsch.-Inf. (FH), Security Engineer > https://www.suse.com/security > Phone: +49 911 740 53 290 > GPG Key ID: 0x14C405C971923553 > > SUSE Software Solutions Germany GmbH > HRB 36809, AG Nürnberg > Geschäftsführer: Felix Imendörffer > -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- Carsten Haitzler - [email protected] _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
