Skip to content

Commit

Permalink
Rollup merge of rust-lang#121650 - GrigorenkoPV:cap_setgid, r=Amanieu
Browse files Browse the repository at this point in the history
change std::process to drop supplementary groups based on CAP_SETGID

A trivial rebase of rust-lang#95982

Should fix rust-lang#39186 (from what I can tell)

Original description:

> Fixes rust-lang#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 rust-lang#88716 but there was a bit of uncertainty from `@Amanieu` here: [rust-lang#88716 (comment)](rust-lang#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 rust-lang#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.
  • Loading branch information
matthiaskrgr authored Mar 14, 2024
2 parents 68ca795 + 3a6af84 commit eaa8daf
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 3 deletions.
7 changes: 7 additions & 0 deletions library/std/src/os/unix/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ pub trait CommandExt: Sealed {
/// Sets the child process's user ID. This translates to a
/// `setuid` call in the child process. Failure in the `setuid`
/// call will cause the spawn to fail.
///
/// # Notes
///
/// This will also trigger a call to `setgroups(0, NULL)` in the child
/// process if no groups have been specified.
/// This removes supplementary groups that might have given the child
/// unwanted permissions.
#[stable(feature = "rust1", since = "1.0.0")]
fn uid(&mut self, id: UserId) -> &mut process::Command;

Expand Down
14 changes: 11 additions & 3 deletions library/std/src/sys/pal/unix/process/process_unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,14 +330,22 @@ impl Command {
if let Some(u) = self.get_uid() {
// When dropping privileges from root, the `setgroups` call
// will remove any extraneous groups. We only drop groups
// if the current uid is 0 and we weren't given an explicit
// if we have CAP_SETGID and we weren't given an explicit
// set of groups. If we don't call this, then even though our
// uid has dropped, we may still have groups that enable us to
// do super-user things.
//FIXME: Redox kernel does not support setgroups yet
#[cfg(not(target_os = "redox"))]
if libc::getuid() == 0 && self.get_groups().is_none() {
cvt(libc::setgroups(0, crate::ptr::null()))?;
if self.get_groups().is_none() {
let res = cvt(libc::setgroups(0, crate::ptr::null()));
if let Err(e) = res {
// Here we ignore the case of not having CAP_SETGID.
// An alternative would be to require CAP_SETGID (in
// addition to CAP_SETUID) for setting the UID.
if e.raw_os_error() != Some(libc::EPERM) {
return Err(e.into());
}
}
}
cvt(libc::setuid(u as uid_t))?;
}
Expand Down

0 comments on commit eaa8daf

Please sign in to comment.