On Wed, 22 Apr 2020 17:00:37 +0200 Stefan Schmidt <ste...@datenfreihafen.org> said:
> Hello. > > On 22.04.20 16:54, Carsten Haitzler (The Rasterman) wrote: > > On Wed, 22 Apr 2020 11:02:46 +0200 Matthias Gerstner <mgerst...@suse.de> > > said: > > > > > > fixed. :) > > And obviously a big thanks to Matthias for taking the time to analyze > and report the issues! and simotek too for pushing this along. i though the best way to respect this was to handle it asap. :) > regards > Stefan Schmidt > > > > >> 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 <matthias.gerst...@suse.de> > >> 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 > >> > > > > > > > _______________________________________________ > enlightenment-devel mailing list > enlightenment-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- Carsten Haitzler - ras...@rasterman.com _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
