On Tue, Oct 15, 2024 at 01:43:42PM +0200, Matthias Gerstner wrote: > 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
What about opening the path one portion at a time using openat() with O_NOFOLLOW (and, as applicable, O_DIRECTORY), ensuring that each portion is not "." or "..", does not contain "/", and is owned by either the target user or root? This solves all race conditions and does not require spawning another process. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab
signature.asc
Description: PGP signature
