Fix for ESR31 in bug 1092388 can be circumvented

VERIFIED FIXED in Firefox -esr31, Firefox OS v2.0

Status

()

VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: moz_bug_r_a4, Assigned: bzbarsky)

Tracking

({sec-high})

31 Branch
mozilla31
x86
Windows 7
sec-high
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox35 unaffected, firefox36 unaffected, firefox37 unaffected, firefox38 unaffected, firefox-esr3136+ verified, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master unaffected)

Details

(Whiteboard: [adv-esr31.5-])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 8535402 [details]
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.
Keywords: sec-high
status-firefox-esr31: --- → affected
tracking-firefox-esr31: --- → ?
Component: Security → DOM
Bobby, are you going to have time to look into this?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
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.
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-firefox35: --- → unaffected
status-firefox36: --- → unaffected
status-firefox37: --- → unaffected
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.
(Reporter)

Comment 6

4 years ago
(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.
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?
(Reporter)

Comment 8

4 years ago
(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.
(Reporter)

Comment 9

4 years ago
Created attachment 8547504 [details]
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.
Created attachment 8552612 [details] [diff] [review]
Align the base URI window determinations in nsWindowWatcher::URIfromURL, nsGlobalWindow::FireAbuseEvents, and nsGlobalWindow::SecurityCheckURL
Attachment #8552612 - Flags: review?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Thank you for the updated testcase!  That was very very helpful.
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.
Attachment #8552612 - Flags: review?(bobbyholley) → review+
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.
Attachment #8552612 - Flags: approval-mozilla-esr31?
Boris, this needs sec-approval.  Also, hopefully Al can mark the approval for esr31 at the same time as he gives sec-approval.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Attachment #8552612 - Flags: sec-approval?
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.
Attachment #8552612 - Flags: sec-approval?
Attachment #8552612 - Flags: sec-approval+
Attachment #8552612 - Flags: approval-mozilla-esr31?
Attachment #8552612 - Flags: approval-mozilla-esr31+
https://hg.mozilla.org/releases/mozilla-esr31/rev/fbe02a90af22
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.1S: --- → unaffected
status-b2g-master: --- → unaffected
status-firefox38: --- → unaffected
status-firefox-esr31: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
tracking-firefox-esr31: ? → 36+
Whiteboard: [adv-esg31.5-]
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.
Status: RESOLVED → VERIFIED
status-firefox-esr31: fixed → verified
Whiteboard: [adv-esg31.5-] → [adv-esr31.5-]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.