Bug 1319070 (CVE-2017-5386)

Web Extension's data: scheme pages can be controlled by other extensions

VERIFIED FIXED in Firefox -esr45

Status

defect
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: sdna.muneaki.nishimura, Assigned: zombie, NeedInfo)

Tracking

({sec-moderate})

Trunk
mozilla53
Bug Flags:
sec-bounty +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox-esr4551+ verified, firefox50 wontfix, firefox51+ verified, firefox52+ verified, firefox53+ verified)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])

Attachments

(6 attachments, 12 obsolete attachments)

5.65 KB, application/zip
Details
2.88 KB, patch
Details | Diff | Splinter Review
2.49 KB, patch
Details | Diff | Splinter Review
2.01 KB, patch
Details | Diff | Splinter Review
1.19 KB, patch
Details | Diff | Splinter Review
7.10 KB, patch
cbook
: checkin+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
Posted file 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

3 years ago
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?
Group: firefox-core-security → toolkit-core-security
Component: Untriaged → WebExtensions: General
Flags: needinfo?(kmaglione+bmo)
Product: Firefox → Toolkit
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.
Flags: needinfo?(kmaglione+bmo)
zombie, any chance you want to work on this? If not, I'll take care of it.
Flags: needinfo?(tomica)
Reporter

Updated

3 years ago
Summary: Web Extension's data: scheme pages can be controller by other extensions → Web Extension's data: scheme pages can be controlled by other extensions
Assignee

Comment 4

3 years ago
sure, i can try.
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Flags: needinfo?(tomica)
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.
Keywords: sec-high
Flags: sec-bounty?
Assignee

Comment 6

3 years ago
Posted patch bug-1319070-fix.patch (obsolete) — Splinter Review
Attachment #8813787 - Flags: review?(kmaglione+bmo)
Assignee

Comment 7

3 years ago
Posted patch bug-1319070-tests.patch (obsolete) — Splinter Review
Attachment #8813789 - Flags: review?(kmaglione+bmo)
Assignee

Comment 8

3 years ago
(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?
(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 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...
Assignee

Comment 11

3 years ago
> > +      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?  ;)
Assignee

Comment 12

3 years ago
Posted patch bug-1319070-fix-v2.patch (obsolete) — Splinter Review
Typo corrected.
Attachment #8813787 - Attachment is obsolete: true
Attachment #8813787 - Flags: review?(kmaglione+bmo)
Attachment #8813894 - Flags: review?(kmaglione+bmo)
Assignee

Comment 13

3 years ago
(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).
Flags: needinfo?(amckay)

Comment 14

3 years ago
(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.
Flags: needinfo?(amckay)

Comment 15

3 years ago
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).
Assignee

Comment 16

3 years ago
Posted patch bug-1319070-tests-v2.patch (obsolete) — Splinter Review
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).
Attachment #8813789 - Attachment is obsolete: true
Attachment #8813789 - Flags: review?(kmaglione+bmo)
Attachment #8814476 - Flags: review?(kmaglione+bmo)
Assignee

Comment 17

