Consider disabling XPCOM services in child process that conflict with sandboxing

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks 1 bug)

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm5+, firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Vlad and I talked today about how we can simulate having a sandbox without actually having one. One obvious way it to disable the parts of XPCOM that allow writing to files in the child process. There would be two benefits:

1. We'd probably be able to generate better error messages than a lower-level sandbox could. We could throw exceptions with useful error messages ("You can't use nsIFile in a child process") and stack traces rather than crashing with something inscrutable like "Syscall __NR_stat64 is forbidden".

2. Add-on authors who try to test e10s without a sandbox won't be caught by surprise when we enable the sandbox. This is only a concern if it takes a while to enable the sandbox, but it is a possibility.

Benjamin, where do you think the right places would be to put this sort of code? We won't be able to catch everything, but it would be nice if we could cover the most common uses by instrumenting a small number of places.
Flags: needinfo?(benjamin)

Comment 1

5 years ago
I'm mainly worried about whether you could do this and the browser would keep running, at least for now. We certainly use files in the app directory in the content process.

If we were going to do this, we'd have to whitelist the app directory, and then we can use the following chokepoints:

* nsLocalFile::FillStatCache (nsLocalFileUnix) or nsLocalFile::ResolveAndStat (nsLocalFileWin)

Although nsLocalFile::FillStatCache currently only returns bool, so we might need to change its error-reporting a bit.

Those places should definitely report something to the error console, not just via a NS_WARNING or returning a failure code.

* OS.File methods. These are in JS and should be easy to make throw or fail and report to the console.

AFAIK currently the child process doesn't even know the profile location, which is good. But if it does, we should stop doing that and probably make any requests for it fail/report console errors.
Flags: needinfo?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> I'm mainly worried about whether you could do this and the browser would
> keep running, at least for now. We certainly use files in the app directory
> in the content process.

These should be remoted through PRemoteOpenFile or something similar, shouldn't they?  (Key word being “should”; see bug 930258).

Comment 3

5 years ago
"Should" in what timeframe?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> "Should" in what timeframe?

That is a good question to which I don't have the answer.
Flags: needinfo?(sstamm)
I doubt we could complete remoting file access for the app directory in the next week or two unless someone steps up and is willing to own it.
Flags: needinfo?(sstamm)
No longer blocks: e10s-m1
I wrote some code to prototype this. The patch adds a main_process_scriptable_only flag that can go on IDL interfaces. This flag means that JS can use objects of this interface only in the main process. I added the flag to nsIFile to see what would happen.

I haven't looked into it too deeply, but all the problems appear to be test related. Lots of tests use TmpD. Lots of tests also use the MockFilePicker in a way that causes us to create nsIFiles in JS. I think with some effort we should be able to land this, but I'm not sure I'll have time to work on it.
Bill is going to change this patch to log warnings instead of returning errors, then file a follow up bugs to return errors and fix tests. The warnings will be caught by AMO reviewers, so add-on developers will be prevented from using XPCOM services in the content process before our sandbox rules actually enforce this policy.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
There may be some components that can be made Module::MAIN_PROCESS_ONLY without breaking anything that doesn't need to be broken — nsDownloader comes to mind, now that bug 1034143 is done, but it may not be the only one.
Note that disabling the nsIDeviceContextSpec component for the content process would break printing for all platforms. Disabling nsIPrintingPromptService for the content process would break printing for OS X.
Posted patch patchSplinter Review
I changed this to only emit a warning. The idea here is to make sure that add-ons that use nsIFile in a content process don't pass review.

The patch removes the __LOCATION__ property from JSM globals in content processes since it's exposed as an nsIFile. Mossop, can you confirm that that won't break module loading in Jetpack?
Attachment #8574166 - Attachment is obsolete: true
Attachment #8578955 - Flags: review?(mrbkap)
Attachment #8578955 - Flags: feedback?(dtownsend)
(In reply to Bill McCloskey (:billm) from comment #10)
> The patch removes the __LOCATION__ property from JSM globals in content
> processes since it's exposed as an nsIFile. Mossop, can you confirm that
> that won't break module loading in Jetpack?

I can't see the SDK using that anywhere, I know we use __URI__ though so it should be fine.
Attachment #8578955 - Flags: feedback?(dtownsend) → feedback+
Comment on attachment 8578955 [details] [diff] [review]
patch

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

I don't know if I have the authority to review the xpcom/reflect bits, but this looks good (and pretty straightforward!) to me.

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp
@@ +240,5 @@
> +        nsAutoString filename;
> +        uint32_t lineno = 0;
> +        nsJSUtils::GetCallingLocation(cx, filename, &lineno);
> +        nsCOMPtr<nsIScriptError> error(do_CreateInstance(NS_SCRIPTERROR_CONTRACTID));
> +        error->Init(NS_LITERAL_STRING("Use of nsIFile in content processes is deprecated."),

Can you use aInfo->GetName instead of nsIFile here?
Attachment #8578955 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/1652874e2d97
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1187410

Comment 15

4 years ago
(In reply to Chris Peterson [:cpeterson] from comment #7)
> Bill is going to change this patch to log warnings instead of returning
> errors, then file a follow up bugs to return errors and fix tests. 

Has this bug been filed?
Flags: needinfo?(wmccloskey)
OS: Linux → All
Hardware: x86_64 → All
No, just filed bug 1189064.
Flags: needinfo?(wmccloskey)
You need to log in before you can comment on or make changes to this bug.