POpen does not close fds when fds have been inherited from a process with a higher resource limit

[sstewartgallus]

BPO	21618
Nosy	@gpshead, @larryhastings, @4kir4
Files	python.patch issue21618-34-gps01.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gpshead'
closed_at = <Date 2014-06-01.21:04:03.852>
created_at = <Date 2014-05-31.04:24:41.163>
labels = ['type-bug']
title = 'POpen does not close fds when fds have been inherited from a process with a higher resource limit'
updated_at = <Date 2014-06-01.21:04:03.850>
user = 'https://bugs.python.org/sstewartgallus'

bugs.python.org fields:

activity = <Date 2014-06-01.21:04:03.850>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2014-06-01.21:04:03.852>
closer = 'gregory.p.smith'
components = []
creation = <Date 2014-05-31.04:24:41.163>
creator = 'sstewartgallus'
dependencies = []
files = ['35419', '35425']
hgrepos = []
issue_num = 21618
keywords = ['patch']
message_count = 13.0
messages = ['219440', '219442', '219443', '219455', '219460', '219477', '219478', '219479', '219480', '219491', '219509', '219523', '219525']
nosy_count = 5.0
nosy_names = ['gregory.p.smith', 'larry', 'akira', 'python-dev', 'sstewartgallus']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue21618'
versions = ['Python 3.4', 'Python 3.5']

[sstewartgallus]

The sysconf(_SC_OPEN_MAX) approach to closing fds is kind of flawed. It is kind of hacky and slow (see http://bugs.python.org/issue1663329). Moreover, this approach is incorrect as fds can be inherited from previous processes that have had higher resource limits. This is especially important because this is a possible security problem. I would recommend using the closefrom system call on BSDs or the /dev/fd directory on BSDs and /proc/self/fd on Linux (remember not to close fds as you read directory entries from the fd directory as that gives weird results because you're concurrently reading and modifying the entries in the directory at the same time). A C program that illustrates the problem of inheriting fds past lowered resource limits is shown below.

#include <errno.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <sys/time.h>
#include <sys/resource.h>

int main() {
    struct rlimit const limit = {
        .rlim_cur = 0,
        .rlim_max = 0
    };
    if (-1 == setrlimit(RLIMIT_NOFILE, &limit)) {
        fprintf(stderr, "error: %s\n", strerror(errno));
        exit(EXIT_FAILURE);
    }

puts("Printing to standard output even though the resource limit is lowered past standard output's number!");

    return EXIT_SUCCESS;
}

[sstewartgallus]

Okay here's a stub patch that address FreeBSD, NetBSD and Linux. I'm not sure how to address the other platforms.

[sstewartgallus]

Oh right! I forgot a possible problem with my proposed patch. It is incompatible with Valgrind (see issue https://bugs.kde.org/show_bug.cgi?id=331311). Either this patch won't be applied, Valgrind compatibility is judged not essential or the Valgrind developers will start emulating /proc/self/fd.

[gpshead]

Are you aware that the subprocess module does use /proc/self/fd in Python 3.2 and later? The fd closing is not done from Python code.

See Modules/_posixsubprocess.c - http://hg.python.org/cpython/file/53fa2c9523d4/Modules/_posixsubprocess.c

[gpshead]

regardless, the current C code for this does limit itself to the sysconf(_SC_OPEN_MAX) max_fd from module import time when closing fds found in /proc/self/fd so this code does still have a bug in that fds higher than that will remain unclosed (at which point your valgrind issue would come into play unless we can detect we are running under valgrind and alter our behavior to obey the max in that case).

[gpshead]

There appear to be a two bugs here, depending on which platform subprocess is being used on.

1. on systems where it uses /prod/self/fd, /dev/fd or similar:

It should not pay attention to end_fd at all. It knows the list of actual open fds and should use that. If possible, consider detecting and avoiding closing valgrind fds; but that is a special case for a valgrind bug and likely not worth it.

2. on systems without a way to get the list of open file descriptors:

The sysconf("SC_OPEN_MAX") value is only saved at module import time but may be changed up or down at runtime by the process by using the setrlimit(RLIMIT_NOFILE, ...) libc call. what sysconf returns is the same as the current rlim_cur setting. (at least on Linux where this code path wouldn't actually be taken because #1 is available).

possible solution: call getrlimit(RLIMIT_NOFILE) and use rlim_max instead of sysconf("SC_OPEN_MAX") at module import time.
caveat: rlim_max can be raised by processes granted that capbility. It is impossible to do anything about that in this scenario given we're operating w/o a way to get a list of open fds.
impact: only on OSes that lack implementations that get a list of open fds as in #1 above. so... nothing that anyone really uses unless they choose to come contribute support for that themselves. (linux, bsd and os x all fall into #1 above)

Neither of these are likely scenarios so I wouldn't consider this a high priority to fix but it should be done. Most code never ever touches its os resource limits. getrlimit and setrlimit are not exposed in the os module; you must use ctypes or an extension module to call them from Python:

import ctypes
class StructRLimit(ctypes.Structure):
  _fields_ = [('rlim_cur', ctypes.c_ulong), ('rlim_max', ctypes.c_ulong)]
libc = ctypes.cdll.LoadLibrary('libc.so.6')
RLIMIT_NOFILE = 7  # Linux
limits = StructRLimit()
assert libc.getrlimit(RLIMIT_NOFILE, ctypes.byref(limits)) == 0
print(limits.rlim_cur, limits.rlim_max)
limits.rlim_cur = limits.rlim_max
assert libc.setrlimit(RLIMIT_NOFILE, ctypes.byref(limits)) == 0

[sstewartgallus]

I agree that this is not a likely scenario but I can think of one
mildly plausible scenario. Suppose some web server runs a Python CGI
script but has a bug that leaks a file descriptor into the script. The
web server sandboxes the Python CGI script a little bit with resource
limits so the leaked file descriptor is higher than the script's file
descriptor maximum. The Python CGI script then runs a sandboxed
(perhaps it's run as a different user) utility and leaks the file
descriptor again (because the descriptor is above the resource
limits). This utility is somehow exploited by an attacker over the
internet by being fed bad input. Because of the doubly leaked file
descriptor the attacker could possibly break out of a chroot or start
bad input through a sensitive file descriptor. Anyways, the bug should
be fixed regardless.

Thanks for correcting me on the location of the fd closing code. Some
observations.

Strangely, there seems to be a _close_fds method in the Python
subprocess module that is not used anywhere. Either it should be
removed or fixed similarly. For understandability if it is fixed it
should simply delegate to the C code.

The bug I mentioned earlier about concurrently modifing the fd dir and
reading from it occurs in _close_open_fd_range_safe which is a genuine
security issue (although I don't know if it's ver likely to happen in
practise). Because _close_open_fd_range_safe can't allocate memory the
code there will be pretty ugly but oh well.

There doesn't seem to be any point to caching max_fd in a variable on
module load. Why not just use sysconf every time it is needed? Is
there some need for really fast performance? Does sysconf allocate
memory or something?

Anyways, the code should be refactored to not use max_fd on the
platforms that support that.

Thank you for your thoughts. Also, should I keep discussion of some of
the bugs I observed here or raise them in other issues so they don't
get lost?

[4kir4]

getrlimit and setrlimit are not exposed in the os module; you must use ctypes or an extension module to call them from Python:

There is resource module:

  >>> import resource
  >>> resource.getrlimit(resource.RLIMIT_NOFILE) 
  (1024, 4096)

[sstewartgallus]

I found another problem with _close_open_fd_range_safe. POSIX leaves
the state of a file descriptor given to close undefined if the close
fails with EINTR. I believe but haven't double checked that close
should not be retried on EINTR on all of our supported platforms. If
you must have absolute portability, block all signals so that close
can't fail with EINTR and then unblock them after close. This isn't an
actual problem because the code will just close an extra time but it's
still bothersome.

[gpshead]

Here's a patch with a unittest that reproduces the problem with fixes to stop using any end_fds. The max fd is only ever used in the absolute fallback situation where no way to get a list of open fd's is available. In that case it is obtained from sysconf() at the time it is needed rather than module load time as sysconf() is async-signal-safe.

I'm not worried about calling close() an additional time on EINTR in the single threaded child process prior to exec(). The most that will happen is one extra call with a different error if the fd is in a bad state from the previous one. That is better than any chance of one being left open.

[sstewartgallus]

Thank you for the very quick patch Gregory P. Smith.

It's fair enough if you don't bother to fix the EINTR issue.

One small note:

•    """Confirm that bpo-21618 is fixed (mail fail under valgrind)."""
  

That's a typo right? Shouldn't it be may instead of mail?

[python-dev]

New changeset 5453b9c59cd7 by Gregory P. Smith in branch '3.4':
Don't restrict ourselves to a "max" fd when closing fds before exec()
http://hg.python.org/cpython/rev/5453b9c59cd7

New changeset 012329c8c4ec by Gregory P. Smith in branch 'default':
Don't restrict ourselves to a "max" fd when closing fds before exec()
http://hg.python.org/cpython/rev/012329c8c4ec

[gpshead]

Backported to subprocess32 in https://code.google.com/p/python-subprocess32/source/detail?r=1c27bfe7e98f78e6aaa746b5c0a4d902a956e2a5