For completeness of archives our tracker bug was https://phab.enlightenment.org/T8669
On 4/23/20 12:24 AM, Carsten Haitzler (The Rasterman) wrote: > 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 >> > > -- Simon Lees (Simotek) http://simotek.net Emergency Update Team keybase.io/simotek SUSE Linux Adelaide Australia, UTC+10:30 GPG Fingerprint: 5B87 DB9D 88DC F606 E489 CEC5 0922 C246 02F0 014B
signature.asc
Description: OpenPGP digital signature
_______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
