Closed Bug 1408708 (CVE-2018-5106) Opened 7 years ago Closed 6 years ago

Developer tool's traffic routes through service worker

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox-esr52 disabled)

VERIFIED FIXED
Firefox 59
Tracking Status
firefox-esr52 --- disabled

People

(Reporter: s.h.h.n.j.k, Assigned: ochameau)

Details

(Keywords: csectype-sop, sec-moderate, Whiteboard: [adv-main58+])

Attachments

(1 file, 8 obsolete files)

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.
Oops, video file was too large. Uploaded unlisted video below.

https://youtu.be/yU4SybqSBsI
Okay, this issue is caused by Style Editor in Developer tool. You only need to open Style Editor to trigger the bug.
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.
Group: firefox-core-security → core-security
Component: Untriaged → DOM: Service Workers
Product: Firefox → Core
Group: core-security → dom-core-security
Version: 1.0 Branch → unspecified
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?
Group: dom-core-security → firefox-core-security
Component: DOM: Service Workers → Developer Tools
Flags: needinfo?(jlaster)
Product: Core → Firefox
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.
Flags: sec-bounty?
Adding some more devtools folks.  I'm not sure who the right person to ask about this is.
I think Alex Poirot would have some context here
Flags: needinfo?(jlaster) → needinfo?(poirot.alex)
Attached patch patch v1 (obsolete) — Splinter Review
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...
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?
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #8920515 - Flags: review?(pbrosset)
Attachment #8920262 - Attachment is obsolete: true
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?
Attachment #8920515 - Flags: review?(pbrosset) → review+
(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;
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #8920522 - Flags: review+
Attachment #8920515 - Attachment is obsolete: true
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?
Attached patch patch v4 (obsolete) — Splinter Review
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)
Attachment #8920522 - Attachment is obsolete: true
Attached patch patch v5 (obsolete) — Splinter Review
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.
Attachment #8922000 - Flags: review?(pbrosset)
Attachment #8921958 - Attachment is obsolete: true
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.
Attachment #8922000 - Flags: review?(pbrosset) → review+
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #8922753 - Flags: review+
Attachment #8922000 - Attachment is obsolete: true
Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c53ee8e2eb742739b2bbdb73f733e01251157ef6
Assignee: nobody → poirot.alex
Flags: needinfo?(poirot.alex)
Keywords: checkin-needed
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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?)
Assuming this affects more than just trunk, this needs sec-approval.
Keywords: checkin-needed
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.
Attachment #8922753 - Flags: sec-approval?
(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.
Flags: needinfo?(poirot.alex)
Keywords: sec-highsec-moderate
(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.
Flags: needinfo?(poirot.alex)
BTW, as I said in comment 2, opening style editor is the right repro and victim doesn't have to click on error.
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.
Attachment #8922753 - Flags: sec-approval?
Flags: needinfo?(poirot.alex)
Flags: sec-bounty? → sec-bounty+
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #8930116 - Flags: review+
Attachment #8922753 - Attachment is obsolete: true
sec-moderate, I don't feel the need to track this.  Please request beta approval when you believe this has baked enough in nightly.
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
Flags: needinfo?(poirot.alex)
Attached patch patch v7 (obsolete) — Splinter Review
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.
Attachment #8935031 - Flags: review?(pbrosset)
Attachment #8930116 - Attachment is obsolete: true
Attachment #8935031 - Flags: review?(pbrosset) → review+
Attached patch patch v8Splinter Review
Fixed eslint.
Attachment #8935311 - Flags: review+
Attachment #8935031 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/412a612a30d0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Is this worth getting into 58?
Flags: needinfo?(poirot.alex)
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
Flags: needinfo?(poirot.alex)
Attachment #8935311 - Flags: approval-mozilla-beta?
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 on attachment 8935311 [details] [diff] [review]
patch v8

Sec issue, let's uplift to beta.
Attachment #8935311 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: firefox-core-security → core-security-release
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [adv-main58+]
Alias: CVE-2018-5106
Product: Firefox → DevTools
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.