1319070 - (CVE-2017-5386) Web Extension's data: scheme pages can be controlled by other extensions

Status: VERIFIED FIXED

(WebExtensions :: General, defect)

Description

[Muneaki Nishimura]

Attachment: poc.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/602.1.50 (KHTML, like Gecko) Version/10.0 Safari/602.1.50

Steps to reproduce:

1) Install both "attacker" and "victim" extensions in the attachment to Firefox
2. Open the browser_action of "victim" and click a link "Click to Reproduce" in the page



Actual results:

You can see a message "victim is pwned by attacker" in the page and an alert is shown with the content of victim extension's localStorage, i.e., SECRET

This means that the the attacker's extensions can control other extension's pages with data: scheme.

The cause of this issue is below.
https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/MatchPattern.jsm#21
Here the data: scheme is whitelisted but data: documents in Firefox inherits opener's context.


Expected results:

The data: scheme pages in another extension should be controlled by other extensions.

Comment 1

[:Gijs (he/him)]

Kris, is this as simple as removing "data" from the whitelist, or is this more complicated? Maybe if add-ons like using data: for their initial URIs or something, we just need to make sure that any followup navigations include DISABLE_INHERIT_PRINCIPAL or whatever the flag is?

Comment 2

[Kris Maglione [:kmag]]

We should probably do something similar to what we're now doing for about:blank, and checking the URL of the principal rather than of the page.

Comment 3

[Kris Maglione [:kmag]]

zombie, any chance you want to work on this? If not, I'll take care of it.

Comment 4

[Tomislav Jovanovic :zombie]

sure, i can try.

Comment 5

[Kris Maglione [:kmag]]

Potential for privilege escalation. Mitigated by the fact that extension permissions aren't currently exposed to users, and legacy extensions aren't sandboxed at all. Can't be used to gain chrome privileges since content scripts are never allowed to inherit the system principal.

Comment 6

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix.patch

Comment 7

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-tests.patch

Comment 8

[Tomislav Jovanovic :zombie]

(In reply to Kris Maglione [:kmag] from comment #2)
> We should probably do something similar to what we're now doing for
> about:blank, and checking the URL of the principal rather than of the page.

That made sense to me, so I did that, but I'm not the happiest with the outcome.

This does in fact fix the security issue at hand, but the bigger dev story about specifying a "data:" scheme in the match pattern makes little sense after this fix.

We could either deprecate the use case, or at least document the issue (later, obviously).  Not sure if there would be any actual impact on compatibility.  Maybe Andy could check what percentage of Chrome extensions actually tries to match against data: URIs (explicitly), for what purpose, and if this would break any?

Comment 9

[Kris Maglione [:kmag]]

(In reply to Tomislav Jovanovic :zombie from comment #8)
> (In reply to Kris Maglione [:kmag] from comment #2)
> > We should probably do something similar to what we're now doing for
> > about:blank, and checking the URL of the principal rather than of the page.
> 
> That made sense to me, so I did that, but I'm not the happiest with the
> outcome.
> 
> This does in fact fix the security issue at hand, but the bigger dev story
> about specifying a "data:" scheme in the match pattern makes little sense
> after this fix.
> 
> We could either deprecate the use case, or at least document the issue
> (later, obviously).  Not sure if there would be any actual impact on
> compatibility.  Maybe Andy could check what percentage of Chrome extensions
> actually tries to match against data: URIs (explicitly), for what purpose,
> and if this would break any?

Yeah. So, I can see two... three alternatives:

1) Remove support for data: URL match patterns, and just match them based on the principal URL.
2) Keep supporting match patterns, but check that the principal URL either matches another match pattern or matches a host permission.
3) Treat data: URLs the same way we treat about:blank and about:srcdoc, and either add matchDataURLs or reuse matchAboutBlank to match them.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8813787 [details] [diff] [review]
bug-1319070-fix.patch

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

