1408708 - (CVE-2018-5106) Developer tool's traffic routes through service worker

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Jun (Request needinfo for comment, I ignore other updates)]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

1. Go to https://vuln.shhnjk.com/swnav.html
2. Now go to https://vuln.shhnjk.com/iframer.php?url=//www.bing.com/images/search?q=Edge
3. Open developer tools and click on any error. 


Actual results:

Resources required to show error details are fetched through service worker registered on https://vuln.shhnjk.com/swnav.html. Leaking cross-origin information.

Attaching PoC  video since it might be difficult to repro.


Expected results:

Developer tool's traffic should not go thought service worker.

Comment 1

[Jun (Request needinfo for comment, I ignore other updates)]

Oops, video file was too large. Uploaded unlisted video below.

https://youtu.be/yU4SybqSBsI

Comment 2

[Jun (Request needinfo for comment, I ignore other updates)]

Okay, this issue is caused by Style Editor in Developer tool. You only need to open Style Editor to trigger the bug.

Comment 3

[Frederik Braun [:freddy]]

I can confirm that due to this bug the ServiceWorker sees the URLs for resources in cross-origin iframes.
These resources are fetched by the DevTools and should bypass the ServiceWorker.

I'm not sure about the severity, trying sec-high first.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I believe this is a devtools bug.  They should be setting nsIChannel::LOAD_BYPASS_SERVICE_WORKER on their network requests.

Jason, do you know who on your team would be the best person to look at this?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Or alternatively, the network requests need to be made from the iframes document context and not the top window's context.  That will ensure the iframe's service worker (if any) sees the request but not the top level service worker.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Adding some more devtools folks.  I'm not sure who the right person to ask about this is.

Comment 7

[Jason Laster [:jlast]]

I think Alex Poirot would have some context here

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

This seems to fix it, but to be honest I don't quite follow the issue.
Aren't we going to be confused in some other cases, do we *always* expect to bypass service workers in all type of ressources displayed by DevTools?

And I imagine we should review everything to ensure we don't do another request like this.
Many codes now use fetch(), but we may still have custom request code somewhere else...

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

After thinking about this, comment 5 might be better.  Ideally we would use the same service worker used to load the resources for content.

Can you tell if the top level window is being passed instead of the iframe window for any of these fetches?

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Ok, that makes more sense to me.
Here we ensure using StyleSheet document (here an iframe) intead of top level document we debug.

Still, we would have to review all our code to ensure we don't have this issue somewhere else.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8920515 [details] [diff] [review]
patch v2

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

