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

feat(unstable): allow specifing gid and uid for subprocess #11586

Merged
merged 8 commits into from
Sep 13, 2021
Merged

feat(unstable): allow specifing gid and uid for subprocess #11586

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3cee3c5
impl
crowlKats Aug 5, 2021
5342bdc
fix
crowlKats Aug 24, 2021
21b37b5
add test
crowlKats Aug 25, 2021
f8762ad
Merge branch 'main' into set_gid/uid
crowlKats Aug 25, 2021
354a713
Merge branch 'main' into set_gid/uid
bartlomieju Sep 11, 2021
03a33ff
add pre_exec hook and update docs
crowlKats Sep 13, 2021
cd1ef49
fmt and lint
bartlomieju Sep 13, 2021
8cef2d8
Merge branch 'main' into set_gid/uid
bartlomieju Sep 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cli/dts/lib.deno.ns.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2041,6 +2041,9 @@ declare namespace Deno {
* Environmental variables for subprocess can be specified using `opt.env`
* mapping.
*
* On unix `opt.gid` and `opt.uid` can be used to define the gid and uid for
* the subprocess.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's going to be clear to most users what this means. Can I suggest copying the description from libstd's CommandExt?

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.

And:

Similar to uid, but sets the group ID of the child process. This has the same semantics as the uid field.

I'd argue changing uid/gid without being able to drop group membership is insecure (a mistake I made in libuv) but .groups() is experimental in libstd and tokio doesn't support it at all.

Copy link
Member

Choose a reason for hiding this comment

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

@crowlKats please update doc with Ben's suggestion.

I'd argue changing uid/gid without being able to drop group membership is insecure (a mistake I made in libuv) but .groups() is experimental in libstd and tokio doesn't support it at all.

@bnoordhuis what would you suggest in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either hold off on merging until Tokio grows the right APIs or add a really big warning to the documentation.

The problem with a warning is that it's not actionable. There's no way to detect an insecure situation and act on it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, as you point out the second option is not actionable. Let's hold off merging, and open an feature request to tokio.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at how libstd implements Command::spawn() (which tokio wraps) and it calls setgroups(0, null()) to drop group membership (barring bug rust-lang/rust#88716) so, on second thought, this is probably good to merge.

If you wanted to be extra secure, you could add a Unix-ony pre_exec hook that calls setgroups(0, null()).

Copy link
Member

Choose a reason for hiding this comment

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

@crowlKats could you add that call to pre_exec that Ben pointed out?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bartlomieju done

*
* By default subprocess inherits stdio of parent process. To change that
* `opt.stdout`, `opt.stderr` and `opt.stdin` can be specified independently -
* they can be set to either an rid of open file or set to "inherit" "piped"
Expand Down
4 changes: 4 additions & 0 deletions cli/dts/lib.deno.unstable.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,8 +794,12 @@ declare namespace Deno {
export function run<
T extends RunOptions & {
clearEnv?: boolean;
gid?: number;
uid?: number;
} = RunOptions & {
clearEnv?: boolean;
gid?: number;
uid?: number;
},
>(opt: T): Process<T>;

Expand Down
4 changes: 4 additions & 0 deletions runtime/js/40_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@
cwd = undefined,
clearEnv = false,
env = {},
gid = undefined,
uid = undefined,
stdout = "inherit",
stderr = "inherit",
stdin = "inherit",
Expand All @@ -114,6 +116,8 @@
cwd,
clearEnv,
env: ObjectEntries(env),
gid,
uid,
stdin: isRid(stdin) ? "" : stdin,
stdout: isRid(stdout) ? "" : stdout,
stderr: isRid(stderr) ? "" : stderr,
Expand Down
13 changes: 13 additions & 0 deletions runtime/ops/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ pub struct RunArgs {
cwd: Option<String>,
clear_env: bool,
env: Vec<(String, String)>,
gid: Option<u32>,
uid: Option<u32>,
stdin: String,
stdout: String,
stderr: String,
Expand Down Expand Up @@ -123,6 +125,17 @@ fn op_run(
c.env(key, value);
}

#[cfg(unix)]
if let Some(gid) = run_args.gid {
super::check_unstable(state, "Deno.run.gid");
c.gid(gid);
}
#[cfg(unix)]
if let Some(uid) = run_args.uid {
super::check_unstable(state, "Deno.run.uid");
c.gid(uid);
crowlKats marked this conversation as resolved.
Show resolved Hide resolved
}

// TODO: make this work with other resources, eg. sockets
if !run_args.stdin.is_empty() {
c.stdin(subprocess_stdio_map(run_args.stdin.as_ref())?);
Expand Down