This looks pretty good to me, but I'm leaving it r? pending a decision on how we should handle the match patterns.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +155,3 @@
>        // When matching top-level about:blank documents,
>        // allow loading into any with a NullPrincipal.
> +      if (uri.spec === "about.blank" && window === window.top && principal.isNullPrincipal) {

"about:blank"

Which means I guess we're missing a test for this?

@@ +165,5 @@
> +      }
> +    }
> +
> +    // Documents with data: URIs also inherit the principal.
> +    if (uri.scheme === "data") {

I wonder if we should just check `Services.netUtils.URIChainHasFlags(uri, URI_INHERITS_SECURITY_CONTEXT)`? I guess it shouldn't make a difference, given that we already restrict match patterns, though...

Comment 11

[Tomislav Jovanovic :zombie]

> > +      if (uri.spec === "about.blank" && window === window.top && principal.isNullPrincipal) {
> 
> "about:blank"
> 
> Which means I guess we're missing a test for this?

No, this only means I didn't run those tests after the last "just some code cleanup", I'm sorry! :(

browser_ext_tabs_executeScript.js#205 fails with that.


> I wonder if we should just check `Services.netUtils.URIChainHasFlags(uri,
> URI_INHERITS_SECURITY_CONTEXT)`? I guess it shouldn't make a difference,
> given that we already restrict match patterns, though...

I was considering that, but no other scheme that we allow inherits.  Using a check against "data" probably results in more explicit, readable code.  Although, this being a security fix, maybe that's not desirable?  ;)

Comment 12

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix-v2.patch

Typo corrected.

Comment 13

[Tomislav Jovanovic :zombie]

(In reply to Kris Maglione [:kmag] from comment #9)
> 2) Keep supporting match patterns, but check that the principal URL either
> matches another match pattern or matches a host permission.

This is basically my current fix (host permissions are already used for tabs.executeScript).

Options 1) and 3) need to be public by nature, and can be done as a follow-up after this lands to fix the immediate threat.  So my current thinking is to go with this, and decide on further actions outside the time pressure of this bug.


Andy, what do you think about the big picture?  Can your script/stats help us estimate how many Chrome extensions explicitly match against data: URIs?  (not <all_urls>, as those will continue to work).

Comment 14

[Andy McKay]

(In reply to Tomislav Jovanovic :zombie from comment #13)
> Andy, what do you think about the big picture?  Can your script/stats help
> us estimate how many Chrome extensions explicitly match against data: URIs? 
> (not <all_urls>, as those will continue to work).

I found only 1 mention of data: in the manifests of 100k extensions. 

  	"web_accessible_resources": [
  		"css/images/*.png", 
  		"css/images/*.gif",
  		"data:image/*"
  	],

https://chrome.google.com/webstore/detail/rummage/pahcjlpnciigaejakjnndfmmnppihhmg

Has one user. Assuming I'm searching for the right thing.

Comment 15

[:Gijs (he/him)]

In case it's relevant: note that data: is treated differently by chrome than by us - it can inherit a principal in Firefox, and doesn't in Chrome, see https://groups.google.com/d/msg/mozilla.dev.platform/w0IgmymXDds/4-EvoXPvAAAJ (which is about us aligning with Chrome, which is... difficult).

Comment 16

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-tests-v2.patch

Updated test to avoid checking tabId from webNavigation (not available on android),
and to use the same mechanism in the other test (works better in non-e10s mode).

Comment 17

[Tomislav Jovanovic :zombie]

(In reply to Andy McKay [:andym] from comment #14)
> I found only 1 mention of data: in the manifests of 100k extensions. 

No other mentions in content_scripts declarations?  That's almost unbelievable.


(In reply to :Gijs Kruitbosch from comment #15)
> In case it's relevant: note that data: is treated differently by chrome than
> by us - it can inherit a principal in Firefox, and doesn't in Chrome, see
> https://groups.google.com/d/msg/mozilla.dev.platform/w0IgmymXDds/4-EvoXPvAAAJ
> (which is about us aligning with Chrome, which is... difficult).

From smaug's last email in that thread, it looks like this is actually happening.

Once that is done, alternatives 1) and 3) become unnecessary, and we might want to revert to matching against the data: URI (and not principal) for compat reasons.

Which brings the question whether we should do anything here, apart from fixing the (temporary) security issue until data: URIs get the unique opaque origins?

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8813894 [details] [diff] [review]
bug-1319070-fix-v2.patch

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

Sorry to be such a pain about this. Given the discussion so far, I'd be happy with just the two changes below. They should also allow the code to correctly match non-inherited data: URI origins, when they're implemented, without any changes.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +165,5 @@
> +      }
> +    }
> +
> +    // Documents with data: URIs also inherit the principal.
> +    if (uri.scheme === "data") {

Let's go with the URI_INHERITS_SECURITY_CONTEXT flag check for now, just for the sake of future proofing, and add a comment that it currently applies only to data: URIs. I agree that this version is more readable, but it seems like this behavior is going to change, and we might wind up overlooking other similar cases in the future.

@@ +170,1 @@
>        uri = principal.URI;

Given the way we're currently handling this, and how similar it is to the use case and handling of about:srcdoc, I think maybe we should only do this if match_about_blank is set, and reject the match otherwise.

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8814476 [details] [diff] [review]
bug-1319070-tests-v2.patch

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

Looks good aside from a few nits. Thanks!

::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript_bad.js
@@ +147,5 @@
> +    permissions: ["<all_urls>", "webNavigation"],
> +  };
> +
> +  const files = {
> +    "page.html": `

Please always include a `<!DOCTYPE html>` declaration so we don't wind up in quirks mode.

@@ +166,5 @@
> +    let tab;
> +
> +    browser.webNavigation.onCompleted.addListener(({url, frameId}) => {
> +      browser.test.log(`Document loading complete: ${url}`);
> +      if (!frameId) {

Nit: `if (frameId === 0)` so it's clearer we're looking for the top-level frame rather than an omitted frame ID.

@@ +179,5 @@
> +          allFrames: true,
> +        }).then(([result]) => {
> +          browser.test.fail(`Should not have matched ${result}`);
> +          browser.test.sendMessage("done");
> +        }).catch(({message}) => {

Nit: Please use a second argument to the `.then()` clause rather than a `.catch()` clause here, since we're not trying to catch errors from the success handler.

Or, even better, please just use `browser.test.assertRejects`, and make this an async function, so we wind up with something like:

    if (cmd === "execute") {
      await browser.test.assertRejects(
        browser.tabs.executeScript(tab.id, {
          code: "location.href;",
          allFrames: true,
        }),
        /No matching window/,
        "Should not execute in `data:` frame");

      browser.test.sendMessage("done");
    }

@@ +195,5 @@
> +  yield extension.startup();
> +
> +  // Test extension page with a data: iframe.
> +  const page = yield extension.awaitMessage("tab-ready");
> +  ok(page.includes("page.html"), "Extension page loaded into a tab");

Nit: s/includes/endsWith/

@@ +198,5 @@
> +  const page = yield extension.awaitMessage("tab-ready");
> +  ok(page.includes("page.html"), "Extension page loaded into a tab");
> +
> +  extension.sendMessage("execute");
> +  yield extension.awaitMessage("done");

mixedpuppy is currently working on allowing extensions to match their own moz-extension: URLs. I'm not actually sure what the expected behavior will be for executeScript with actual extension pages at that point... so this might break. But we'll deal with that when we come to it.

The fix would probably involve just loading two extensions rather than one.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_data_uri.html
@@ +33,5 @@
> +      body { background: yellow; }
> +    `,
> +    "all_urls.js": function() {
> +      document.body.style.color = "red";
> +      browser.test.assertFalse(window.location.href.startsWith("data:"),

Nit: `assertTrue(location.protocol != "data:", ...`

@@ +34,5 @@
> +    `,
> +    "all_urls.js": function() {
> +      document.body.style.color = "red";
> +      browser.test.assertFalse(window.location.href.startsWith("data:"),
> +        `Matched document not a data URI: ${window.location.href}`);

Nit: Please align with opening `(`

Comment 20

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix-v3.patch

> Sorry to be such a pain about this.

Based on what I know about security bugs, it wasn't really that painful.  ;)

Anyway, your arguments make sense, even if I don't fully agree with the match_about_blank part.

Comment 21

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-tests-v3.patch

> Or, even better, please just use `browser.test.assertRejects`, and make this
This loses the ability to fail with the matched document's URI in the log (which was useful to me during debugging), so I kept the other approach.


> mixedpuppy is currently working on allowing extensions to match their own
> moz-extension: URLs.   [...]   so this might break. 
Since Shane's changes are likely to land before these tests, I went ahead and updated them, so r? again.


> Nit: Please align with opening `(`
I hate this rule.  It feels ugly and foreign to JS, coming from C++ land.
Why does WE code style even differ from the general Mozilla style guide?

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8814747 [details] [diff] [review]
bug-1319070-tests-v3.patch

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

::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript_bad.js
@@ +145,5 @@
> +add_task(function* testMatchDataURI() {
> +  const target = ExtensionTestUtils.loadExtension({
> +    files: {
> +      "page.html": `
> +        <!doctype html>

Nit: DOCTYPE should be capitalized, and this should start on the previous line.

@@ +174,5 @@
> +        }
> +      });
> +
> +      browser.test.onMessage.addListener(msg => {
> +        browser.tabs.executeScript({

Nit: Please use browser.test.assertRejects and an async function. If you really want the document URL for debugging, you can add it to the assertion message, or change assertRejects to report the resolution value.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_data_uri.html
@@ +15,5 @@
> +add_task(function* test_contentscript_data_uri() {
> +  const target = ExtensionTestUtils.loadExtension({
> +    files: {
> +      "page.html": `
> +        <!doctype html>

As above, DOCTYPE, and on the line above.

Comment 23

[Kris Maglione [:kmag]]

(In reply to Tomislav Jovanovic :zombie from comment #21)
> > Nit: Please align with opening `(`
> I hate this rule.  It feels ugly and foreign to JS, coming from C++ land.
>
> Why does WE code style even differ from the general Mozilla style guide?

It's a common rule in C and C++, too, and also in Toolkit code. All arguments
are aligned with the first argument, and the first argument either goes
immediately after the opening paren, or on its own line at the next indent
level.

Comment 24

[Tomislav Jovanovic :zombie]

Comment on attachment 8814740 [details] [diff] [review]
bug-1319070-fix-v3.patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not too easily.  Understanding the issue in this bug could be considered easy (for a security researcher).  Realizing that this could also apply to another extension's page is a first non-obvious leap IMO.  Finally, constructing an exploit extension that navigates a "victim" extension's page to a data: URI is probably fairly hard (I'm not aware of a way to do that).


> Do comments in the patch, the check-in comment, or tests included
> in the patch paint a bulls-eye on the security problem?
The tests are in a separate patch.  The use of URI_INHERITS_SECURITY_CONTEXT constant, and the comment above make it somewhat obvious this a security-related patch, especially if it lands linked to a protected bug.  But nothing in the patch points to this applying for extension pages, the first necessary leap explained above.


> Which older supported branches are affected by this flaw?
The original POC works on aurora.  It doesn't work on beta and release, but that is related to the use of popup.  Opening victim extension's page in a tab still allows the attacker extension to attach the content script after navigating to a data:URI.  The `localStorage.get()` method call throws "Component is not available" for some reason, but that is only one venue of attack, there are likely others available to attacker that can attach a content script.


> If not all supported branches, which bug introduced the flaw?
No bug.  This is an architectural mismatch between Chrome's extension API and our inheriting principals for data: URIs.


> Do you have backports for the affected branches?
No.


> If not, how different, hard to create, and risky will they be?
Slightly different because the code is related to `match_about_blank` which was implemented in the last cycle (current aurora).  But not risky as the actual fix is fairly simple and easy to understand/test. 


> How likely is this patch to cause regressions; how much testing does it need?
It is unlikely this will cause a lot of regressions.  Per Andy's comment above, explicitly matching against data: URIs is rare, and using <all_urls> will continue to work if extensions have the right to match the owner document.

Not a single existing web extensions test failed as a result of this fix.

Comment 25

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

Comment on attachment 8814740 [details] [diff] [review]
bug-1319070-fix-v3.patch

sec-approval+ for trunk for the patch (but please don't check in tests yet). Please nominate this for affected branches as well so we can patch there.

Comment 26

[Tomislav Jovanovic :zombie]

(In reply to Tomislav Jovanovic :zombie from comment #24)
> > How likely is this patch to cause regressions; how much testing does it need?
> It is unlikely this will cause a lot of regressions.  Per Andy's comment
> above, explicitly matching against data: URIs is rare, and using <all_urls>
> will continue to work if extensions have the right to match the owner
> document.

Looking again into this, I disagree with myself here.  I was thinking of the impact of my original proposed patch.  With the final patch, which refuses the match without match_about_blank, this is much more likely to break existing extensions.

And this is especially true for backports to earlier branches where match_about_blank was not implemented, and extensions couldn't even include it because schema validation would throw.


Kris, in light of this, do you agree with backports ignoring match_about_blank, and always matching against the principal (basically my original patch)?  I know it's not your preferred fix, but it would only be "temporary", and would give us time to document the new behavior in Nightly and let developers "fix" extensions while this rides the trains?

Comment 27

[Tomislav Jovanovic :zombie]

Andy, can you please check again how many extensions is this likely to affect?

The number of Chrome extensions that specify <all_urls> in `content_scripts`, and how many of those also specify match_about_blank?

Comment 28

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix-beta.patch

This is the patch I propose for beta and earlier branches.


Built and passing all tests on beta, but:
 - Couldn't build on ESR45 for some reason,
 - Successfully built on the release branch, but our mochitests don't run for for another obscure reason.  Manually confirmed the security fix.


Including match_about_blank in those versions would require changes in quite a few more places: tabs.executeScript, schema...  To minimize risk from those, I suggest we go with this.

Comment 29

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix-aurora.patch

I'm not sure about the protocol for security bugs.  Do I need to ask for review/sec-approval for each of these, or just approval-branch?

Comment 30

[Tomislav Jovanovic :zombie]

Comment on attachment 8816737 [details] [diff] [review]
bug-1319070-fix-aurora.patch

This is a trivial port of the original patch that got r+ and sec-approval+ above.


Approval Request Comment
> [Feature/Bug causing the regression]:
No bug.  Web extensions content scripts have a security issue.

> [User impact if declined]:
Potential privilege escalation between extensions.

> [Is this code covered by automated tests?]:
Existing functionality around this code has good test coverage.  I wrote new tests for this issue, though they are not landing yet because security bug.

> [Has the fix been verified in Nightly?]:
Yes, I built and manually verified on both nightly and aurora.

> [Needs manual test from QE? If yes, steps to reproduce]: 
Unsure what is the protocol for security bugs.  I'm guessing no.
If I'm wrong, STR are in comment #0.

> [List of other uplifts needed for the feature/fix]:
None

> [Is the change risky?]:
Somewhat

> [Why is the change risky/not risky?]:
Overall, not too risky as impact is limited to Web extensions.  But the change might break some existing extensions.  This is unavoidable, and we have steps for developers to mitigate (not yet public due to security bug).

> [String changes made/needed]:
None

Comment 31

[Tomislav Jovanovic :zombie]

(In reply to Tomislav Jovanovic :zombie from comment #27)
> The number of Chrome extensions that specify <all_urls> in
> `content_scripts`, and how many of those also specify match_about_blank?

From Andy:

    198 match_about_blank
   4339 all_urls
     68 both

out of ~50k Chrome extensions.

Comment 32

[Kris Maglione [:kmag]]

(In reply to Tomislav Jovanovic :zombie from comment #26)
> (In reply to Tomislav Jovanovic :zombie from comment #24)
> > > How likely is this patch to cause regressions; how much testing does it need?
> > It is unlikely this will cause a lot of regressions.  Per Andy's comment
> > above, explicitly matching against data: URIs is rare, and using <all_urls>
> > will continue to work if extensions have the right to match the owner
> > document.
> 
> Looking again into this, I disagree with myself here.  I was thinking of the
> impact of my original proposed patch.  With the final patch, which refuses
> the match without match_about_blank, this is much more likely to break
> existing extensions.

I don't this is very likely to cause serious issues in practice.

> Kris, in light of this, do you agree with backports ignoring
> match_about_blank, and always matching against the principal (basically my
> original patch)?  I know it's not your preferred fix, but it would only be
> "temporary", and would give us time to document the new behavior in Nightly
> and let developers "fix" extensions while this rides the trains?

I don't really have a strong opinion either way.

Comment 33

[Kris Maglione [:kmag]]

Comment on attachment 8816734 [details] [diff] [review]
bug-1319070-fix-beta.patch

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

(Please remove the reference to data: URIs from the commit message before landing)

Comment 34

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix-v4.patch

This is the patch that got r+ and sec-approval+ above, with the vaguer commit message.

Comment 35

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix-aurora-v2.patch

This is a trivial port (with vaguer commit message) of the original patch that got r+ and sec-approval+ above.

> Approval Request Comment

See comment #30 above.

Comment 36

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix-beta-v2.patch

This is the patch for beta and release that got r+ above, with a vaguer commit message.

> Approval Request Comment
Same as aurora approval request comment #30 above.

Comment 37

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

Kris and zombie - Is ESR affected here?

Comment 38

[Gerry Chang [:gchang]]

Hi :abillings,
Do we need sec-approval for the patch?

Comment 39

[Tomislav Jovanovic :zombie]

From my understanding of the ESR_Landing_Process wiki, we're supposed to wait for things to land on beta before asking for ESR approval.  Is that right for security bugs too?


(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #37)
> Kris and zombie - Is ESR affected here?
From the most conservative point of view, yes, this code is present in esr45.

Beyond that, many other major parts of Web Extensions were not present in 45, so the likelihood of actual impact is IMO much lower.  Unfortunately, I can't verify this, as I'm unable to build from the esr45 branch on Windows (`mach build` claims I need to run `mercurial-setup`, which throws with an unhelpful error).


(In reply to Gerry Chang [:gchang] from comment #38)
> Hi :abillings,
> Do we need sec-approval for the patch?
Not trying to speak for :abillings, just to give details:
 - the original patch got sec-approval+, the latest version changed just the commit message,
 - the patch for aurora is a trivial back-port,
 - the patch for beta/release is modified because the flag used in the original patch was not present in those versions of the code.  IMO, this change does not modify the security implications of the patch.

Comment 40

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

(In reply to Gerry Chang [:gchang] from comment #38)
> Hi :abillings,
> Do we need sec-approval for the patch?

Yes, it is a sec-high that affects more than one branch.

Comment 41

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

Re-reading comment 39, please fill out the sec-approval? template anyway but I'll just approve it.

Comment 42

[Tomislav Jovanovic :zombie]

Comment on attachment 8820053 [details] [diff] [review]
bug-1319070-fix-v4.patch

[Security approval request comment]
Mostly the same as comment #24, as the only thing changed is a commit message.


> How easily could an exploit be constructed based on the patch?
Not too easily.  Understanding the issue in this bug could be considered easy (for a security researcher).  Realizing that this could also apply to another extension's page is a first non-obvious leap IMO.  Finally, constructing an exploit extension that navigates a "victim" extension's page to a data: URI is probably fairly hard (I'm not aware of a way to do that).


> Do comments in the patch, the check-in comment, or tests included
> in the patch paint a bulls-eye on the security problem?
The tests are in a separate patch.  The use of URI_INHERITS_SECURITY_CONTEXT constant, and the comment above make it somewhat obvious this a security-related patch, especially if it lands linked to a protected bug.  But nothing in the patch points to this applying for extension pages, the first necessary leap explained above.


> Which older supported branches are affected by this flaw?
The original POC works on aurora.  It doesn't work on beta and release, but that is related to the use of popup.  Opening victim extension's page in a tab still allows the attacker extension to attach the content script after navigating to a data:URI.  The `localStorage.get()` method call throws "Component is not available" for some reason, but that is only one venue of attack, there are likely others available to attacker that can attach a content script.

ESR 45 is potentially also affected, see comment #39 for details.


> If not all supported branches, which bug introduced the flaw?
No bug.  This is an architectural mismatch between Chrome's extension API and our inheriting principals for data: URIs.


> Do you have backports for the affected branches?
Yes, attached above, except for ESR 45, which I'm unable to build from source (will try via a push to try _after_ this lands to all other branches).


> If not, how different, hard to create, and risky will they be?
It was slightly different and not hard or risky.


> How likely is this patch to cause regressions; how much testing does it need?
It is unlikely this will cause a lot of regressions.  All existing web extensions tests pass with this patch applied.

Comment 43

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

Comment on attachment 8820053 [details] [diff] [review]
bug-1319070-fix-v4.patch

sec-approval+ for trunk landing. Please nominate for branch, aurora, and beta. If you need altered patches for branches, please attach those and nominate them. 

Do *not* land any tests. We won't land tests for this issue until after we publicly ship a fix so we don't 0day ourselves with our own tests.

Comment 44

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

Comment on attachment 8820054 [details] [diff] [review]
bug-1319070-fix-aurora-v2.patch

Giving aurora and beta approval. Release approval would only be given if we were chemspilling for it or it was going in another point release so I'm clearing that.

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f4f0b816009

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Iris backed this out from Aurora for Jetpack failures. Not sure why this got uplifted to Aurora prior to landing on trunk, though, TBH.
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf6b75460a2544ee06f6331cb6ed0a0da32cddf1

Comment 47

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix-v5.patch

Since this already got landed once, in order to speed things up, and per discussion with Ryan, this is the updated patch containing a one-line test-only fix, with r=me on the Jetpack part (where I am a peer).

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ccd471984af

Comment 49

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix-aurora-v3.patch

Same thing as comment #47.

Comment 50

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix-beta-v3.patch

Again, same as comment #47.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/31d044fc3878

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ef9b3667a06

ni? zombie as a reminder for the ESR45 patch.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ccd471984af

Comment 54

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-fix-esr45-v2.patch

Comment 55

[Tomislav Jovanovic :zombie]

Try push for the esr45 patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c78f31b7e0c4

Failures seem unrelated to this bug.

Comment 56

[Tomislav Jovanovic :zombie]

Comment on attachment 8822683 [details] [diff] [review]
bug-1319070-fix-esr45-v2.patch

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is a sec:high bug.

> User impact if declined: 
Potential privilege escalation across extensions.

> Fix Landed on Version:
Just landed on trunk which is currently at version 53.

> Risk to taking this patch (and alternatives if risky): 
Mild risk or breaking some existing Web Extensions.

> String or UUID changes made by this patch: 
N/A

Comment 57

[Daniel Veditz [:dveditz]]

Although this does violate the isolation WebExtensions are trying to create and is therefore important, in the bigger picture this is not a sec-high. If you install a malicious extension you're going to have a bad time and users don't know which add-ons are web extensions and which aren't.

Comment 58

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Andy, Jorge, I am planning to take uplift in ESR45.7 (go-live late Jan). In comment 56, there is a mention of milk risk to some webext that might notice breakage. I just wanted to make you aware and please let me know if you have any concerns.

Comment 59

[Jorge Villalobos [:jorgev] (he/him)]

Looks okay to me.

Comment 60

[Andy McKay]

Given the amount of WebExtensions in 45 (not many) and the amount that affected by this (even less), I'm happy with this change. Thanks Tomica in your persistence with this bug.

Comment 61

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 53.0a1 (2016-11-21) under Windows 10 64-bit (the following window was promped: http://screencast.com/t/mJVw2gPsRF).

Manually tested on Firefox 53.0a1 (2017-01-10), Firefox 52.0a2 (2017-01-10), Firefox 51.0b13 (20170109165508) using Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.12.1 and I noticed the following: 

  - “Null has been stolen by attacked” pop-up is prompted in the First Run Page after installing the attacker webextension: http://screencast.com/t/r1bNdoiYvz5 
  - received the “victim” value after clicking on “Click to Reproduce” link: http://screencast.com/t/ZUd58xLUja 
  - there is no “SECRET has been stolen by attacker” pop-up displayed.


Could you please confirm that this is the expected behavior?

Comment 62

[Tomislav Jovanovic :zombie]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #61)
> Could you please confirm that this is the expected behavior?

Yes, this is correct.

Comment 63

[Vasilica Tamas, QA [:vtamas]]

(In reply to Tomislav Jovanovic :zombie from comment #62)
> (In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #61)
> > Could you please confirm that this is the expected behavior?
> 
> Yes, this is correct.

Thanks for your reply!
Based on Comment 61 and Comment 62 I am marking this bug as Verified.

Comment 64

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8822683 [details] [diff] [review]
bug-1319070-fix-esr45-v2.patch

We could take this one in ESR45, fix is verified, DVeditz agrees. Let's uplift and include in ESR45.7

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/8c171b75a0c6524b117c602740adf1260ccf889e

Comment 66

[Jason Orendorff [:jorendorff]]

Attachment: Match against the principal

MozReview-Commit-ID: 8FnBZsUyvtg

Comment 67

[Jason Orendorff [:jorendorff]]

Comment on attachment 8827632 [details] [diff] [review]
Match against the principal

This patch upload was an `hg bzexport` misfire. Sorry for the noise.

Comment 68

[Vasilica Tamas, QA [:vtamas]]

Confirm that this issue is also fixed on Firefox 45.7.0 esr (20170118123525) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.12.1. Received the same results as those mentioned in Comment 61.

Comment 69

[Tomislav Jovanovic :zombie]

(In reply to Al Billings [:abillings] from comment #43)
> Do *not* land any tests. We won't land tests for this issue until after we
> publicly ship a fix so we don't 0day ourselves with our own tests.

Hi Al, is it time now to land the tests for this?  It has been over two months since this landed on m-c (two release cycles).

Comment 70

[Tomislav Jovanovic :zombie]

Hey Will, can you please make sure this compatibility issue is documented?

Comment 71

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

(In reply to Tomislav Jovanovic :zombie from comment #69)
 
> Hi Al, is it time now to land the tests for this?  It has been over two
> months since this landed on m-c (two release cycles).

Yes! You could probably have landed it two or more weeks after 51 shipped with the fix but we're good now.

Comment 72

[Tomislav Jovanovic :zombie]

Attachment: bug-1319070-tests-v4.patch

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

Comment 73

[Carsten Book [:Tomcat]]

Comment on attachment 8847547 [details] [diff] [review]
bug-1319070-tests-v4.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c704b823d38f479ad228c585439ae41100be166

Comment 74

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c704b823d38

Comment 75

[Tomislav Jovanovic :zombie]

There were recent changes to how Firefox treats data: URIs, specifically about inheriting principals.

Ni? myself to investigate if this is still a compat issue or not.

Comment 76

[Kris Maglione [:kmag]]

(In reply to Tomislav Jovanovic :zombie from comment #75)
> There were recent changes to how Firefox treats data: URIs, specifically
> about inheriting principals.

Principal inheritance for data: URIs is actually pretty weird at this point. They do inherit principals in most cases, but they're special cased in document loads:

https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/docshell/base/nsDocShell.cpp#11267-11270

SVG images with data: URLs, though, still wind up creating documents with inherited principals.

Comment 77

[Tomislav Jovanovic :zombie]

Also, recent blog post relevant to this
https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-data-urls-firefox-58/

So, the original POC in this bug wouldn't even work currently, since top-level navigation to data: URIs is blocked (which is probably worth documenting somewhere, separately from this issue).

But this remains a compat issue for data: iframes inside web pages, so it should still be documented.