Change linux std::process to drop supplementary groups based on CAP_SETGID
#95982
Conversation
|
Thank you for submitting a new PR for the library teams! If this PR contains a stabilization of a library feature that has not already completed FCP in its tracking issue, introduces new or changes existing unstable library APIs, or changes our public documentation in ways that create new stability guarantees then please comment with |
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
JohnCSimon
added
S-waiting-on-review
apiraino
added
the
T-libs
JohnCSimon
added
S-waiting-on-review
JohnCSimon
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #101077) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@Elliot-Roberts hi! As you might've noticed, your PR has some conflicts. May I ask if you need a helping hand to get this working again? (I don't know if I'd be able to help, but asking doesn't hurt I hope) |
|
@Elliot-Roberts ping from triage. FYI you still have conflicts to fix. It appears you may be inactive on Github. I'm going to mark this as waiting-on-author, and if we don't hear anything from you in the next few months, we'll close this PR. @rustbot author |
rustbot
added
S-waiting-on-author
|
Closing this pr as it is inactive. Feel free to reöpen this or preferably make a new pr in case you have the time for it |
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.
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.
Fixes #88716 and #39186
Replaces PR #90292
std::os::unix::process::CommandExt.uid, there would be asetgroupscall (when the process runs) to clear supplementary groups for the child if the parent was root (to remove potentially unwanted permissions).This new behavior was agreed upon in #88716 but there was a bit of uncertainty from @Amanieu here: #88716 (comment)
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.