linux: std::process, insecure dropping of ancillary groups

[bnoordhuis]

Commit 22ddcd1 made libstd drops ancillary groups when uid == 0:

rust/library/std/src/sys/unix/process/process_unix.rs

Lines 312 to 314 in 385f8e2

	if libc::getuid() == 0 && self.get_groups().is_none() {
	cvt(libc::setgroups(0, ptr::null()))?;
	}

Before that it unconditionally dropped group membership.

The new logic is wrong on Linux: it doesn't account for processes whose uid != 0 but have the CAP_SETGID capability.

Such processes can and should drop ancillary groups, otherwise child processes inherit permissions they otherwise wouldn't have.

Suggested change:

if self.get_groups().is_none() {
    let _ = libc::setgroups(0, ptr::null()); // or return unless EPERM
}

[joshtriplett]

This seems like a potentially reasonable approach. It'd be a behavior change, but technically the previous change was a behavior change in the case of CAP_SETGID.

(It seems worth noting that CAP_SETGID is generally root-equivalent, so this doesn't actually seem like a security issue of any kind.)

[cuviper]

Whatever we do, we need to reconcile this with #90292 as it becomes documented.
I agree the suggested change makes sense though.

[bnoordhuis]

It seems worth noting that CAP_SETGID is generally root-equivalent, so this doesn't actually seem like a security issue of any kind.

IMO, it is. Scenario: non-root, sudo-like program that:

1. has CAP_SETGID + CAP_SETUID in order to switch users when spawning child processes, and
2. is a member of one or more supplementary groups in order to read or write sensitive files

Not dropping group membership is a catastrophic security breach in that setup.

[joshtriplett]

Wouldn't it need to drop that capability though, before the exec?

In any case, this does still need fixing if possible. Semantically, though, I don't think we should unconditionally drop supplementary groups; that would be a bug as well. Consider an ordinary user, who has a pile of supplementary groups for permission to various devices (e.g. audio). A Rust program runs a child process, and that process should still have those supplementary groups.

One possible fix: getauxval for AT_SECURE, which is the standard way of detecting "this process gained special privileges when run". That covers setuid/setgid/setcaps. (It doesn't cover programs that start out as root and drop some-but-not-all privileges, but such programs need to take care when running other programs and should already know to drop their remaining privileges in such cases.)

[bnoordhuis]

Wouldn't it need to drop that capability though, before the exec?

Not necessarily, it depends on the capability set (threads have several) the capability is part of. Some persist across execve() calls, some don't.

(I wanted to write up a short explanation of the algorithm but I can't hope to do it justice in a few words, it's complicated.)

I'm pessimistic checking AT_SECURE is sufficient. man 7 capabilities claims the following:

If ambient capabilities cause a process's permitted and effective capabilities to increase during an execve(2), this does not trigger the secure-execution mode described in ld.so(8).

It's set only when permitted is a superset of ambient.

prctl() with one of the PR_CAPxxx flags might work but that gets complicated fast and I'd argue dropping group membership unless the user indicates otherwise is traditional and expected UNIX behavior.

It's what libuv does and a CERT C recommendation.

[joshtriplett]

My concern is that the user should not have to retrieve and re-set the supplementary groups just to have them not cleared.

Also, I'd like to avoid adding an unexpected source of additional complexity; I think inheriting will be the common case.

[Amanieu]

Note that clearing the groups only happens if the user asked to change to a different UID. I think it makes sense in this case to always drop the groups since they are tied to the previous UID and most likely aren't supposed to be kept for the new UID. If you really want to preserve the groups while changing UID (unlikely anyone would ever want that...) then you can explicitly pass the list of groups of the current process.

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.

@rfcbot fcp merge

[rfcbot]

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @cuviper
• @joshtriplett
• @m-ou-se
• @yaahc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[Amanieu]

@bnoordhuis We have accepted this change, would you be willing to write up a PR for it?

[Elliot-Roberts]

I could write a PR for this and maybe also bundle in a redo of #90292.

[bnoordhuis]

@Elliot-Roberts Please go ahead. I'm not working on this at the moment.

[Elliot-Roberts]

Sorry scratch that I'm having trouble with my dev environment and don't have the time to fix it right now. I might pick this back up but I'm no longer claiming it.