::: devtools/server/actors/stylesheets.js
@@ +153,5 @@
> +  /**
> +   * StyleSheet's window.
> +   */
> +  get ownerWindow() {
> +    return this.ownerDocument.window;

Shouldn't this be this.ownerDocument.defaultView instead?

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Patrick Brosset <:pbro> from comment #11)
> Shouldn't this be this.ownerDocument.defaultView instead?

Good catch!


* So. It looks like some other code would suffer from the same issue:
http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/devtools/server/actors/source.js#382-391
      let win = this.threadActor._parent.window;
      ...
        let webNav = win.QueryInterface(Ci.nsIInterfaceRequestor)
                        .getInterface(Ci.nsIWebNavigation);
        let channel = webNav.currentDocumentChannel;
        principal = channel.loadInfo.loadingPrincipal;
      ...
      let sourceFetched = fetch(this.url, {
        principal,
        ...
      });

Also, is principal enough?
When we pass this `principal` attribute we end up setting it like this:
  channelOptions.loadingPrincipal = principal;

When we pass a `window`, we also do this:
  channel.loadGroup = window.QueryInterface(Ci.nsIInterfaceRequestor)
                          .getInterface(Ci.nsIWebNavigation)
                          .QueryInterface(Ci.nsIDocumentLoader)
                          .loadGroup;

* We may also have issue on source map, but is sourcemap suffering from the same issue??
http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/devtools/server/actors/utils/TabSources.js#466
  let fetching = fetch(absSourceMapURL, { loadFromCache: false })

* And may be also the request replay feature:
http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/devtools/server/actors/webconsole.js#1668-1679
    let doc = this.window.document; // this.window is the top level document...
    let channel = NetUtil.newChannel({
      uri: NetUtil.newURI(url),
      loadingNode: doc,
      securityFlags: Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
      contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER
    });
    channel.QueryInterface(Ci.nsIHttpChannel);
    channel.loadGroup = doc.documentLoadGroup;

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Can you include a regression test for this?  Seems you could register a SW for a nested cross-origin iframe that populates the body with something and then verify both content and devtools see the same value?

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

With a test involving service workers, but there is failures on try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=94edc75130de0fd69d21a40e505a052824133990&selectedJob=138924592
(not sure it is related to the new test, it may be because of the fix)

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Patrick, I had to tweak the cases where it isn't easy to retrieve stylesheet's document.
And also contributed a non-trivial test to cover this.

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8922000 [details] [diff] [review]
patch v5

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

::: devtools/client/styleeditor/test/bug_1405342_serviceworker_iframes.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +<head>
> + <title>Bug 1405342</title>

Please add <meta charset="utf-8"> to avoid warnings in the console

::: devtools/client/styleeditor/test/iframe_service_worker.js
@@ +1,1 @@
> +onfetch = function(event) {

ESLint might need "use strict"; to be added at the top of this file.

::: devtools/client/styleeditor/test/iframe_with_service_worker.html
@@ +1,1 @@
> +Iframe loading a stylesheet via a service worker

Please add <meta charset="utf-8"> here too

@@ +1,3 @@
> +Iframe loading a stylesheet via a service worker
> +<script>
> +function waitForActive(swr) {

I think ESLint is going to emit a warning here because "use strict"; is missing.

::: devtools/server/actors/stylesheets.js
@@ +222,5 @@
> +
> +    // When the style is imported, `styleSheet.ownerNode` is null,
> +    // so retrieve the topmost parent style sheet which has an ownerNode
> +    let parentStyleSheet = styleSheet;
> +    while (parentStyleSheet.parentStyleSheet)

nit: please wrap the while body in { } and indent it.

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: patch v6

Comment 19

[Alexandre Poirot [:ochameau]]

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c53ee8e2eb742739b2bbdb73f733e01251157ef6

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

I believe you need sec-approval flag before landing a sec-high.  (Should this really be a sec-high if it requires devtools toolbox open?)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Assuming this affects more than just trunk, this needs sec-approval.

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8922753 [details] [diff] [review]
patch v6

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It involves having devtools opened, so I would not say it is easy.
Here, you have to have the style editor opened.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I'm not sure I get this question. bulls-eye?
The test very specificaly test this particular issue.
But the check-in and comments only say what we are doing and are not really highlighting a particular security issue.

Which older supported branches are affected by this flaw?

mozilla-release branch is affected, so all supported branches are affected.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I haven't checked, but the files I'm modifying aren't actively modified, so it should be easy to backport.

How likely is this patch to cause regressions; how much testing does it need?

It may affect inspector and style editor, so it would be great to have these panel tested. But devtools don't have QA smoketest.

Comment 23

[Daniel Veditz [:dveditz]]

(In reply to Alexandre Poirot [:ochameau] from comment #22)
> mozilla-release branch is affected, so all supported branches are affected.
> 
> If not all supported branches, which bug introduced the flaw?

What about ESR-52 branch? That's really the point of the question since the whole point of ESR is supporting people with security fixes.

Since this requires a victim to open dev tools and click on error links I'm lowering the severity to moderate.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Daniel Veditz [:dveditz] from comment #23)
> (In reply to Alexandre Poirot [:ochameau] from comment #22)
> > mozilla-release branch is affected, so all supported branches are affected.
> > 
> > If not all supported branches, which bug introduced the flaw?
> 
> What about ESR-52 branch? That's really the point of the question since the
> whole point of ESR is supporting people with security fixes.

Service workers are disabled on ESR.  So its not an issue there.

Comment 25

[Jun (Request needinfo for comment, I ignore other updates)]

BTW, as I said in comment 2, opening style editor is the right repro and victim doesn't have to click on error.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8922753 [details] [diff] [review]
patch v6

We've made this a sec-moderate so I'm clearing the sec-approval. Feel free to check it into trunk.

Comment 27

[Phil Ringnalda (:philor)]

Landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9886e3e68e479eeb1507e9e1d6725d0a0bffa2 and backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2e37923eab0096bc9303127659123ae323d5deaf for eslint bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=140566727&repo=mozilla-inbound

Comment 28

[Alexandre Poirot [:ochameau]]

Attachment: patch v6

Comment 29

[Alexandre Poirot [:ochameau]]

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cada4204925d053d42972f810c384fb5f52d8b33

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f10d05c0b3d21ee77242e4396c9d0bb05264347d

Comment 31

[Julien Cristau [:jcristau]]

sec-moderate, I don't feel the need to track this.  Please request beta approval when you believe this has baked enough in nightly.

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing devtools at devtools/client/styleeditor/test/browser_styleeditor_bug_1405342_serviceworker_iframes.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f573a266230f7007fab88d2fcfb4b6588d1b91a7

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f10d05c0b3d21ee77242e4396c9d0bb05264347d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=146471053&repo=mozilla-inbound

17:03:46     INFO -  12960 INFO Entering test bound
17:03:46     INFO -  12961 INFO Adding a new tab with URL: 'https://test1.example.com/browser/devtools/client/styleeditor/test/bug_1405342_serviceworker_iframes.html'
17:03:46     INFO -  12962 INFO URL 'https://test1.example.com/browser/devtools/client/styleeditor/test/bug_1405342_serviceworker_iframes.html' loading complete
17:03:46     INFO -  Buffered messages logged at 17:03:45
17:03:46     INFO -  12963 INFO Console message: [JavaScript Error: "The stylesheet https://test2.example.com/browser/devtools/client/styleeditor/test/sheet.css was not loaded because its MIME type, “text/plain”, is not “text/css”." {file: "https://test2.example.com/browser/devtools/client/styleeditor/test/iframe_with_service_worker.html" line: 0}]
17:03:46     INFO -  12964 INFO Console message: [JavaScript Warning: "XUL box for h3 element contained an inline #text child, forcing all its children to be wrapped in a block." {file: "chrome://devtools/content/styleeditor/styleeditor.xul" line: 0}]
17:03:46     INFO -  Buffered messages logged at 17:03:46
17:03:46     INFO -  12965 INFO TEST-PASS | devtools/client/styleeditor/test/browser_styleeditor_bug_1405342_serviceworker_iframes.js | Got the iframe stylesheet -
17:03:46     INFO -  Buffered messages finished
17:03:46    ERROR -  12966 INFO TEST-UNEXPECTED-FAIL | devtools/client/styleeditor/test/browser_styleeditor_bug_1405342_serviceworker_iframes.js | stylesheet content is the one served by the service worker - Got <html>                    <head><title>404 Not Found</title></head>                    <body>                      <h1>404 Not Found</h1>                      <p>                        <span style='font-family: monospace;'>&#47;&#98;&#114;&#111;&#119;&#115;&#101;&#114;&#47;&#100;&#101;&#118;&#116;&#111;&#111;&#108;&#115;&#47;&#99;&#108;&#105;&#101;&#110;&#116;&#47;&#115;&#116;&#121;&#108;&#101;&#101;&#100;&#105;&#116;&#111;&#114;&#47;&#116;&#101;&#115;&#116;&#47;&#115;&#104;&#101;&#101;&#116;&#46;&#99;&#115;&#115;</span> was not found.                      </p>                    </body>                  </html>, expected * { color: green; }
17:03:46     INFO -  Stack trace:
17:03:46     INFO -  chrome://mochikit/content/browser-test.js:test_is:1269
17:03:46     INFO -  chrome://mochitests/content/browser/devtools/client/styleeditor/test/browser_styleeditor_bug_1405342_serviceworker_iframes.js:null:21

Comment 33

[Alexandre Poirot [:ochameau]]

Attachment: patch v7

Sorry, this bug dropped off my radar.
Looks like DevToolsUtils had an issue with requests coming from SW when running in non-e10s.

Patrick, please have a look at the new change in DevToolsUtils.
Accessing `nsIChannel.contentCharset` throws when accessing it for a request served
by a Service worker. That, only when running on non-e10s.
I didn't saw that first as we run non-e10s tests only on windows debug builds and I'm pushing to try only on linux.

Comment 34

[Alexandre Poirot [:ochameau]]

New try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f57e4bb084ac9855ffae092e87296d93a9bd565

Comment 35

[Alexandre Poirot [:ochameau]]

Attachment: patch v8

Fixed eslint.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/412a612a30d09fdf0d7c04a23de3c0c78337a81b

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/412a612a30d0

Comment 38

[Julien Cristau [:jcristau]]

Is this worth getting into 58?

Comment 39

[Alexandre Poirot [:ochameau]]

Comment on attachment 8935311 [details] [diff] [review]
patch v8

Approval Request Comment
[Feature/Bug causing the regression]:
  Probably has always been around.
[User impact if declined]:
  Leak cross origin information when a Service worker is involved *and* the style editor is opened.
[Is this code covered by automated tests?]:
  Yes, most of the complexity of this patch comes from its test. The fix itself is quite simple.
[Has the fix been verified in Nightly?]:
  Not yet by QA.
[Needs manual test from QE? If yes, steps to reproduce]: 
  Would be great, comment 0 describes a good STR.
[List of other uplifts needed for the feature/fix]:
  None
[Is the change risky?]:
  No
[Why is the change risky/not risky?]:
  Limited to DevTools, this code is only used by style editor and inspector.
  The patch looks quite simple.
[String changes made/needed]:
  None

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

For security issues we normally don't land the tests until after the bug is un-hidden.
But, since they are already on trunk, may as well bring this to beta (should be in beta 11 or 12)

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8935311 [details] [diff] [review]
patch v8

Sec issue, let's uplift to beta.

Comment 42

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/198a20c061dadba7366c0963ec845823c8e9a034

Comment 43

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Managed to reproduce the issue using 57.0 build4 (20171112125346). I can confirm that it is verified fixed on 59.0a1 (2017-12-28) and 58.0b13 build1 (20171226085105), using Windows 10 x64, macOS 10.13.2 and Ubuntu 16.04 x64.