Hi, thanks for bringing up the potential problems with the patch we (SUSE) suggested. The missing drop of the ancillary group list has indeed been overlooked and will result in a lack of protection, since the "unprivileged" process will likely still be a member of the root group.
I will adjust the patch to contain this and one or two other adjustments and can then share it again here on the list. Please find a few more comments below inline. On Tue, Oct 08, 2024 at 10:56:59PM +0200, Solar Designer wrote: > On Tue, Oct 08, 2024 at 08:10:17AM +0200, Simon Josefsson wrote: > > I noticed that that there are Linux-PAM helpers to drop privileges: > > > > https://github.com/linux-pam/linux-pam/blob/master/libpam/pam_modutil_priv.c#L52 > > This currently switches fsuid/fsgid (so for the current thread only), > but uses initgroups() and setgroups() libc functions (so affects all > threads). Regarding thread safety, the SUSE patch forks a new process to drop the privileges, so it shouldn't be an issue here. > In particular, I worry that the SUSE approach could be susceptible to > hard link attacks (when the fs.protected_hardlinks sysctl is not set). > Would this allow to overwrite someone else's file (the original issue) > if the user can hard link that file? I currently don't see why not, so > it's probably a vulnerability. Indeed the patch does not take care of hard link attacks. Our products don't have any supported configuration without protected_hardlinks enabled, so we didn't have this in mind. The change to address this concern should be rather small, though, so I will try to incorporate it in a new version of the patch. > In general, switching to a user not only drops privileges for file > access, but also potentially exposes the process as that user's. > fsuid/fsgid switching is the safest in this respect (these were meant > just for file access purposes), but with other IDs (depending on which) > there may be extra exposure of the partially privileged log in process > to the user via /proc, kill(), setpriority(), etc. ... but thankfully > and hopefully not also via ptrace() on modern systems anymore. On modern Linux there shouldn't be a problem with dropping UID/GID, as the kernel will set the process's suid_dumpable attribute to the setting found in sys.fs.suid_dumpable, which should be 0. When this happens no ptrace() etc. will be possible on the end on the user/group that the process drops privileges to. It can be problematic when the unprivileged process subsequently performs an execve() without closing sensitive file descriptors, like it happened in open-vm-tools (CVE-2023-34059). An explicit prctl(PR_SETDUMPABLE, 0) could be considered in the patch to make this requirement explicit. Dropping only the fsuid and fsgid on Linux would avoid any potential ptrace() dangers. The system calls are marked deprecated, though, and have unfortunate error handling. What makes me feel a bit uneasy about this approach is that the programmer has to make sure that the privilege drop context only ever deals with file system operations. Things like e.g. obtaining SO_PEERCREDs from a UNIX domain socket will still operate on the egid and euid, which will remain at 0. For me it feels better to drop all privileges for good. For specific purposes like in the case of pam-oath I believe it can make sense to take that route, though. Best Regards Matthias
signature.asc
Description: PGP signature
