945268 - Enable CSP tests on mutltiprocess Gecko

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Garrett Robinson [:grobinson]]

Many of the CSP tests use the "http-on-modify-request" observer to check if the content policies are functioning as expected. This observer was disabled in multiprocess Gecko (B2G and e10s) in bug 806753, and this condition was maintained in bug 827269.

We currently disable the CSP tests on these platforms. This is unnacceptable on B2G, especially since CSP is an important of its security model. It is also unacceptable if we want e10s to eventually be the default process mode of Firefox.

Comment 1

[Garrett Robinson [:grobinson]]

For the reasons described in the bugs linked in Comment 1, it would be difficult to implement the http-on-modify request observer in the child. It causes a performance hit (extra IPC roundtrip). It increases the attack surface in the child.

The two primary consumers of this observer topic are addons and the CSP tests. If addons are to be run in the parent (which appears to be the current trend), then adding this observer back to the child just for the CSP tests does not seem worth the drawbacks. Additionally, these tests don't really need to modify the request - they just need to be notified that it occurred.

An alternative would be to create a proxy for the observer topic in the parent using SpecialPowers. Currently observers are created in specialpowers.js, which runs in the content process. The observe function is a callback defined in the Mochitest. It is wrapped and given to an observer. Note that SpecialPowers also has a component defined in SpecialPowersObserver.js, which runs in the parent. It fulfills requests from the SpecialPowers content script that it cannot fulfill (because it runs in the child/content process) [0].

One could theoretically special case the "http-on-modify-request" observer so we actually create the observer in the parent, and proxy its observer down to the one in the test.

Another option would be to create a new "read-only" version of "http-on-modify-request", e.g. "http-on-notify-request".

[0] https://developer.mozilla.org/en-US/docs/SpecialPowers#Adding_new_APIs

Comment 2

[Garrett Robinson [:grobinson]]

Attachment: 945268-multiprocess-csp-tests-wip-1.patch

This is a WIP patch that takes the second approach described in the above comment, and adds a read-only "http-on-notify-request" observer. This observer extracts the URI of the request and sends it as the aData parameter for nsIObserver.observer(), and makes aSubject null (so it cannot be modified by the observer).

Minor modifications are required for the CSP tests, but their general mechanism does not change. This patch only changes test_CSP.html. If you apply this patch and run "mach mochitest-plain --e10s content/base/test/csp/test_CSP.html", all the tests pass :-)

Comment 3

[Garrett Robinson [:grobinson]]

Attachment: 945268-multiprocess-csp-tests-wip-2.patch

This is an alternative approach, basically the first approach described in the "implementation options" comment above. An "http-on-modify-request" is registered in the component of SpecialPowers running in the parent. When it is called to observe, it extracts the URI of the request (all that we need for the CSP tests), and sends an async message via the message manager to the component of SpecialPowers running in the child. The child then triggers a different observer, unique to special powers, to inform the tests of a request that was made.

It might be overkill to have the content process fire another observer. We could probably just have the CSP tests listen for the async message sent to the child instead. I am not doing that in this patch because my goal here was to minimize the modifications to the CSP tests.

I prefer this approach because it doesn't require any changes to Necko and localizes the special case observer to SpecialPowers, so it can only be used by tests.

Again, only test_CSP.html has been modified to use the new code. It passes under e10s.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8343482 [details] [diff] [review]
945268-multiprocess-csp-tests-wip-1.patch

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

Seems like a good direction.  You should get review from necko peers on this one.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1013,5 @@
>    // notify "http-on-modify-request" observers
>    //CallOnModifyRequestObservers();
> +  // XXX: is this the best place for this (the request hasn't been fired yet -
> +  // are we sure it will be?)
> +  gHttpHandler->OnNotifyRequest(this);

This is probably a good spot, but I defer to the network folks.  Makes coherent sense for previous consumers of http-on-modify-request and should be pretty close to the last chance to know it's gonna happen before it does.

