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

darwin: spawn new process with POSIX_SPAWN_CLOEXEC_DEFAULT #1483

Open
bnoordhuis opened this issue Sep 19, 2014 · 6 comments
Open

darwin: spawn new process with POSIX_SPAWN_CLOEXEC_DEFAULT #1483

bnoordhuis opened this issue Sep 19, 2014 · 6 comments
Labels

Comments

@bnoordhuis
Copy link
Contributor

On Linux (and FreeBSD too now, I think), libuv opens file descriptors with O_CLOEXEC. On other platforms however, there is a race window between the creation of the file descriptor and setting its FD_CLOEXEC flag. As a result, uv_spawn() may leak file descriptors into the new process and that's a potential security issue.

OS X has a POSIX_SPAWN_CLOEXEC_DEFAULT posix_spawnattr_t flag that could mitigate that. There is a gotcha however: you use posix_spawn_file_actions_addinherit_np() for file descriptors that the new process should inherit but that function only accepts file descriptors < OPEN_MAX (10,240.)

Note that this could perhaps be solved generically with a pthread_atfork() handler but I think that would require libuv to store file descriptors in a global structure and exclusively use atomic operations to update that structure. A secondary issue with that approach is that it gives odd results when RLIMIT_NOFILE has been lowered. You won't be able to close file descriptors above the limit.

See also this comment.

@saghul saghul added the Unix label Sep 19, 2014
@txdv
Copy link
Contributor

txdv commented Oct 16, 2014

How do you come up with these?

@bnoordhuis
Copy link
Contributor Author

The angels talk to me.

(Actually, the thought popped in my head while going over open rust issues.)

@vtjnash
Copy link
Contributor

vtjnash commented Jul 22, 2015

Is there a reason that bInheritHandles is being set on Windows during CreateProcess? I would not expect to want to do this on Windows either.

https://github.com/libuv/libuv/blob/01bbf6fb1ccba74575fa232ff69bf1a00431c37b/src/win/process.c#L1082
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx

@saghul
Copy link
Contributor

saghul commented Jul 22, 2015

@vtjnash please open an issue over at libuv/libuv, we moved over there a while ago :-)

@vtjnash
Copy link
Contributor

vtjnash commented Jul 22, 2015

for the windows bit, i can just open a PR over there. just figured i would try to confirm first whether this was intentional.

@jaykrell
Copy link

Note that Windows mechanism here is flawed in one or two ways, and there is a workaround.
Here are the two problems:
Even if you give an explicit list of handles, they must be marked as inheritable.
Other code in the process calling CreateProcess in the old way without a list, will still inherit all inheritable handles.

The workaround for explicit inheritance is to first create a dummy parent process without inheritance, duplicate handles into it, and then specify it as the parent in the real CreateProcess.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants