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.