753107 - [Security Review] Review B2G usage of CLOEXEC

Status: RESOLVED FIXED

(mozilla.org :: Security Assurance: Review Request, task, P2)

Description

[Guillaume Destuynder [:kang] [this account is no longer active]]

1) Who is/are the point of contact(s) for this review?
Chris Jones <[e-mail]>
   
2) Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):
CLOEXEC is "close on exec", in a nutshell file descriptors marked CLOEXEC via fcntl() will close on exec (fork & exec of the B2G content process for example) to ensure the fds are not leaking to the child process.
We need to review that this is correctly being used, and that no fd are missing CLOEXEC or not being closed properly (aka no fd leaking)

3) Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description:

For more detailed and more complete information see:

http://lwn.net/Articles/292843/
http://udrepper.livejournal.com/20407.html
https://bugzilla.redhat.com/show_bug.cgi?id=443321
See also https://wiki.mozilla.org/B2G/Architecture/Runtime

4) Does this request block another bug? If so, please indicate the bug number
No

5) This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?
Strongly required for B2G M3 release, aka this should be done _before_ the end of this quarter (Q2 2012)

6) To help prioritize this work request, does this project support a goal specifically listed on this quarter's goal list? If so, which goal?
Yes, OS security for B2G (Security Assurance goal)

7) Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.)
a) Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users?
Yes it will (B2G, when released). It may affect Firefox Linux eventually, but not directly.

b) Are there any portions of the project that interact with 3rd party services?
Not strictly speaking, but the B2G content process which is the main reason for the CLOEXEC review loads content from various 3rd parties (Web Applications)

c) Will your application/service collect user data? If so, please describe 
No related to CLOEXEC directly.

8) If you feel something is missing here or you would like to provide other kind of feedback, feel free to do so here (no limits on size):
N/A

9) Desired Date of review (if known from https://mail.mozilla.com/home/[e-mail]/Security%20Review.html) and whom to invite. 
Any, the sooner the better. People to invite:
- Chris Jones
- Guillaume Destuynder
- Optional: Paul Theriault
- Optional: Lucas Adamski

Comment 1

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Since Paul is already leading B2G work for us I am assigning this to him.

:pauljt - let me know if we need a team review for this request of if you are going to handle it individually. If we do need a team review lets get a date set.

Comment 2

[Daniel Veditz [:dveditz]]

This review request seems to be requesting a low-level code audit of b2g looking for this pattern. Not sure the security team has the resources to do that at this time, and in fact in the past we've usually identified issues like this and assigned them back to the developers to seek and destroy. Could maybe use some of the static analysis tools developers have used in the past, sixgill and dehydra. Assigning to mcoates to figure out the staffing for this kind of request.

Comment 3

[Michael Coates [:mcoates] (acct no longer active)]

Spoke with Guillaume. A good option forward is to have Guillaume and Chris Jones meet to discuss the analysis process. Guillaume can do a first pass with Chris and then hand off to Chris for further analysis.

Review for this item requires code inspection and is best accomplished by someone more knowledgeable about the specific code.  Guillaume will advise on pattern for detecting problem.

Comment 4

[Guillaume Destuynder [:kang] [this account is no longer active]]

We're using mFileMap which is a structure containing all the FDs to be passed while launching the child process.

the fds, such as the ones for the IPC communication, and the android property workspace  as explicitely pushed in gecko/ipc/glue/GeckoChildProcessHost.cpp

the work mostly takes place in process_util_linux.cc in bool LaunchApp()

it calls CloseSuperfluousFds() (defined in ipc/chromium/src/base/process_util_posix.cc) which close all fds except the ones from the filemap.

This is called just before executing the child process.

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Resolved as per previous comment.