3 years ago
(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 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.
Attachment #8813894 - Flags: review?(kmaglione+bmo)
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 `(`
Attachment #8814476 - Flags: review?(kmaglione+bmo) → review+
Assignee

Comment 20

3 years ago
Posted patch bug-1319070-fix-v3.patch (obsolete) — Splinter Review
> 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.
Attachment #8813894 - Attachment is obsolete: true
Attachment #8814740 - Flags: review?(kmaglione+bmo)
Assignee

Comment 21

3 years ago
Posted patch bug-1319070-tests-v3.patch (obsolete) — Splinter Review
> 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?
Attachment #8814476 - Attachment is obsolete: true
Attachment #8814747 - Flags: review?(kmaglione+bmo)
Attachment #8814740 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8814747 - Flags: review?(kmaglione+bmo) → review+
(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.
Assignee

Comment 24

3 years ago
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.
Attachment #8814740 - Flags: sec-approval?
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.
Attachment #8814740 - Flags: sec-approval? → sec-approval+
Assignee

Comment 26

3 years ago
(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?
Flags: needinfo?(kmaglione+bmo)
Assignee

Comment 27

3 years ago
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?
Flags: needinfo?(amckay)
Assignee

Comment 28

3 years ago
Posted patch bug-1319070-fix-beta.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(kmaglione+bmo)
Attachment #8816734 - Flags: review?(kmaglione+bmo)
Assignee

Comment 29

3 years ago
Posted patch bug-1319070-fix-aurora.patch (obsolete) — Splinter Review
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?
Assignee

Comment 30

3 years ago
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
Attachment #8816737 - Flags: approval-mozilla-aurora?
Assignee

Comment 31

3 years ago
(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.
Flags: needinfo?(amckay)
(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.
Attachment #8816734 - Flags: review?(kmaglione+bmo) → review+
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)
Assignee

Comment 34

3 years ago
Posted patch bug-1319070-fix-v4.patch (obsolete) — Splinter Review
This is the patch that got r+ and sec-approval+ above, with the vaguer commit message.
Attachment #8814740 - Attachment is obsolete: true
Assignee

Comment 35

3 years ago
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.
Attachment #8816737 - Attachment is obsolete: true
Attachment #8816737 - Flags: approval-mozilla-aurora?
Attachment #8820054 - Flags: approval-mozilla-aurora?
Assignee

Comment 36

3 years ago
Posted patch bug-1319070-fix-beta-v2.patch (obsolete) — Splinter Review
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.
Attachment #8816734 - Attachment is obsolete: true
Attachment #8820056 - Flags: approval-mozilla-release?
Attachment #8820056 - Flags: approval-mozilla-beta?
Kris and zombie - Is ESR affected here?
Flags: needinfo?(tomica)
Flags: needinfo?(kmaglione+bmo)
Hi :abillings,
Do we need sec-approval for the patch?
Flags: needinfo?(abillings)
Assignee

Comment 39

3 years ago
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.
Flags: needinfo?(tomica)
(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.
Flags: needinfo?(abillings)
Re-reading comment 39, please fill out the sec-approval? template anyway but I'll just approve it.
Flags: needinfo?(kmaglione+bmo)
Assignee

Comment 42

3 years ago
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.
Attachment #8820053 - Flags: sec-approval?
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.
Attachment #8820053 - Flags: sec-approval? → sec-approval+
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.
Attachment #8820054 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8820056 - Flags: approval-mozilla-release?
Attachment #8820056 - Flags: approval-mozilla-beta?
Attachment #8820056 - Flags: approval-mozilla-beta+
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
Assignee

Comment 47

3 years ago
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).
Attachment #8820053 - Attachment is obsolete: true
Assignee

Comment 49

3 years ago
Same thing as comment #47.
Attachment #8820054 - Attachment is obsolete: true
Assignee

Comment 50

3 years ago
Again, same as comment #47.
Attachment #8820056 - Attachment is obsolete: true
https://hg.mozilla.org/releases/mozilla-beta/rev/0ef9b3667a06

ni? zombie as a reminder for the ESR45 patch.
Flags: needinfo?(tomica)
https://hg.mozilla.org/mozilla-central/rev/6ccd471984af
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee

Comment 54

3 years ago
Flags: needinfo?(tomica)
Assignee

Comment 55

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

Failures seem unrelated to this bug.
Assignee

Comment 56

3 years ago
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
Attachment #8822683 - Flags: approval-mozilla-esr45?
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
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.
Flags: sec-bounty? → sec-bounty+
Keywords: sec-highsec-moderate
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.
Flags: needinfo?(jorge)
Flags: needinfo?(amckay)
Looks okay to me.
Flags: needinfo?(jorge)

Comment 60

3 years ago
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.
Flags: needinfo?(amckay)
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?
Flags: needinfo?(tomica)
Group: toolkit-core-security → core-security-release
Assignee

Comment 62

3 years ago
(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.
Flags: needinfo?(tomica)
(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 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
Attachment #8822683 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Posted patch Match against the principal (obsolete) — Splinter Review
MozReview-Commit-ID: 8FnBZsUyvtg
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
Alias: CVE-2017-5386
Comment on attachment 8827632 [details] [diff] [review]
Match against the principal

This patch upload was an `hg bzexport` misfire. Sorry for the noise.
Attachment #8827632 - Attachment is obsolete: true
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.
Assignee

Comment 69

2 years ago
(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).
Flags: needinfo?(abillings)
Assignee

Comment 70

2 years ago
Hey Will, can you please make sure this compatibility issue is documented?
Flags: needinfo?(wbamberg)
(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.
Flags: needinfo?(abillings)
Assignee

Updated

2 years ago
Attachment #8847547 - Flags: checkin?
Flags: in-testsuite? → in-testsuite+
Group: core-security-release
Assignee

Comment 75

2 years ago
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.
Flags: needinfo?(tomica)
(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.
Assignee

Comment 77

2 years ago
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.
Flags: needinfo?(tomica)

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.