Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Document that std::os::unix::process::CommandExt.uid() triggers setgroups(0,0) #39186

Closed
zackw opened this issue Jan 19, 2017 · 4 comments · Fixed by #121650
Closed

Document that std::os::unix::process::CommandExt.uid() triggers setgroups(0,0) #39186

zackw opened this issue Jan 19, 2017 · 4 comments · Fixed by #121650
Labels
A-process Area: `std::process` and `std::env` C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@zackw
Copy link
Contributor

zackw commented Jan 19, 2017

While digging around for something else I have just discovered that setting the UID for a subprocess with CommandExt.uid() also causes a call to setgroups(0,0) in the child. This should be documented.

(Related to #38527)
@Mark-Simulacrum Mark-Simulacrum added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label May 20, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@steveklabnik steveklabnik added I-needs-decision Issue: In need of a decision. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools labels Aug 30, 2017
@steveklabnik
Copy link
Member

re-tagging as @rust-lang/libs ; would you like this to appear in the docs, or not? If so, we're happy to do it, but want to make sure to not over-specify.

@m-ou-se m-ou-se self-assigned this Feb 10, 2021
@joshtriplett
Copy link
Member

This seems like something we should document, yes. It's an unusual special-case that people should be aware of.

This also interacts with #72160 .

@joshtriplett joshtriplett added E-help-wanted Call for participation: Help is requested to fix this issue. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Feb 18, 2021
@m-ou-se m-ou-se assigned joshtriplett and unassigned m-ou-se Feb 18, 2021
@joshtriplett joshtriplett removed the I-needs-decision Issue: In need of a decision. label Feb 18, 2021
@joshtriplett
Copy link
Member

Ok, this is ready for a PR. I've labeled it accordingly.

@Elliot-Roberts Elliot-Roberts mentioned this issue Oct 26, 2021
Closed
@Elliot-Roberts Elliot-Roberts mentioned this issue Apr 12, 2022
Closed
@TornaxO7
Copy link

TornaxO7 commented May 2, 2023 •
edited
Loading

@joshtriplett hi! It may be a long time now but may I ask if this issue is still needed?

Because I can't find the setgroups(0, 0) call anywhere. I looked it as
follows:

  1. std::os::unix::process::CommandExt
  2. std::os::unix::process::CommandExt::uid
  3. std::process::Command::uid
  4. proccess_common.rs::uid

Did I lookup the wrong function and structs?

@workingjubilee workingjubilee added the A-process Area: `std::process` and `std::env` label Jul 22, 2023
@GrigorenkoPV GrigorenkoPV mentioned this issue Feb 26, 2024
Merged
@bors bors closed this as completed in eaa8daf Mar 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 15, 2024
@rust-timer
Unrolled build for rust-lang#121650
3137f94 
Rollup merge of rust-lang#121650 - GrigorenkoPV:cap_setgid, r=Amanieu

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-process Area: `std::process` and `std::env` C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
7 participants
@steveklabnik @joshtriplett @zackw @m-ou-se @Mark-Simulacrum @workingjubilee @TornaxO7