997325 - Consider disabling XPCOM services in child process that conflict with sandboxing

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 1

[Benjamin Smedberg]

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.

Comment 2

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

(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

[Benjamin Smedberg]

"Should" in what timeframe?

Comment 4

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

(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.

Comment 5

[Sid Stamm [:geekboy or :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.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Implement main process scriptable only flag in XPIDL

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.

Comment 7

[Chris Peterson [:cpeterson]]

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.

Comment 8

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

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.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

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?

Comment 11

[Dave Townsend [:mossop]]

(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.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1652874e2d97

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1652874e2d97

Comment 15

[Florian Bender]

(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?

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

No, just filed bug 1189064.