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.

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

Allow configuration of uid/gid/detach on processes #12085

Merged
merged 2 commits into from
Feb 17, 2014

Conversation

alexcrichton
Copy link
Member

This just copies the libuv implementation for libnative which seems reasonable
enough (uid/gid fail on windows).

Closes #12082

fn main() {
unsafe { libc::setsid(); }

let config = process::ProcessConfig {
Copy link
Member

Choose a reason for hiding this comment

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

This seems like something that's ripe for FSU:

process::ProcessConfig {
    program: "/bin/sh",
    io: &[CreatePipe(true, false)],
    detach: true,
    .. default()
}

@alexcrichton
Copy link
Member Author

Updated with more FSU, configuration options in the std::run module, and a few more tests.

@bnoordhuis
Copy link
Contributor

I'm afraid the libuv implementation is pretty flawed. Things it should do but doesn't:

  • call setgroups() so auxiliary groups that the user is a member of are dropped
  • on Linux, drop some or all capabilities

It's something I had on my mental TODO list but never got around to fixing. I was going to report it but it turns out someone opened an issue for it 8 days ago, see joyent/libuv#1093.

}
None => {}
}
if config.detach && libc::setsid() == -1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Libuv doesn't check for setsid() errors because setsid() fails if the process is already the group leader. Probably an academic issue right after forking, though.

@alexcrichton
Copy link
Member Author

Interesting! Should this call setgroups(0, NULL) right before the call to setuid? Taking a look at the manpage isn't not totally clear to me what's going on, but it sounds important!

Additionally, when you say to drop capabilities, it sounds reasonable to me, but I don't think I'm familiar enough with linux to know which functions do that. Do you have a particular set of functions in mind?

Also, thanks for taking a look at this!

@bnoordhuis
Copy link
Contributor

Should this call setgroups(0, NULL) right before the call to setuid?

Yes, that's correct.

Additionally, when you say to drop capabilities, it sounds reasonable to me, but I don't think I'm familiar enough with linux to know which functions do that. Do you have a particular set of functions in mind?

Here is where things get somewhat complicated, I'm afraid...

The recommended approach is to use libcap or libcap-ng but that means taking on a dependency on a library that doesn't always come with the base system.

The alternative approach is to make the capget and capset system calls directly. Those system calls have gone through a number of iterations so you need to probe the kernel to figure out what is actually supported. It's not super difficult but it involves a bit of special-casing.

It's probably worth mentioning that the man page for the system calls is years out of date. The current API is 0x20080522 and was added in 2.6.26. It should be mostly compatible with 0x20071026, IIRC it was added to work around ABI breakage with 32 bits programs on 64 bits kernels.

About capability dropping itself, I would suggest making it opt-in or maybe opt-out but not mandatory. Sometimes you want the child process to inherit some or all of your capabilities.

@alexcrichton
Copy link
Member Author

Hm, dropping capabilities sounds like it's quite complicated to do robustly, so I may defer that to a later patch (but open an issue on that).

Could you explain to me a little more about setgroups? I didn't find much googling around except for this stackoverflow question. Should setgroups be called for either setuid or setgid, or only one? The idea is to drop all "extra groups" when you're trying to drop privileges?

@bnoordhuis
Copy link
Contributor

Hm, dropping capabilities sounds like it's quite complicated to do robustly, so I may defer that to a later patch (but open an issue on that).

If you cc me on the issue, I'll have a stab at it.

Could you explain to me a little more about setgroups? I didn't find much googling around except for this stackoverflow question. Should setgroups be called for either setuid or setgid, or only one?

It's a privileged operation so it should come before the call to setuid(). Whether you call setgroups() or setgid() first doesn't matter, the ancillary groups are a property of the user, not the user's primary group.

The idea is to drop all "extra groups" when you're trying to drop privileges?

That's right. There are cases where you might want to retain membership of some groups so it should probably be configurable.

@alexcrichton
Copy link
Member Author

I have updated with a call to setgroups and ignoring the return value of setsid, @bnoordhuis is this what you had in mind?

I also opened #12137 about dropping capabilities.

(errno << 0) as u8,
];
assert!(output.inner_write(bytes).is_ok());
unsafe { intrinsics::abort() }
Copy link
Contributor

Choose a reason for hiding this comment

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

intrinsics::abort() gets lowered to UD2 or abort(), right? Seems a little harsh for what is basically a run-time error. A call to _exit() is perhaps more appropriate, if only because it won't make the process dump core.

@bnoordhuis
Copy link
Contributor

Left one more comment but apart from that looks great to me.

@alexcrichton
Copy link
Member Author

I, too, have noticed that our failed processes are causing core dumps which is a little annoying.

Updated with _exit, thanks @bnoordhuis for all the great advice!

@alexcrichton
Copy link
Member Author

ping r?

@alexcrichton
Copy link
Member Author

This is blocked on joyent/libuv#1117.

This just copies the libuv implementation for libnative which seems reasonable
enough (uid/gid fail on windows).

Closes rust-lang#12082
This notably includes joyent/libuv@6f62d62c in order to fix when processes fail
to spawn on windows
bors added a commit that referenced this pull request Feb 17, 2014
This just copies the libuv implementation for libnative which seems reasonable
enough (uid/gid fail on windows).

Closes #12082
@bors bors closed this Feb 17, 2014
@bors bors merged commit 74b42c6 into rust-lang:master Feb 17, 2014
@emberian emberian mentioned this pull request Feb 17, 2014
@alexcrichton alexcrichton deleted the issue-12082 branch February 19, 2014 18:27
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

io::Process needs to expose more functionality
4 participants