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
signature.asc
Description: PGP signature
_______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
