1110614 - Fix for ESR31 in bug 1092388 can be circumvented

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[moz_bug_r_a4]

The principal check for sourceWindow was added to nsGlobalWindow::SecurityCheckURL, but nsWindowWatcher::URIfromURL does not have the same check for baseWindow. If content calls window.open with a relative URI, SecurityCheckURL gets a base URI from a content window, and thus CheckLoadURIFromScript passes, but URIfromURL gets a base URI from a chrome window, and thus windowwatcher opens a chrome URI.

Comment 1

[moz_bug_r_a4]

Attachment: testcase - arbitrary code execution

This works on firefox-31.3.0esr-2014-12-11-00-05-06 with AutoPager 0.8.0.10 or Firebug 2.0.7 or Browser Console.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Bobby, are you going to have time to look into this?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Bobby, are you going to have time to look into this?

At some point yes, but I don't think it's as high priority as the media stuff I'm working on (only affects ESR people with certain addons, not super discoverable). So it might be a few more weeks. If you have a free moment to look at it that would be great.

Comment 4

[Andrew McCreight [:mccr8]]

It is too late for the next release, I think, so this wouldn't really be able to land for 2 or 3 weeks anyways.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So I tried looking at this in a current esr31 debug build, but having trouble reproducing it so far... e.g. using Firebug I get some popup windows, but nothing involving Components.stack.

Comment 6

[moz_bug_r_a4]

(In reply to Boris Zbarsky [:bz] from comment #5)
> So I tried looking at this in a current esr31 debug build, but having
> trouble reproducing it so far... e.g. using Firebug I get some popup
> windows, but nothing involving Components.stack.

The testcase consists of two parts, and the second part was fixed in bug 1096319, so the testcase can open browser.xul but cannot run arbitrary code. So far, I cannot think of a new way to run arbitrary code.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm. I was getting blank-looking windows, not something that looked like browser.xul.  Is that about what you expect me to see if I'm seeing this bug?

Comment 8

[moz_bug_r_a4]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Hmm. I was getting blank-looking windows, not something that looked like
> browser.xul.  Is that about what you expect me to see if I'm seeing this bug?

Yes, I'm getting blank-looking browser.xul windows. When I filed this bug, I did not try to make the testcase explicitly show that it opened browser.xul, since the testcase was able to indicate the bug by showing Components.stack alert.

I'll attach a testcase that shows that content can open chrome URLs.

Comment 9

[moz_bug_r_a4]

Attachment: testcase 2 - open chrome URLs

This works on 31.4.0esr-candidates-build1 with AutoPager 0.8.0.10 or Firebug 2.0.7 or Browser Console.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Align the base URI window determinations in nsWindowWatcher::URIfromURL, nsGlobalWindow::FireAbuseEvents, and nsGlobalWindow::SecurityCheckURL

Comment 11

[Boris Zbarsky [:bzbarsky]]

Thank you for the updated testcase!  That was very very helpful.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8552612 [details] [diff] [review]
Align the base URI window determinations in nsWindowWatcher::URIfromURL, nsGlobalWindow::FireAbuseEvents, and nsGlobalWindow::SecurityCheckURL

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

Thanks for taking care of this Boris.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8552612 [details] [diff] [review]
Align the base URI window determinations in nsWindowWatcher::URIfromURL, nsGlobalWindow::FireAbuseEvents, and nsGlobalWindow::SecurityCheckURL

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-high security bug.
Fix Landed on Version: Firefox 35
Risk to taking this patch (and alternatives if risky): I _think_ this is fairly
   low-risk.  The main risk would be in terms of extension-compat, if extensions
   are doing something weird...  I'm not sure there's a less risky alternative.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 15

[Andrew McCreight [:mccr8]]

Boris, this needs sec-approval.  Also, hopefully Al can mark the approval for esr31 at the same time as he gives sec-approval.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8552612 [details] [diff] [review]
Align the base URI window determinations in nsWindowWatcher::URIfromURL, nsGlobalWindow::FireAbuseEvents, and nsGlobalWindow::SecurityCheckURL

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not very easily, I suspect, but not with that much difficulty either.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  I don't think so.

Which older supported branches are affected by this flaw?  ESR 31.

If not all supported branches, which bug introduced the flaw?  It's been around for a long time; it's just fixed elsewhere.

Do you have backports for the affected branches?  This is the backport.

How likely is this patch to cause regressions; how much testing does it need?  Risk is, imo, medium. Testing with extensions would be nice if possible.

Comment 17

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

Comment on attachment 8552612 [details] [diff] [review]
Align the base URI window determinations in nsWindowWatcher::URIfromURL, nsGlobalWindow::FireAbuseEvents, and nsGlobalWindow::SecurityCheckURL

Party on, Wayne. Approvals given.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/fbe02a90af22

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/ef542c0d310

Comment 20

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue several times using the POC/instructions from comment # 9 with the following build:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/31.4.0esr/

Went through verification using the following build:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/31.5.0esr-candidates/build2/

When running through the POC with fx 31.4.0, every time the new windows opened either via the "Test" button, Firebug or AutoPager, you could see the top level URLs:

[0] chrome://browser/content/browser.xul
[1] chrome://browser/content/devtools/webconsole.xul
[2] chrome://browser/content/browser.xul?x
[3] chrome://browser/content/preferences/preferences.xul?x
[4] chrome://global/content/aboutAbout.xhtml?x"

When running through the POC with fx 31.5.0, you would receive the following:

[0] chrome://browser/content/browser.xul
[1] chrome://browser/content/devtools/webconsole.xul
[2] chrome://browser/content/browser.xul
[3] chrome://browser/content/browser.xul
[4] chrome://browser/content/browser.xul

If I miss understood the issue or incorrectly tested this, please let me know! Marking as verified.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/ef542c0d310