1124898 - (CVE-2015-0802) Privileged Window.webidl stuff is exposed based on the docshell type, not the principal of the actual page

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

From bug 1120261.

>         setTimeout(function(){
>           x.messageManager.loadFrameScript('data:,throw Components.utils',
> false);

This is the most egregious bit, IMO. it looks like Window.webidl puts all of the privileged stuff behind Func="nsGlobalWindow::IsChromeWindow", rather than making it ChromeOnly. This means that, if content can ever get a reference to a chrome window (which it shouldn't, but can happen when paired with another exploit as in this bug), it can navigate that window to its own page, and go wild on chrome-privileged stuff.

Boris, is there any reason for the above? Seems like we should almost certainly fix it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The initial deal with nsGlobalWindow::IsChromeWindow was to flag precisely the set of things that came from nsIDOMChromeWindow with it, to preserve the old behavior.

There is no reason past that.

I think "messageManager" should in fact also have a [ChromeOnly] flag, as should getGroupMessageManager and beginWindowMove.  The constants I wouldn't necessarily worry about, or probably windowState.  Not sure about browserDOMWindow or the various getAttention/setCursor/maximize/minimize/restore bits, or notifyDefaultButtonLoaded.

It might be safe to just check for chrome caller in IsChromeWindow if we're sure that there are no cases of someone loading untrusted stuff in type="chrome" docshells and expecting it to use any of these bits.  Which seems like an OK assumption.

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Andrew McCreight [:mccr8]]

Bobby, do you think this rises to the level of a sec-high or sec-critical that we should backport everywhere, or is this just more of a nice to have defense-in-depth thing?

Assigning to Bobby because he has a patch for this issue. Adjust as needed.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Bobby, do you think this rises to the level of a sec-high or sec-critical
> that we should backport everywhere, or is this just more of a nice to have
> defense-in-depth thing?

The message manager thing is worrisome, though it's still not an exploit all on its own (since you still need a way to get your hands on a chrome window).

sec-moderate, though I might still argue for at least backporting to aurora.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Stop exposing ex-nsIDOMChromeWindow things for random unprivileged things loaded in chrome docshells. v1

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8553993 [details] [diff] [review]
Stop exposing ex-nsIDOMChromeWindow things for random unprivileged things loaded in chrome docshells. v1

r=me

Have I mentioned recently how much more pleasant this sort of thing is than working with xpconnect bindings?  ;)

Comment 7

[Bobby Holley (:bholley)]

Attachment: Tests. v1

I realized this probably wants tests.

Comment 8

[Bobby Holley (:bholley)]

Also, boris - do you think that landing this as sec-moderate on nightly and aurora (with tests) is safe enough?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8554200 [details] [diff] [review]
Tests. v1

Might be better to setTimeout for 10ms or something instead of executeSoon while busy-waiting, so you don't starve the event loop too much.

r=me if you add a comment to file_empty about other tests depending on the page title.

And yes, landing on nightly+aurora seems ok.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Might be better to setTimeout for 10ms or something instead of executeSoon
> while busy-waiting, so you don't starve the event loop too much.

Yes, but setTimeouts are kinda banned these days.

> r=me if you add a comment to file_empty about other tests depending on the
> page title.

Ok.
 
> And yes, landing on nightly+aurora seems ok.

Ok great - thanks.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Yes, but setTimeouts are kinda banned these days.

Sure, but we still have a way to do them, if there's good reason...  and in this case I think there is.  I don't feel super-strongly about it, though.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> > Yes, but setTimeouts are kinda banned these days.
> 
> Sure, but we still have a way to do them, if there's good reason...  and in
> this case I think there is.  I don't feel super-strongly about it, though.

Yeah I could go either way I guess - I was also concerned about requestFlakeyTimeout not being available on older branches that we may decide to backport this to. Anyway, I already pushed by the time your comment came in.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/adba768607dc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bab6997a3bf8

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8553993 [details] [diff] [review]
Stop exposing ex-nsIDOMChromeWindow things for random unprivileged things loaded in chrome docshells. v1

Approval Request Comment
[Feature/regressing bug #]: longstanding
[User impact if declined]: potential vector for privilege escalation
[Describe test coverage new/current, TreeHerder]: Checked in with tests
[Risks and why]: Could theoretically break addons. I don't think we gain much testing there on Nightly/Aurora, which is why I'm proposing to accelerate this fix 6 weeks. Low potential for regressions unrelated to the intended behavior change.
[String/UUID change made/needed]: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/adba768607dc
https://hg.mozilla.org/mozilla-central/rev/bab6997a3bf8

Comment 15

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

Comment on attachment 8553993 [details] [diff] [review]
Stop exposing ex-nsIDOMChromeWindow things for random unprivileged things loaded in chrome docshells. v1

Agreed. Let's get this into 37 now and deal with any fallout early. Aurora+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/669f865e1e77
https://hg.mozilla.org/releases/mozilla-aurora/rev/2884eaa7f1e5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/669f865e1e77
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2884eaa7f1e5

Comment 18

[Daniel Veditz [:dveditz]]

(In reply to Bobby Holley (:bholley) from comment #4)
> The message manager thing is worrisome, though it's still not an exploit all
> on its own (since you still need a way to get your hands on a chrome window).
> 
> sec-moderate, though I might still argue for at least backporting to aurora.

That seems optimistic -- lots of moz_bug_r_a4 exploits (and more recently Mariusz) are all about finding a same-origin violation plus some way of getting a handle to a chrome window. So far they haven't run out of the latter. Since the resulting combination is sec-critical I'd argue that this is a particular dangerous rock hanging over our heads. Sec-high might better represent our level of worry.


[Tracking Requested - why for this release]:
I'm concerned sec-moderate undersells the risk. If there's any sec-high same-origin violations left on ESR-31 then this bug turns them into sec-critical remote code execution.

Comment 19

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

Bobby, do you think we're going to get an ESR31 patch this cycle?

Comment 20

[Bobby Holley (:bholley)]

This won't be easy to fix without WebIDL window, which landed in 32 (bug 789261). I think we should embargo.