Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

how to prevent open file descriptors from leaking to child processes? #6905

Closed
jandre opened this issue Jan 18, 2014 · 6 comments
Closed

how to prevent open file descriptors from leaking to child processes? #6905

jandre opened this issue Jan 18, 2014 · 6 comments
Assignees
Milestone

Comments

@jandre
Copy link

jandre commented Jan 18, 2014

Hi,

There's no obvious way right now to prevent a child process forked from a node process from having access to a file descriptor (e.g., calling fs.open with O_CLOEXEC).

This is a security risk and very likely a resource leak (not sure if the child process FD handles are counted against open fd limits).

Example code to illustrate what I'm talking about, which leaks access to /tmp/sensitive_file: https://gist.github.com/jandre/8485608

Supporting evidence as to why this is bad: http://www.python.org/dev/peps/pep-0446/#security-vulnerability

Anyway, I may be missing something, but the only workaround I think is to call fs.open with hardcoded O_CLOEXEC, or to write a C++ addon that exposes uv__cloexec-like functionality for a file descriptor.

As far as a real fix, maybe consider the following options:

  • a flag I'd pass to fs.open() (similar to 'w', 'r', etc) that sets O_CLOEXEC
  • a uv__cloexec wrapper for the fs module
  • (longer term) automatic 'closing' of non stdio fds on exec-like functions toggleable with a flag (like python's close_fds parameter described in the PEP above).
@indutny
Copy link
Member

indutny commented Jan 18, 2014

I think, it should be O_CLOEXEC by default. I'll look into it.

@ghost ghost assigned indutny Jan 18, 2014
@isaacs
Copy link

isaacs commented Jan 19, 2014

I cannot think of many cases where you wouldn't want O_CLOEXEC set on a fd. Nevertheless, if we make O_CLOEXEC the default, we should have a way to specify a flag that doesn't set it, so that it's at least possible to leave O_CLOEXEC unset in the cases where you do intend to share the fd with a child proc.

@piscisaureus
Copy link

I cannot think of many cases where you wouldn't want O_CLOEXEC set on a fd.

Agree. Libuv does this (almost) always by default on all platforms, but uv_fs_open on unix doesn't for some reason. This looks like just an oversight and should be easy to fix (*), patch welcome.

Nevertheless, if we make O_CLOEXEC the default, we should have a way to specify a flag that doesn't set it, so that it's at least possible to leave O_CLOEXEC unset in the cases where you do intend to share the fd with a child proc.

I don't think this is necessary. uv_spawn provides an alternative means to specify explicitly which FDs should be inherited by the child process.

(*) The only caveat is that since opening files happens in the thread pool, a race condition may exist so that an fd may still be leaked if uv_spawn runs between the call to open() and the call to uv__cloexec. This situation currently also exists when a process uses multiple event loops. It can be solved on not-extremely-old versions of linux by calling open with the O_CLOEXEC flag, but there is no solution for it's silly cousin OS X, so the only solution there would be to use a global mutex that prevents fd-creation and process-spawning from happening at the same time.

@isaacs
Copy link

isaacs commented Jan 19, 2014

Yeah, just calling cloexec always is probably fine, then. I'd forgotten about the explicit stdio fd sharing. Better to just use that rather than rely on implicit junk.

@indutny
Copy link
Member

indutny commented Jan 29, 2014

Should be fixed by joyent/libuv#1091

indutny added a commit to joyent/libuv that referenced this issue Jan 31, 2014
Every file descriptor opened using libuv should be automatically marked
as CLOEXEC to prevent it from leaking to a child process. Note that
since we are opening fds in a thread pool, there is a possible race
condition between `uv_spawn()` and the `open()` + `uv__cloexec()`. The
rwlock was added to avoid it.

see nodejs/node-v0.x-archive#6905
@indutny
Copy link
Member

indutny commented Jan 31, 2014

Consider it fixed in joyent/libuv@6abe1e4

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

No branches or pull requests

4 participants