1092388 - nsGlobalWindow::SecurityCheckURL can allow content to load restricted URIs

Status: RESOLVED FIXED

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

Description

[moz_bug_r_a4]

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.

Comment 1

[moz_bug_r_a4]

Attachment: testcase - arbitrary code execution

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.)

Comment 2

[Daniel Veditz [:dveditz]]

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).

Comment 3

[moz_bug_r_a4]

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.

Comment 4

[moz_bug_r_a4]

Attachment: testcase - arbitrary code execution

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.)

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(Assuming bholley is looking at this.)

Comment 8

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=22ce0a2a738f

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove cross-origin document.write test. v1

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.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 2 - Clamp the entry and incumbent global to the current global if their principals don't match. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove now-unnecessary nsGlobalWindow::CallerGlobal. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 4 - Tests. v1

Comment 13

[Bobby Holley (:bholley)]

I think we should fix bug 984467 too while we're already making noise in this area.

Comment 14

[Bobby Holley (:bholley)]

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?

Comment 15

[moz_bug_r_a4]

(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.)

Comment 16

[Bobby Holley (:bholley)]

(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!

Comment 17

[Boris Zbarsky [: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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Glad to see this thing die.  r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8519176 [details] [diff] [review]
Part 4 - Tests. v1

r=me

Comment 21

[Bobby Holley (:bholley)]

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.

Comment 22

[Bobby Holley (:bholley)]

(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.

Comment 23

[Bobby Holley (:bholley)]

Attachment: Part 4 - Tests. v2 r=bz

Comment 24

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

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.

Comment 25

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7de86d3b2a08
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/366564330ff3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/56185630acb5

Comment 26

[Bobby Holley (:bholley)]

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

Comment 27

[Lawrence Mandel [:lmandel] (use needinfo)]

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-

Comment 28

[Bobby Holley (:bholley)]

(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 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7de86d3b2a08
https://hg.mozilla.org/mozilla-central/rev/366564330ff3
https://hg.mozilla.org/mozilla-central/rev/56185630acb5

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f0e0ea2e0734
https://hg.mozilla.org/releases/mozilla-aurora/rev/0671ae35933f
https://hg.mozilla.org/releases/mozilla-aurora/rev/5531b615a485

Comment 32

[Ryan VanderMeulen [:RyanVM]]

This looks like an easy backport to b2g34. b2g32/esr31 are going to need a bit of attention, though.

Comment 33

[Bobby Holley (:bholley)]

Attachment: Check the entry window before using it on ESR31. v1

GetEntryGlobal didn't exist on esr31, so the best we can do there is to
spot-fix.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8524031 [details] [diff] [review]
Check the entry window before using it on ESR31. v1

r=me

Comment 35

[Bobby Holley (:bholley)]

I only tested this on esr31, but it should also apply on b2g32. Please let me know if it doesn't.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c63666bdbe08
https://hg.mozilla.org/releases/mozilla-esr31/rev/3362a6dccaa2

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/a4901c6f40f4
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/6c7b84807f8f
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/64a9229bc479

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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

Comment 39

[Bobby Holley (:bholley)]

(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.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Thanks.

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2d0860bd0225
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/2d0860bd0225

Comment 41

[Bobby Holley (:bholley)]

Al, per bug 1110614 I think this should be embargoed somehow. Unfortunately I don't have time to look at it at the moment.

Comment 42

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

Ah, ok. Thanks for letting me know. I'll remove the CVE.

Comment 43

[Daniel Veditz [:dveditz]]

(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?

Comment 44

[Daniel Veditz [:dveditz]]

... until bug 1110614 fixed presumably.