nsGlobalWindow::SecurityCheckURL can allow content to load restricted URIs

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: moz_bug_r_a4, Assigned: bholley)

Tracking

({csectype-priv-escalation, regression, sec-high})

unspecified
mozilla36
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox34 wontfix, firefox35+ fixed, firefox36+ fixed, firefox-esr3135+ fixed, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main35-][adv-esr31.4-][embargo until bug 1110614 fixed])

Attachments

(5 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
This seems to be a regression from bug 997067.

If a chrome code that is running on a JSContext associated with a chrome window calls into a content function and that function calls window.open(), the chrome window is used as sourceWindow (this behavior seems weird but this is not a regression).  Before bug 997067, it only affects the base URI, but now it also affects the subject principal that nsScriptSecurityManager::CheckLoadURIFromScript uses.

As far as I know, AutoPager and Firebug are exploitable.
Reporter

Comment 1

5 years ago
Posted file testcase - arbitrary code execution (obsolete) —
This works on fx >= 31 with AutoPager 0.8.0.10 or Firebug 2.0.4 or Error Console.

(This does not work with Firebug on Nightly. On Nightly, Firebug's code seems to run on a JSContext associated with a Sandbox.)
When I try this using the Error Console version on a release build (33, which should be in the affected range) I get an error that top.opener is null. Am I using the wrong Error Console? I've tried in the devtools "Console", the "Browser Console", and the deprecated (requires setting a pref) "Error Console" with the same results.

I _can_ reproduce using AutoPager 0.8.0.10, it would just be easier for QA later on if they didn't have to install an add-on to verify. This still reproduces on Nightly using AutoPager so I guess that's the best way to test this for now.

Repro note: AutoPager doesn't work with e10s so make sure that's disabled. If AutoPager is working you'll get an AutoPager settings dialog following the repro steps whether or not the bug is present (the bug is demonstrated by the JS stack alert).
Blocks: 997067
Keywords: regression
Reporter

Comment 3

5 years ago
It seems that I cannot get bugmails (the last bugmail I got is the one sent on 2014-10-13). Who should I ask for help?

(In reply to Daniel Veditz [:dveditz] from comment #2)
> When I try this using the Error Console version on a release build (33,
> which should be in the affected range) I get an error that top.opener is
> null. Am I using the wrong Error Console? I've tried in the devtools
> "Console", the "Browser Console", and the deprecated (requires setting a
> pref) "Error Console" with the same results.

I'm not sure why you cannot reproduce using the deprecated Error Console. I've been using the deprecated Error Console but even so, I should have used the Browser Console to write STR. I'll attach a testcase that contains new STR using the Browser Console.
Reporter

Comment 4

5 years ago
This works on fx >= 31 with AutoPager 0.8.0.10 or Firebug 2.0.4 or Browser Console.

(This does not work with Firebug on Nightly. On Nightly, Firebug's code seems to run on a JSContext associated with a Sandbox.)
(In reply to moz_bug_r_a4 from comment #3)
> It seems that I cannot get bugmails (the last bugmail I got is the one sent
> on 2014-10-13). Who should I ask for help?

This has now been fixed (which you probably noticed from receiving a bug of bugmail all at once...). It was bug 1092949.
(Assuming bholley is looking at this.)
Assignee: nobody → bobbyholley
Component: Security → DOM
Assignee

Comment 9

5 years ago
This test is pretty bogus. It uses enablePrivilege and then tries to test
various document.write behaviors across origins.

This test in general is pretty terrible. I spent a while trying to improve it,
got frustrated, was tempted to remove the whole thing, and then finally settled
on this as the path of least resistance.
Attachment #8519173 - Flags: review?(bzbarsky)
Assignee

Comment 12

5 years ago
Posted patch Part 4 - Tests. v1 (obsolete) — Splinter Review
Attachment #8519176 - Flags: review?(bzbarsky)
Assignee

Comment 13

5 years ago
I think we should fix bug 984467 too while we're already making noise in this area.
Flags: needinfo?(bobbyholley)
Assignee

Comment 14

5 years ago
So anyway. The basic problem here is that window.open does GetEntryGlobal() followed by JSAutoCompartment, effectively stealing the privileges of the entry global, which may not be something we trust. I think the safest thing to do here is to clamp this at the source.

Relatedly, it seems totally insane to me that the mere fact of opening a window with browser.xul lets unprivileged content subsequently navigate to arbitrary data:URIs and let them run with System Principal. Boris, is there something we can do on this front?
Flags: needinfo?(bzbarsky)
Reporter

Comment 15

5 years ago
(In reply to Bobby Holley (:bholley) from comment #14)
> Relatedly, it seems totally insane to me that the mere fact of opening a
> window with browser.xul lets unprivileged content subsequently navigate to
> arbitrary data:URIs and let them run with System Principal.

That is not what the testcase does. The testcase takes advantage of browser.js code that opens the same sidebar that opener window has. This trick was used in the testcases in bug 982906 and bug 982909. (Actually, my testcase need not have used a subframe and a data: URI.)
Assignee

Comment 16

5 years ago
(In reply to moz_bug_r_a4 from comment #15)
> That is not what the testcase does. The testcase takes advantage of
> browser.js code that opens the same sidebar that opener window has. This
> trick was used in the testcases in bug 982906 and bug 982909. (Actually, my
> testcase need not have used a subframe and a data: URI.)

Oh hm, that sounds like something we should fix then. I've filed bug 1096319. Thanks for pointing that out!
Flags: needinfo?(bzbarsky)
Comment on attachment 8519173 [details] [diff] [review]
Part 1 - Remove cross-origin document.write test. v1

Do we have an existing test that makes sure that chrome doing document.open on a non-chrome document throws?

r=me if so or if one is added.
Attachment #8519173 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519174 [details] [diff] [review]
Part 2 - Clamp the entry and incumbent global to the current global if their principals don't match. v1

Ah, here is the test I was asking about.

Can we add a test here that after the successful open() call the nodePrincipal of the document is the same as it was before, and in particular, isn't our document's nodePrincipal (which is presumably the system principal; that's worth explicitly checking here too).

r=me, though at some point we may want to think a bit about the performance of the script settings stuff.
Attachment #8519174 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519175 [details] [diff] [review]
Part 3 - Remove now-unnecessary nsGlobalWindow::CallerGlobal. v1

Glad to see this thing die.  r=me
Attachment #8519175 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519176 [details] [diff] [review]
Part 4 - Tests. v1

r=me
Attachment #8519176 - Flags: review?(bzbarsky) → review+
Assignee

Comment 21

5 years ago
Comment on attachment 8519175 [details] [diff] [review]
Part 3 - Remove now-unnecessary nsGlobalWindow::CallerGlobal. v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

So-so. It's clear from the patch that we're returning an incumbent or entry global that isn't subsumed by the code that's using it, but they'd have to go hunting to find the problem case.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

See above.

Which older supported branches are affected by this flaw?

31 and up.

If not all supported branches, which bug introduced the flaw?

Bug 997067.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I don't have backports. They should be straightforward.

How likely is this patch to cause regressions; how much testing does it need?

Testing would be good.
Attachment #8519175 - Flags: sec-approval?
Assignee

Comment 22

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #18)
> Can we add a test here that after the successful open() call the
> nodePrincipal of the document is the same as it was before, and in
> particular, isn't our document's nodePrincipal (which is presumably the
> system principal; that's worth explicitly checking here too).

I'm going to add it in the testcase that'll get checked in later on.
Flags: in-testsuite?
Assignee

Comment 23

5 years ago
Attachment #8519176 - Attachment is obsolete: true
Attachment #8522405 - Flags: review+
Comment on attachment 8519175 [details] [diff] [review]
Part 3 - Remove now-unnecessary nsGlobalWindow::CallerGlobal. v1

sec-approval+.

If we are going to backport this, especially to ESR31, we should get patches made and nominated ASAP.
Attachment #8519175 - Flags: sec-approval? → sec-approval+
Assignee

Comment 26

5 years ago
Comment on attachment 8519175 [details] [diff] [review]
Part 3 - Remove now-unnecessary nsGlobalWindow::CallerGlobal. v1

Approval Request Comment
[Feature/regressing bug #]: bug 997067
[User impact if declined]: sec
[Describe test coverage new/current, TBPL]: just pushed to inbound. I have a local mochitest that I'm not checking in.
[Risks and why]: medium-low. This touches code that's pretty central to the platform, but the changes it makes should be unobservable to web-content.
[String/UUID change made/needed]: none
Attachment #8519175 - Flags: approval-mozilla-esr31?
Attachment #8519175 - Flags: approval-mozilla-beta?
Attachment #8519175 - Flags: approval-mozilla-aurora?
Comment on attachment 8519175 [details] [diff] [review]
Part 3 - Remove now-unnecessary nsGlobalWindow::CallerGlobal. v1

I don't this we should take this in 34. We have already shipped this in 3 releases, the patch hasn't yet landed on m-c and bholley thinks it has moderate risk, and we're shipping the final beta today. Unless there is a really strong case for taking this bug in 34, I suggest that we take it in 35 and ESR 31.4.0. Beta-
Attachment #8519175 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Assignee

Comment 28

5 years ago
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #27)
> I don't this we should take this in 34. We have already shipped this in 3
> releases, the patch hasn't yet landed on m-c and bholley thinks it has
> moderate risk, and we're shipping the final beta today. Unless there is a
> really strong case for taking this bug in 34, I suggest that we take it in
> 35 and ESR 31.4.0. Beta-

SGTM.
Comment on attachment 8519175 [details] [diff] [review]
Part 3 - Remove now-unnecessary nsGlobalWindow::CallerGlobal. v1

Approving for Aurora, setting the tracking on ESR31  -- this can't be approved for that landing until we've shipped 31.3
Attachment #8519175 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This looks like an easy backport to b2g34. b2g32/esr31 are going to need a bit of attention, though.
Assignee

Comment 33

5 years ago
GetEntryGlobal didn't exist on esr31, so the best we can do there is to
spot-fix.
Attachment #8524031 - Flags: review?(bzbarsky)
Comment on attachment 8524031 [details] [diff] [review]
Check the entry window before using it on ESR31. v1

r=me
Attachment #8524031 - Flags: review?(bzbarsky) → review+
Assignee

Comment 35

5 years ago
I only tested this on esr31, but it should also apply on b2g32. Please let me know if it doesn't.
Flags: needinfo?(ryanvm)
Attachment #8519175 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
b2g32 has bug 997987 on it, so the esr31 patch doesn't work. Backed out.

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/169cbacd13c1
Flags: needinfo?(bobbyholley)
Assignee

Comment 39

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #38)
> b2g32 has bug 997987 on it, so the esr31 patch doesn't work. Backed out.
> 
> https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/169cbacd13c1

You mean bug 998002, right? If that's all it is, you just need to do is rename nsContentUtils::GetSubjectPrincipal to nsContentUtils::SubjectPrincipal.
Flags: needinfo?(bobbyholley) → needinfo?(ryanvm)
Whiteboard: [adv-main35+][adv-esr31.4+]
Alias: CVE-2014-8636
Assignee

Comment 41

4 years ago
Al, per bug 1110614 I think this should be embargoed somehow. Unfortunately I don't have time to look at it at the moment.
Flags: needinfo?(abillings)
Ah, ok. Thanks for letting me know. I'll remove the CVE.
Alias: CVE-2014-8636
Flags: needinfo?(abillings)
Whiteboard: [adv-main35+][adv-esr31.4+] → [adv-main35-][adv-esr31.4-][embargo]
Assignee

Updated

4 years ago
Blocks: 1120261

Updated

4 years ago
Group: core-security → core-security-release
(In reply to Bobby Holley (PTO through June 13) from comment #41)
> Al, per bug 1110614 I think this should be embargoed somehow. Unfortunately
> I don't have time to look at it at the moment.

Embargoed until when?
Whiteboard: [adv-main35-][adv-esr31.4-][embargo] → [adv-main35-][adv-esr31.4-][embargo (until when?)]
... until bug 1110614 fixed presumably.
Group: core-security-release
Whiteboard: [adv-main35-][adv-esr31.4-][embargo (until when?)] → [adv-main35-][adv-esr31.4-][embargo until bug 1110614 fixed]
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.