Comment 5

[Garrett Robinson [:grobinson]]

Comment on attachment 8343863 [details] [diff] [review]
945268-multiprocess-csp-tests-wip-2.patch

feedback?'ing rcampbell to see if the "Special Powers-only" approach is sane.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8343863 [details] [diff] [review]
945268-multiprocess-csp-tests-wip-2.patch

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

approach looks reasonable to me, but the commented out dumps should probably go.

::: testing/specialpowers/components/SpecialPowersObserver.js
@@ +69,5 @@
>          }
>          break;
>  
> +      case "http-on-modify-request":
> +        //dump("** got http-on-modify-request\n");

this line should be removed.

@@ +73,5 @@
> +        //dump("** got http-on-modify-request\n");
> +        if (aSubject instanceof Ci.nsIChannel) {
> +          let uri = aSubject.URI.spec;
> +          //dump("   uri is " + uri + "\n");
> +          //Services.obs.notifyObservers(null, "specialpowers-http-notify-request", uri);

these too.

Comment 7

[Garrett Robinson [:grobinson]]

Attachment: 945268-multiprocess-csp-tests-1.patch

Cleaned up version of the wip-2 patch. If this is r+'ed, I'll fix the rest of the CSP tests to use the proxied observer topic and post that patch here. Is there anybody else I should get a review from?

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8344824 [details] [diff] [review]
945268-multiprocess-csp-tests-1.patch

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

r+ with a greenish try push. Should be fine with Sid's review.

Comment 9

[Garrett Robinson [:grobinson]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=4545bbda467b

Note that you can't test e10s on try atm (but on IRC markh said that should be fixed soon). This try run just tests regular single-process Firefox (and B2G). Only test_CSP.html is rewritten (I'm waiting on confirmation from Sid before I rewrite the rest of the CSP tests).

After rewriting the rest of the tests, I'm going to re-enable the CSP tests that are currently disabled in testing/mochitest/b2g.json and re-push to try. If they work on B2G, they should should work on multiprocess generally. That will also let me move forward with closing Bug 858787.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8344824 [details] [diff] [review]
945268-multiprocess-csp-tests-1.patch

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

Yeah, looks good to me (pretty straightforward edits to the mochitest).  Do the rest.

Comment 11

[Garrett Robinson [:grobinson]]

Converted the rest of the tests, ran into an unexpected issue with test_nonce_source.html. Investigating now.

Comment 12

[Garrett Robinson [:grobinson]]

Attachment: 945268-multiprocess-csp-tests-2.patch

Re-flagging for review because I made significant changes while resolving the issues I came across yesterday. Here's a summary:

Previous patch registered an event listener in the SpecialPowers constructor. The goal was to listen for events sent from the parent via the Message Manager and use them to notify observers in the child. This assumed that this constructor was only called once (not sure why I assumed that). What ended up happening is, the SpecialPowers constructor is called numerous times, and each created its own listener w/ callback. So a single observer notification triggered by a request in the parent turned into several (5-6) observer notifications for the same event in the child. This broke the nonce-source test because of the way it counts requests, and more importantly is not a viable state of affairs. It is not reasonable to write tests based on the assumption that a single request can trigger an unknown number of corresponding notifications.

This new patch registers/unregisters the event listener as part of SpecialPowers.addObserver/removeObserver. This enables the proxy to exactly echo the parent notifications to the child.

All of the CSP tests continue to pass on single-process Firefox. On multi-proces, they all pass except for "content/base/test/csp/test_CSP_evalscript_getCRMFRequest.html". Note that this test never used the missing http-on-modify-request observer - it is failing because window.crypto.getCRMFRequest is not implemented in e10s. Since there has been debate over whether it is worth engineering effort to port this and other similarly non-standard window.crypto functions to e10s (or eliminate them entirely), I did not think that should be a blocker for this bug (but can be addressed in a follow-up).

As a result, I have enabled all of the CSP tests *except* "test_CSP_evalscript_getCRMFRequest.html" in testing/mochitest/b2g.json. So now all but that one CSP test should run on b2g try runs. As soon as e10s try runs exist, I will make sure the CSP test are running there as well.

Comment 13

[Garrett Robinson [:grobinson]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=59022a45a0d1

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8346274 [details] [diff] [review]
945268-multiprocess-csp-tests-2.patch

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

CSP test bits *look* good, but I'm a little concerned about the test breakage/timeouts on try B2G emulator mochitests:
https://tbpl.mozilla.org/php/getParsedLog.php?id=31846041&tree=Try

r=me if you can fix them with trivial updates to the tests, otherwise please fix and send back for another review.

::: content/base/test/csp/test_nonce_source.html
@@ +54,5 @@
>          ok(/_bad/.test(testid), "Blocked URI with testid " + testid);
>          ranTests(1);
>        } catch (e) {
>          // if the subject is blocked inline, data will be a violation msg (defined at the top of contentSecurityPolicy.js)
> +        //dump("** exception in csp-on-violate-policy: " + e + "\n");

nit: commented-out dump, please remove.

::: testing/specialpowers/components/SpecialPowersObserver.js
@@ +71,5 @@
>  
> +      case "http-on-modify-request":
> +        if (aSubject instanceof Ci.nsIChannel) {
> +          let uri = aSubject.URI.spec;
> +          this._sendAsyncMessage("specialpowers-http-notify-request", { uri: uri });

This is an important difference from http-on-modify-request: the data sent is a URI spec, not the nsIURI object.  It's all we need for the CSP tests, but is it harder to proxy more data?

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8346274 [details] [diff] [review]
945268-multiprocess-csp-tests-2.patch

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

lgtm with appropriate try run and verifying that this doesn't break b2g.

(hi garrett!)

Comment 16

[Garrett Robinson [:grobinson]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #14)
> Comment on attachment 8346274 [details] [diff] [review]
> 945268-multiprocess-csp-tests-2.patch
> 
> Review of attachment 8346274 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> CSP test bits *look* good, but I'm a little concerned about the test
> breakage/timeouts on try B2G emulator mochitests:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31846041&tree=Try
> 
> r=me if you can fix them with trivial updates to the tests, otherwise please
> fix and send back for another review.

Looking into those now. My assumption was that if they work on e10s, they would work on b2g. Clearly not the case.

> ::: testing/specialpowers/components/SpecialPowersObserver.js
> @@ +71,5 @@
> >  
> > +      case "http-on-modify-request":
> > +        if (aSubject instanceof Ci.nsIChannel) {
> > +          let uri = aSubject.URI.spec;
> > +          this._sendAsyncMessage("specialpowers-http-notify-request", { uri: uri });
> 
> This is an important difference from http-on-modify-request: the data sent
> is a URI spec, not the nsIURI object.  It's all we need for the CSP tests,
> but is it harder to proxy more data?

You can't send an object reference (like an nsIURI) from the parent process to the content process. Anything sent from the parent must be JSON-serializable (i.e. only use primitive types). [0] It would not be hard to extract other attributes of interest as long as they can be converted to JS primitives. For a more involved example of this pattern, check out SPConsoleListener.prototype in specialpowersAPI.js.

[0] https://developer.mozilla.org/en-US/docs/The_message_manager#Messages

Comment 17

[Garrett Robinson [:grobinson]]

I narrowed the issue down on B2G to the failure of the child process SpecialPowers to receive messages from the parent process. This was tracked in Bug 951895 and we now have a patch that resolves this issue. From some localized testing, this appears to have resolved *that* problem. I am able to get test_nonce_source.html (for example) to pass in the B2G emulator!

However, there appear to be some other minor problems. First up is in test_CSP.html. Many more tests pass with the patch from 951895, but there is still one test that does not get run - media_bad. I am investigating this further.

After this issue is resolved, I will continue trying the CSP tests on the B2G emulator to see if I can get them all passing. After that, I will do another try run, which should be green :D

Comment 18

[Garrett Robinson [:grobinson]]

Attachment: debug-incomplete-csp-tests

Modifications to test_CSP.html to log the tests that fail to run on B2G. Currently:

** incomplete tests **
media_good
media_bad
media_spec_compliant_good
media_spec_compliant_bad

Comment 19

[Garrett Robinson [:grobinson]]

Attachment: 945268-multiprocess-csp-tests.patch

Carrying over r+'s from sstamm and rcampbell. This patch fixes the failing media_* tests in test_CSP.html.

These failures were caused by a different default setting for "media.preload.default" on Desktop vs. mobile (Android and B2G). On mobile, the default preload setting was to do no preloading, while on desktop preloading is on by default. That means that on mobile, no data is loaded (the request for the media element URI is not made) until the user interacts with the corresponding widget on the page. This explains why the media tests were failing to run (neither passing, or failing, but timing out). The requests were not being made in the first place, so neither a request was observed or a violation was reported.

I fixed this by setting the pref back to the desktop Firefox default in test_CSP.html's pushPrefEnv. This pref's setting doesn't affect CSP, so changing the pref does not change the effectiveness of this test on mobile.

I have confirmed that all of the CSP tests (except for test_CSP_evalscript_getCRMFRequest, see Comment 12) are now passing in the B2G emulator.

Try: https://tbpl.mozilla.org/?tree=Try&rev=1d69398c03a6

Comment 20

[Garrett Robinson [:grobinson]]

Try looks pretty good, but test_csp_redirects unexpectedly timed out on B2G. This didn't happen when I ran the test locally earlier - trying to reproduce now.

Comment 21

[Garrett Robinson [:grobinson]]

Reproduced locally (actually, it was happening earlier, but I missed it because it was a timeout, not a test failure). I'm going to dig into this more tomorrow, but here's the relevant logcat output:

E/Sandbox (  339): install_syscall_filter() failed
I/Gecko   (  295): 
I/Gecko   (  295): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv
I/Gecko   (  295):

Comment 22

[Ian Melven :imelven]

(In reply to Garrett Robinson [:grobinson] from comment #21)
> Reproduced locally (actually, it was happening earlier, but I missed it
> because it was a timeout, not a test failure). I'm going to dig into this
> more tomorrow, but here's the relevant logcat output:
> 
> E/Sandbox (  339): install_syscall_filter() failed
> I/Gecko   (  295): 
> I/Gecko   (  295): ###!!! [Parent][MessageChannel] Error: Channel error:
> cannot send/recv
> I/Gecko   (  295):

that looks like the seccomp stuff ? cc'ing jld and kang.

Comment 23

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

(In reply to Ian Melven :imelven from comment #22)
> (In reply to Garrett Robinson [:grobinson] from comment #21)
> > E/Sandbox (  339): install_syscall_filter() failed

This message usually indicates that we're running on a system without seccomp-bpf support, which is currently everything except Geeksphones (including the emulator, pending bug 941375).  More generally, it means that we failed to enable sandboxing, so this probably isn't sandbox-related.  Which device was this run on?

Also, "eng" builds use --enable-content-sandbox-reporter, so if a sandbox violation occurs on one then there should be a line in logcat indicating that.

Comment 24

[Garrett Robinson [:grobinson]]

> Which device was this run on?

Emulator

Comment 25

[Garrett Robinson [:grobinson]]

Attachment: 945268-multiprocess-csp-tests.patch

Fixed the test_csp_redirects timeout. It needed the same pref flipped as test_CSP.html did in order for the media* tests to run.

Try: https://tbpl.mozilla.org/?tree=Try&rev=02770bb1b232

Comment 26

[Garrett Robinson [:grobinson]]

Try looks good. The seccomp stuff from Comment 21 appears to have been a red herring.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6520037420c1

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6520037420c1