Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

unix: call setgoups before calling setuid/setgid #1105

Closed
wants to merge 1 commit into from
Closed

unix: call setgoups before calling setuid/setgid #1105

wants to merge 1 commit into from

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Feb 10, 2014

Partial fix for #1093

Shamelessly taken from: rust-lang/rust#12085

/cc @indutny

If you have a few milliseconds to spare, it'd be great to get your input, @bnoordhuis.

As for the dropping caps part, I'll need to look into that, doesn't look anywhere straightforward :-/

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Saúl Ibarra Corretgé

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@@ -330,6 +330,17 @@ static void uv__process_child_init(const uv_process_options_t* options,
_exit(127);
}

if (options->flags & (UV_PROCESS_SETUID|UV_PROCESS_SETGID)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add space between: ...SETUID | ...SETGID

@indutny
Copy link
Contributor

indutny commented Feb 10, 2014

Minor style nit, otherwise LGTM. But yeah, having @bnoordhuis opinion on this would be totally cool!

@saghul
Copy link
Contributor Author

saghul commented Feb 10, 2014

Force pushed addressing feedback, thanks @indutny!

int saved_errno;
saved_errno = errno;
setgroups(0, NULL);
errno = saved_errno;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be shortened to SAVE_ERRNO(setgroups(0, NULL)) but apart from that LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ben, will fix.
On Feb 10, 2014 6:55 PM, "Ben Noordhuis" notifications@github.com wrote:

In src/unix/process.c:

@@ -330,6 +330,20 @@ static void uv__process_child_init(const uv_process_options_t* options,
_exit(127);
}

  • if (options->flags & (UV_PROCESS_SETUID | UV_PROCESS_SETGID)) {
  • /* When dropping privileges from root, the setgroups call will
  • \* remove any extraneous 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. This will fail if we
    
  • \* aren't root, so don't bother checking the return value, this
    
  • \* is just done as an optimistic privilege dropping function.
    
  • */
    
  • int saved_errno;
  • saved_errno = errno;
  • setgroups(0, NULL);
  • errno = saved_errno;

Can be shortened to SAVE_ERRNO(setgroups(0, NULL)) but apart from that
LGTM.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1105/files#r9593936
.

@bnoordhuis
Copy link
Contributor

Change itself LGTM but as food for thought, you may want to consider adding an API that lets the user retain some or all of the ancillary groups. Ditto for capabilities.

@saghul
Copy link
Contributor Author

saghul commented Feb 10, 2014

Thanks guys! Landed as 66ab389. I also created #1106 to track the Linux capabilites part and the API suggesting by Ben.

@saghul saghul closed this Feb 10, 2014
@saghul saghul deleted the setgroups branch September 11, 2014 19:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants