1463873 - Systematically set CLOEXEC to prevent leaking file descriptors to fork+exec child processes

Status: NEW

(Core :: General, enhancement, P3)

Description

[Jan Keromnes [:janx]]

Attachment: cloexec.patch

This bug is an exploration of using the clang-tidy static analyzer on Firefox: http://clang.llvm.org/extra/clang-tidy/

Some clang-tidy checkers ensure that opened (potentially sensitive) file descriptors don't remain open across a fork+exec (e.g. to a lower-privileged SELinux domain), by systematically setting CLOEXEC.

These checkers are called:

    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat
    android-cloexec-dup
    android-cloexec-epoll-create
    android-cloexec-epoll-create1
    android-cloexec-fopen
    android-cloexec-inotify-init
    android-cloexec-inotify-init1
    android-cloexec-memfd-create
    android-cloexec-open
    android-cloexec-socket

We'd like to understand how useful these checkers are (e.g. do they actually fix security problems?) and whether we should officially enable them for Firefox (e.g. via our Code Review Bot).

To illustrate, I'm attaching an example patch that automatically sets CLOEXEC on all file descriptors that we manipulate in Firefox.

Comment 1

[Jan Keromnes [:janx]]

Comment on attachment 8980085 [details] [diff] [review]
cloexec.patch

Kris, since you recently filed & fixed the similar bug 1455446, do these CLOEXEC auto-fixes look valuable to you?

(Also, do they look security-sensitive, or should we open this bug more publicly?)

Comment 2

[Jan Keromnes [:janx]]

Note: I don't fully understand the implications of setting/omitting CLOEXEC on file descriptors, but bug 147659 comment 41 (from 11 years ago) has me worried that it what Dean calls "(3)" is not the best solution to prevent leaking file descriptors.

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8980085 [details] [diff] [review]
cloexec.patch

Review of attachment 8980085 [details] [diff] [review]:
-----------------------------------------------------------------

From a defense in depth standpoint, I think it probably makes sense. However, some points to keep in mind:

1) A lot of these changes are in vendored code that we can't change in-tree. nss and nspr, in particular, are only allowed to be changed by imports from upstream repositories.

2) When we spawn sandboxed content processes, we explicitly close all file descriptors that we don't want inherited after we fork. There are some other ways that we spawn processes, and we ideally don't want those to inherit extraneous descriptors either, but that's generally less of a security issue.

3) Most of these files are opened, read, and then immediately closed. There's still a possibility of them being inherited in a race if a process is spawned by a background thread, but it's at least relatively unlikely. As long as CLOEXEC is set atomically in the open operation, I guess that's still an improvement, and in these cases, I believe it is atomic (at least on some platforms). The way that I set it on socket FDs in bug 1455446 is unfortunately not atomic (and I don't think we have any available atomic solutions in that case), so we still rely on backup measures in those cases.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Putting this in the sandboxing component, since that's where the security impact would be. ni? Jed for feedback.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8980085 [details] [diff] [review]
cloexec.patch

Review of attachment 8980085 [details] [diff] [review]:
-----------------------------------------------------------------

In addition to the general issues with vendored code, there are some problems with these lints:

1. The "e" flag for fopen() is nonportable.  It could be used in OS-specific code (like nsNotifyAddrListener_Linux.cpp), but not in general.

2. The fcntl(fd, F_DUPFD_CLOEXEC) construct is broken; that fcntl takes an argument, which is a lower bound on the fd to create, and if it's uninitialized then it will likely be out-of-range for a file descriptor.  (I tested this under strace and it tried to pass 140736696312632, vs. a maximum fd value of 1023.)  I assume they meant fcntl(fd, F_DUPFD_CLOEXEC, 0), and I have to wonder if anyone has ever actually used this lint.  On the bright side, F_DUPFD_CLOEXEC was added to POSIX in 2008, so we can probably treat it as portable (among Unixes).

Not seen in this patch but also sadly nonportable, for future reference: SOCK_CLOEXEC, pipe2(), and dup3().

And I notice that it didn't pick up this fopen: https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/ipc/chromium/src/base/file_util_posix.cc#152 (but see also bug 1456911).

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

To reiterate part of comment #3: for all of our sandboxed child processes (content, media plugin, …) we close all file descriptors that aren't explicitly given to it (or stdin/out/err), and this is atomic with respect to other threads in the parent process (except for a slight race condition on some Tier-3 platforms, which is covered by bug 1400051).

It generally wouldn't hurt to set close-on-exec where we can, and there have been some issues around that in the past (e.g., bug 1363378), but it's not a security concern for sandboxing.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This isn't a sandbox escape (or otherwise security-sensitive as far as I can tell), so I'm unhiding the bug.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Update to comment #5: F_DUPFD_CLOEXEC is in POSIX 2008, but it's not in Solaris 10, and sort of not in Android until API 21/Lollipop (the kernel should support it but the headers don't define the constant, so it would have to be hard-coded).  So that's also not portable in practice.

On the other hand, most of the extensions I assumed were Linux-specific have been accepted for a future revision of POSIX, if I understand correctly; https://wiki.freebsd.org/AtomicCloseOnExec has some details and links.  So they might be usable at some point in the distant future.


Also, moving this bug out of sandboxing.

Comment 9

[Andrew Overholt [:overholt]]

This feels like the kind of bug that should probably remain in Core::General. Is any more work planned here, Jan?

Comment 10

[Jan Keromnes [:janx]]

Thank you Kris and Jed for the reviews!

(In reply to Andrew Overholt [:overholt] from comment #9)
> Is any more work planned here, Jan?

I'd love to eventually use the feedback to extract the useful fixes from this patch (if there are any), however since it is low-priority and not security-sensitive, it might take a while. Please feel free to close this bug if you don't find it valuable.

Comment 11

[sami.vanttinen]

Any updates for the possible fix?