Closed Bug 1214305 Opened 4 years ago Closed 4 years ago

Enable interception of secure upgraded channels in e10s mode

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s + ---
firefox46 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(11 files, 2 obsolete files)

13.12 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
4.00 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
7.99 KB, patch
jdm
: review+
Details | Diff | Splinter Review
1.37 KB, patch
jdm
: review+
Details | Diff | Splinter Review
1.02 KB, patch
jdm
: review+
Details | Diff | Splinter Review
5.19 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
4.26 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
1.01 KB, patch
jdm
: review+
Details | Diff | Splinter Review
1.08 KB, patch
keeler
: review+
Details | Diff | Splinter Review
1.34 KB, patch
billm
: review+
Details | Diff | Splinter Review
3.48 KB, patch
keeler
: review+
Details | Diff | Splinter Review
This is adding e10s support to bug 1198394.

I originally started to implementing this by getting the child process to ask the parent whether a channel needs to be upgraded before doing the interception but that has turned into a very complex implementation, that after a while we decided is less than ideal.

I'm going to work on another possible idea, which is mirroring the HSTS information in the child process, and decide whether an upgrade is needed directly in the child and performing a redirect from the child side.  This will have the extra benefit of keeping the e10s and non-e10s paths similar.  Also, since we can only start talking to the network in the parent process, we won't be trusting the child to not lie about whether a channel needs to be upgraded, since when we decide to finally go to the network, the parent process will check whether an upgrade is needed just like today, so the HSTS mirrored information in the child process will only be used for determining whether to dispatch the FetchEvent on the upgraded channel or not.
Depends on: 1215723
Assignee: nobody → ehsan
This is needed to ensure that the ServiceWorkerManager uses the
correct URI for non-subresource requests.  Note that we're relying
on the property that non-secure non-subresource requests can never
be intercepted, so we don't need to check the request type explicitly.
Attachment #8682088 - Flags: review?(josh)
This ensures that FetchEvent.request.url will have the correct value
for secure upgraded channels.
Attachment #8682089 - Flags: review?(josh)
This is OK from a security perspective, since this decision only affects
whether the channel will be intercepted with the secure URI in the child
process.  If the intercepting service worker decides to fall back to an
actual network request, we send the request to the parent process with
the original pre-upgrade URI, and the parent process will still be in
charge of whether a network visible HTTP request should be upgraded.
Attachment #8682091 - Flags: review?(mcmanus)
Attachment #8682092 - Flags: review?(overholt)
Attachment #8682092 - Attachment is obsolete: true
Attachment #8682092 - Flags: review?(overholt)
(In reply to [Vacation: Nov 3-19] from comment #10)
> <https://treeherder.mozilla.org/#/jobs?repo=try&revision=22df212c58e7>

This was leaking pretty badly.  I fixed the leak and repushed: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3e3a434b80d>

I think there are a few other real test failures there too, but I won't get a chance to look at them...  Josh, I'd appreciate if you would take a look, please.  Note that bug 1215723 on its own was green on the try server (sorry, don't have a link handy.)
Attachment #8682086 - Flags: review?(mcmanus) → review?(jduell.mcbugs)
Attachment #8682085 - Flags: review?(mcmanus) → review?(jduell.mcbugs)
Attachment #8682090 - Flags: review?(mcmanus) → review?(jduell.mcbugs)
Attachment #8682091 - Flags: review?(mcmanus) → review?(jduell.mcbugs)
Comment on attachment 8682087 [details] [diff] [review]
Part 3: Add a nsIInterceptedChannel.secureUpgradedChannelURI helper

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

::: netwerk/protocol/http/InterceptedChannel.cpp
@@ +319,5 @@
>  
> +NS_IMETHODIMP
> +InterceptedChannelChrome::GetSecureUpgradedChannelURI(nsIURI** aURI)
> +{
> +  return mChannel->GetURI(aURI);

A comment here explaining why we don't need to actually return a different URI would be useful.
Attachment #8682087 - Flags: review?(josh) → review+
Attachment #8682088 - Flags: review?(josh) → review+
Attachment #8682089 - Flags: review?(josh) → review+
Attachment #8682093 - Flags: review?(josh) → review+
Attachment #8682085 - Flags: review?(jduell.mcbugs) → review+
Attachment #8682086 - Flags: review?(jduell.mcbugs) → review+
Attachment #8682090 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 8682091 [details] [diff] [review]
Part 7: Decide in the child process whether an intercepted channel should go through a secure upgrade

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

All looks OK to me.
Attachment #8682091 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/49b930683c026adb4c3fd8b7ccf782db97abb7eb
Bug 1214305 - Part 0: Ensure site security service is initialized before trying to use DataStorage via IPC.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b85acbe7be7111f06f44ad36a324c9e10415483
Bug 1214305 - Part 1: Refactor the logic for querying whether a connection should go through a secure upgrade into NS_ShouldSecureUpgrade; r=mcmanus

https://hg.mozilla.org/integration/mozilla-inbound/rev/be072bba15fc640071e76c99df7aee5e8b90e07d
Bug 1214305 - Part 2: Refactor the logic for obtaining the secure upgraded URI into HttpBaseChannel; r=mcmanus

https://hg.mozilla.org/integration/mozilla-inbound/rev/347cd0f8f1e3cad2ef816d600310dd8100820ece
Bug 1214305 - Part 3: Add a nsIInterceptedChannel.secureUpgradedChannelURI helper; r=jdm

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3e4f256d4656acd316a6af9a5a9b1e57acce68
Bug 1214305 - Part 4: Use the secure upgraded channel URI in ServiceWorkerManager::PrepareFetchEvent; r=jdm

https://hg.mozilla.org/integration/mozilla-inbound/rev/88ab26108f2de6cb3786208f86a8b570085b9b7c
Bug 1214305 - Part 5: Use the secure upgraded channel URI in FetchEventRunnable::Init; r=jdm

https://hg.mozilla.org/integration/mozilla-inbound/rev/226c83ca9a2aa4c16e42603d54c2636087e985b2
Bug 1214305 - Part 6: Use a non-IPC redirect for synthesized upgraded responses to ensure the response URL is correctly propagated; r=mcmanus

https://hg.mozilla.org/integration/mozilla-inbound/rev/e926606aefbf83605f7125787a2b6cade59be193
Bug 1214305 - Part 7: Decide in the child process whether an intercepted channel should go through a secure upgrade; r=mcmanus

https://hg.mozilla.org/integration/mozilla-inbound/rev/82929be36e95db2e6dfa11fbd7e3d5f1ee9f3e52
Bug 1214305 - Part 8: Enable secure upgrade service worker tests on e10s; r=jdm
Not blocking v1 because its e10s only.
Assignee: ehsan → josh
Status: NEW → ASSIGNED
I spent my time looking into the failing output of `./mach mochitest --e10s devtools/client/webconsole/test/browser_bug1045902_console_csp_ignore_reflected_xss_message.js` . The `content.location = TEST_URI` line appears to fail in the child process, but when I stepped through it in the debugger two things happened:
* I continued to see the following test failure while the child was paused:
10 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_bug1045902_console_csp_ignore_reflected_xss_message.js | A promise chain failed to handle a rejection:  - at chrome://mochitests/content/browser/devtools/client/webconsole/test/browser_bug1045902_console_csp_ignore_reflected_xss_message.js:45 - Error: cross-process JS call failed
* when I stepped completely through the operation, the child never returned a non-NS_OK/true value at any point

Setting the ipc timeout to 0 (indefinite) didn't change this result.
Assignee: josh → ehsan
(In reply to Ben Kelly [:bkelly] from comment #18)
> Not blocking v1 because its e10s only.

I've seen this a few times in service worker bugs. FYI e10s is rolling out to 45 beta to a large number of users. If the service worker team has e10s specific show stoppers can we get those tracked to 45 and owned?
Flags: needinfo?(bkelly)
(In reply to Jim Mathies [:jimm] from comment #21)
> I've seen this a few times in service worker bugs. FYI e10s is rolling out
> to 45 beta to a large number of users. If the service worker team has e10s
> specific show stoppers can we get those tracked to 45 and owned?

There is no way these are getting fixed in 45 given there is just over a week to when 45 merges to aurora.  The service worker team is going to be focusing on how to fix our e10s support at mozlando.  The solution, however, is likely to take longer than a single release cycle.
Flags: needinfo?(bkelly)
Move to SW component for easier tracking.
Component: Networking → DOM: Service Workers
Blocks: e10s-swbeta
Comment on attachment 8690108 [details] [diff] [review]
Part 0: Ensure site security service is initialized before trying to use DataStorage via IPC

This is left-over from bug 1215723.
Attachment #8690108 - Flags: review?(dkeeler)
(In reply to Josh Matthews [:jdm] from comment #20)
> I spent my time looking into the failing output of `./mach mochitest --e10s
> devtools/client/webconsole/test/
> browser_bug1045902_console_csp_ignore_reflected_xss_message.js` . The
> `content.location = TEST_URI` line appears to fail in the child process, but
> when I stepped through it in the debugger two things happened:
> * I continued to see the following test failure while the child was paused:
> 10 INFO TEST-UNEXPECTED-FAIL |
> devtools/client/webconsole/test/
> browser_bug1045902_console_csp_ignore_reflected_xss_message.js | A promise
> chain failed to handle a rejection:  - at
> chrome://mochitests/content/browser/devtools/client/webconsole/test/
> browser_bug1045902_console_csp_ignore_reflected_xss_message.js:45 - Error:
> cross-process JS call failed
> * when I stepped completely through the operation, the child never returned
> a non-NS_OK/true value at any point
> 
> Setting the ipc timeout to 0 (indefinite) didn't change this result.

OK, I finally figured this out.

The failure happens here: <https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/test/browser_bug1045902_console_csp_ignore_reflected_xss_message.js#45>.  Here, |content| is a CPOW.  The setter goes through WrapperOwner::set in the parent process which fails here: <https://dxr.mozilla.org/mozilla-central/source/js/ipc/WrapperOwner.cpp#553>.  The reason SendSet() fails is that the transaction has been canceled <https://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#835>.  I tracked that down to <https://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#881> in the child process.  Basically what's happening is that we're processing a sync message from the parent, and with my changes here, as part of that we send a message to the parent from the child which gets canceled, causing the sync CPOW call to fail.

Setting location off of a timeout bypasses this.
It would be much better to avoid the use of CPOWs entirely since we want to remove them eventually. In this case, gBrowser.loadURI(TEST_FILE) would work (without the timeout). Also, it would be better to use BrowserTestUtils to wait for the load, since browser.addEventListener also uses CPOWs.
Attachment #8703184 - Attachment is obsolete: true
Attachment #8703184 - Flags: review?(ttromey)
Comment on attachment 8703190 [details] [diff] [review]
Part 9: Avoid using CPOWs in browser_bug1045902_console_csp_ignore_reflected_xss_message.js

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

Thanks!
Attachment #8703190 - Flags: review?(wmccloskey) → review+
Comment on attachment 8690108 [details] [diff] [review]
Part 0: Ensure site security service is initialized before trying to use DataStorage via IPC

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

LGTM, but consider the comment below.

::: dom/ipc/ContentParent.cpp
@@ +2677,5 @@
>  ContentParent::RecvReadDataStorageArray(const nsString& aFilename,
>                                          InfallibleTArray<DataStorageItem>* aValues)
>  {
> +    // Ensure the SSS is initialized before we try to use its storage.
> +    nsCOMPtr<nsISiteSecurityService> sss = do_GetService("@mozilla.org/ssservice;1");

Right now we only have one consumer of DataStorage, so this works as expected. If this ever changes, we'll have to do something like map the filename to which service actually uses it, which feels a bit clunky. I think we can probably cross that bridge if/when we get to it, though.

Also, this will block on reading the backing file. Not sure if that's a concern or not.
Attachment #8690108 - Flags: review?(dkeeler) → review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #30)
> ::: dom/ipc/ContentParent.cpp
> @@ +2677,5 @@
> >  ContentParent::RecvReadDataStorageArray(const nsString& aFilename,
> >                                          InfallibleTArray<DataStorageItem>* aValues)
> >  {
> > +    // Ensure the SSS is initialized before we try to use its storage.
> > +    nsCOMPtr<nsISiteSecurityService> sss = do_GetService("@mozilla.org/ssservice;1");
> 
> Right now we only have one consumer of DataStorage, so this works as
> expected. If this ever changes, we'll have to do something like map the
> filename to which service actually uses it, which feels a bit clunky. I
> think we can probably cross that bridge if/when we get to it, though.

Yeah...  I can't think of a clean generic way to do this.  I agree that we should probably cross that bridge when we need to, but the solution would probably end up being something ad-hoc like this again.  :(

> Also, this will block on reading the backing file. Not sure if that's a
> concern or not.

Good question.  Given that the content process needs to block on this being finished, my immediate reaction was to do the simple synchronous thing.  If it turns out to be an issue then we'd need to address it, but I prefer to wait for a real world performance issue, since chances are that the parent process has already initialized this data for its own purposes...
We rely on profile-before-change to clean up sDataStorages, but that notification is not dispatched in the child process, so we hold all DataStorage objects alive during shutdown.  I believe that should fix the leaks: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab91aa00ee8c>
Flags: needinfo?(ehsan)
Comment on attachment 8703755 [details] [diff] [review]
Part 10: Clean up global DataStorage references in the child process

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

LGTM.
Attachment #8703755 - Flags: review?(dkeeler) → review+
(historical note/nit: looks like parts 1, 2, 6, and 7 landed with "r=mcmanus" in the commit message, but they really had r=jduell)
Depends on: 1300464
You need to log in before you can comment on or make changes to this bug.