All users were logged out of Bugzilla on October 13th, 2018
Bug 1124898 (CVE-2015-0802)

Privileged Window.webidl stuff is exposed based on the docshell type, not the principal of the actual page

RESOLVED FIXED in Firefox 37, Firefox OS v2.2

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({sec-high, sec-moderate})

unspecified
mozilla38
x86
Mac OS X
sec-high, sec-moderate
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main37+] Embargo until fixed on ESR31?)

Attachments

(2 attachments)

(Assignee)

Description

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

Updated

4 years ago
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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.
Assignee: nobody → bobbyholley
(Assignee)

Comment 4

4 years ago
(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.
Keywords: sec-moderate
status-firefox35: --- → wontfix
status-firefox36: --- → wontfix
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox-esr31: --- → wontfix
(Assignee)

Comment 5

4 years ago
Created attachment 8553993 [details] [diff] [review]
Stop exposing ex-nsIDOMChromeWindow things for random unprivileged things loaded in chrome docshells. v1
Attachment #8553993 - Flags: review?(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?  ;)
Attachment #8553993 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

4 years ago
Created attachment 8554200 [details] [diff] [review]
Tests. v1

I realized this probably wants tests.
Attachment #8554200 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

4 years ago
Also, boris - do you think that landing this as sec-moderate on nightly and aurora (with tests) is safe enough?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Attachment #8554200 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

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

Comment 12

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

Comment 13

4 years ago
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
Attachment #8553993 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/adba768607dc
https://hg.mozilla.org/mozilla-central/rev/bab6997a3bf8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v1.4: --- → wontfix
status-b2g-v2.0: --- → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1: --- → wontfix
status-b2g-v2.1S: --- → wontfix
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
status-firefox38: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1126023
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+
Attachment #8553993 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [adv-main37+]
Alias: CVE-2015-0802
(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.
status-firefox-esr31: wontfix → affected
tracking-firefox-esr31: --- → ?
Keywords: sec-high
Whiteboard: [adv-main37+] → [adv-main37+] Embargo until fixed on ESR31?
tracking-firefox-esr31: ? → 38+
Bobby, do you think we're going to get an ESR31 patch this cycle?
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 20

4 years ago
This won't be easy to fix without WebIDL window, which landed in 32 (bug 789261). I think we should embargo.
status-firefox-esr31: affected → wontfix
Flags: needinfo?(bobbyholley)

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.