change std::process to drop supplementary groups based on CAP_SETGID

[GrigorenkoPV]

A trivial rebase of #95982

Should fix #39186 (from what I can tell)

Original description:

Fixes #88716

• Before this change, when a process was given a uid via std::os::unix::process::CommandExt.uid, there would be a setgroups call (when the process runs) to clear supplementary groups for the child if the parent was root (to remove potentially unwanted permissions).
• After this change, supplementary groups are cleared if we have permission to do so, that is, if we have the CAP_SETGID capability.

This new behavior was agreed upon in #88716 but there was a bit of uncertainty from @Amanieu here: #88716 (comment)

I agree with this change, but is it really necessary to ignore an EPERM from setgroups? If you have permissions to change UID then you should also have permissions to change groups. I would feel more comfortable if we documented set_uid as requiring both UID and GID changing permissions.

The way I've currently written it, we ignore an EPERM as that's what #88716 originally suggested. I'm not at all an expert in any of this so I'd appreciate feedback on whether that was the right way to go.

[rustbot]

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[ChrisDenton]

The change was fcp'd and I'd be happy to r+ it on that basis but cc @Amanieu as you expressed specific implementation and documentation concerns in #88716 (comment)

[Amanieu]

As per #88716 (comment), I think we should fail the call entirely if we were not able to reset the groups after a uid change. Otherwise we could end up in a situation where the uid changed but kept the old groups.

This should not be a problem in practice because CAP_SETGID and CAP_SETUID will almost always come together.

[Amanieu]

I've traced the history of the failing test: it dates all the way back to when std::process was added to the standard library.

The documentation for the uid method only says that this maps to a setuid call, which will succeed if you pass it the current uid. It doesn't say anything about groups.

However this PR changes uid to also drop supplementary groups using setgroups, which is important to avoid "leaking" privileged groups to an unprivileged process. This breaks the test because calling setgroups always requires elevated privileges and will fail with EPERM otherwise.

The expected impact in practice is small: with a github code search I could only find examples of code that is already running as root dropping down to non-root privileges.

@rust-lang/libs-api How do we feel about making this (possibly breaking) change?

The previous PR ignored any EPERM errors from setgroups, but I argued against it because ignoring errors can be very error-prone for something as sensitive as dropping privileges.

[joshtriplett]

If you have permissions to change UID then you should also have permissions to change groups.

So, there's a weird interactions with containers here, where if we fail on EPERM, that would potentially break users of Linux containers / user namespaces.

In theory, with standard UNIX permissions, it's possible to use supplementary groups as a "negative permission" for something. For instance, in theory you could do this:

chown root:noxyz /usr/bin/xyz
chmod 0705 /usr/bin/xyz

And then users who don't have the noxyz group can run xyz, but users who do have the group noxyz cannot. And in the standard UNIX permission model, users don't normally have permissions to call setgroups to drop supplementary groups themselves.

This is not a very reasonable use of permissions, and almost nobody does this. There are a few random anecdotes of universities having a nogames group. Nonetheless, there were a non-zero number of users, and Linux treated this as a security matter.

Linux supports unprivileged creation of user namespaces, and within a user namespace, users can normally call setuid/setgid, and used to be able to call setgroups as well, until this arose as a security concern, at which point Linux created a mechanism to restrict setgroups.

Now, if you're setting up a user namespace, if you don't set up a gid_map for your container, you won't be permitted to call setgroups at all. If you do want to set up a gid_map, you either have to have CAP_SETGID in the parent namespace, or you have to write deny to /proc/${pid}/setgroups first, denying processes in the container from calling setgroups.

See man 7 user_namespaces for more information.

The reason I have the context on this:

• Years ago I attempted to add a "supplementary UIDs" mechanism to Linux (e.g. setusers analogous to setgroups, so that users could have a set of UIDs that they could use for e.g. setting up unprivileged containers).
• Someone pointed out the theoretical possibility of someone using ---r-xr-x permissions to deny a specific user permissions to something.
• In attempting to argue that this shouldn't be treated as a fatal problem, I observed that the same was already true of setgroups, which at that time could be dropped via user namespaces.
• This was promptly treated as a security bug in user namespaces, resulting in this redesign of the handling of setgroups within user namespaces (and the rejection of setusers).

A quick check on my system shows, for instance, that Firefox creates containers that have setgroups set to deny.

As a result of this, if an unprivileged user sets up a user namespace (as happens regularly to sandbox code), and code running within that namespace wants to drop permissions (e.g. going from container-root to container-unprivileged-user), they can call setuid/setgid to change those permissions, but they can't call setgroups. Which means, if we fail on EPERM from setgroups, calling .uid(...)here will cause process launch to fail for non-obvious and non-documented reasons, and users won't have an alternative other than manually callingsetuid/setgidthemselves inpre_exec` (which seems more unsafe and more error-prone).

I would propose that if you're getting EPERM from setgroups, ignoring that error would not result in unexpectedly retaining privileges, because in the common scenario of being system-root you'll have the requisite permissions, and in the container-root scenario that produces EPERM, it's expected that a process in a container may not be able to drop group permissions, and I think it'd be surprising and unexpected if users have to go out of their way to do something unusual (e.g. call something other than uid or call a different function before/after uid) in order to work inside a container.

Thus, I'd propose that we ignore EPERM from the call to setgroups caused by uid.

(Separately, we should figure out what to do with the call caused by groups, but that's still a nightly-only API.)

I also think we should document very clearly what uid does.

Summary:

• Containers created by unprivileged users are a common scenario where you have permission to call setuid and setgid but not setgroups.
• We should document what uid does, why it calls setgroups, and how it handles EPERM, with a link to https://www.man7.org/linux/man-pages/man7/user_namespaces.7.html.
• I think we should ignore EPERM errors from the setgroups call that uid causes, so that software works in containers by default without having to add special handling for the container case.
• We should add an unresolved question to the tracking issue for the groups nightly API to decide how to handle EPERM there.
• If we end up not ignoring EPERM errors, we need to provide some reasonable way for users to work as expected in containers. I think that wouldn't be the ideal solution, though.

[GrigorenkoPV]

• I think we should ignore EPERM errors from the setgroups call that uid causes, so that software works in containers by default without having to add special handling for the container case.

Added a commit reverting the change. Not squashing to reflect the decision-making process in the commit history. But if you would like me to, I can just reset the branch back to the initial 3a6af84.

[Amanieu]

I agree with Josh's summary, let's just revert back to the original version that ignores EPERM errors.

r=me once the revert is squashed.

[GrigorenkoPV]

I thought GitHub wouldn't re-run the CI for a commit that was already tested before, but whataver.

The PR is now "squashed" (actually just reverted back to the first commit).

[Amanieu]

@bors r+

[bors]

📌 Commit 3a6af84 has been approved by Amanieu

It is now in the queue for this repository.