775868 - (CVE-2012-3986) several nsDOMWindowUtils methods available to untrusted code

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

Attachment: Beginning of a fix.

This surfaced when investigating bug 701299, and potential causes for it. Turns out several methods in nsDOMWindowUtils are not protected by security checks, so any webpage can call them. The unprotected methods are at least, but maybe not limited to:

GetImageAnimationMode
SetImageAnimationMode
Redraw
ElementFromPoint
GetIsMozAfterPaintPending
ClearMozAfterPaintEvents
GetScrollXY
GetIMEIsOpen
GetIMEStatus
GetFocusedInputType
FindElementWithViewId
EnterModalState
LeaveModalState
EnterModalStateWithWindow
LeaveModalStateWithWindow
IsInModalState
GetOuterWindowID
GetCurrentInnerWindowID
GetLayerManagerType
StartFrameTimeRecording
StopFrameTimeRecording
RenderDocument
GetCursorType
GetDisplayDPI
WrapDOMFile
CheckAndClearPaintedState
IsIncrementalGCEnabled
StartPCCountProfiling
StopPCCountProfiling
PurgePCCounts
GetPCCountScriptCount
GetPCCountScriptSummary
GetPCCountScriptContents
GetPaintingSuppressed

Some of those (though not many) are not callable from script (i.e. they take non-scriptable arguments, like JSContext* or what not), so they may be ok, but the majority are not ok.

This is trivially fixable by adding IsUniversalXPConnect() checks in all those methods, but that breaks tests, so we need someone to spend some time fixing up the broken tests. I'll add a patch that pretty blindly just adds the checks, but it needs more work, and I've utterly failed to find the time to fix this up, so someone else needs to take this over... Andrew, maybe you can take a stab at driving this in?

Guessing that this is sg:critical, mostly because of the shear number of unknowns here.

Comment 1

[Bobby Holley (:bholley)]

Can we instead just make the getInterface for nsIDOMWindowUtils fail for unprivileged callers? I thought we already did that, but apparently we don't:

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#8409

Anyway, that seems like a much better approach than whack-a-mole IMO.

Comment 2

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

There was some reason to not do that...but I don't recall what.
Anyhow I'd prefer that approach too.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #2)
> There was some reason to not do that...but I don't recall what.
> Anyhow I'd prefer that approach too.

If this reason came from the good old days, then it was probably to support something we're not willing to support anymore. In particular, we're moving towards making Components available only to chrome and XBL, so nsIDOMWindowUtils will cease to work anyway for content (since they won't be able to QI to nsIInterfaceRequestor).

Comment 4

[Johnny Stenback (:jst)]

I'm all for that too if we can pull it off.

If not, we should write a test that explicitly calls every method on a window utils object and make sure it throws a security exception. We'd have to be a bit inventive in trying different arguments etc, but I think we could make that work (as in, if someone adds something that doesn't work with whatever the test does the test will fail etc).

Comment 5

[Johnny Stenback (:jst)]

Or actually, if we can pull off making getInterface fail, we could also remove the DOM_OBJECT bit from nsDOMWindowUtils' classinfo, and no methods on the object would be callable by non-chrome code, even if a window utils object leaked out to content code somehow.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #5)
> Or actually, if we can pull off making getInterface fail, we could also
> remove the DOM_OBJECT bit from nsDOMWindowUtils' classinfo, and no methods
> on the object would be callable by non-chrome code, even if a window utils
> object leaked out to content code somehow.

Why, exactly?

Comment 7

[Johnny Stenback (:jst)]

The DOM_OBJECT bit in nsIClassInfo is what tells caps whether to use a same origin policy or a system only policy for security checks, if that's not set, we shouldn't even be able to create a wrapper for the object, and calling methods should fail too, that is assuming we still go though XPConnect for this object.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Also note that if we can get away with that we could also move all the scriptable stuff to a webidl interface here, and mark them all as [ChromeOnly].  That's a bit more work, though.

Comment 9

[Andrew McCreight [:mccr8]]

If there are no objections, I'll look into the braindead approach for backporting and whatnot, and we can file a new bug for being less wack-a-mole.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> If there are no objections, I'll look into the braindead approach for
> backporting and whatnot, and we can file a new bug for being less
> wack-a-mole.

Er, has anyone tried the simple approach of just doing a security check in nsGlobalWindow::GetInterface? Does it break stuff?

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Bobby Holley (:bholley) from comment #10)
> Er, has anyone tried the simple approach of just doing a security check in
> nsGlobalWindow::GetInterface? Does it break stuff?

Sure, I can try that first.

Comment 12

[Andrew McCreight [:mccr8]]

Here's an attempt a bholley's suggestion from comment 1:
https://tbpl.mozilla.org/?tree=Try&rev=e76955c9f2b2

Comment 13

[Andrew McCreight [:mccr8]]

I locally confirmed that test_bug61098.html fails with a NS_NOINTERFACE error, so that's something. ;)

Comment 14

[Andrew McCreight [:mccr8]]

While looking at failures, I came across bug 435325, which despite its low number was only fixed last year.  This bug added a method nsDOMWindowUtils::GoOnline(), and it relies on it being available from untrusted content: "This is only allowed from about:neterror, which is unprivileged, so it can't access the io-service itself."

This also breaks content/browser/toolkit/components/places/tests/browser/browser_bug680727.js which attempts to call about:neterror.

Maybe we can move GoOnline to some other web-exposed place...

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: intercept QI, WIP

I think this should pass all tests except the two mentioned in my last comment. I need to go back and look at if these tests I have changed are doing something they might expect to work from unprivileged content.

Comment 16

[Andrew McCreight [:mccr8]]

- The various identity tests use .outerWindowID
- dom/tests/mochitest/bugs/test_bug61098.html uses enterModalStateWithWindow and leaveModalStateWithWindow to simulate some kind of prompt service thing
- layout/forms/test/test_bug348236.html and toolkit/content/tests/widgets/popup_shared.js enable UniversalXPConnect privileges before they does anything with their handle on the window utils, so this shouldn't affect when they can do.
- toolkit/components/prompts/test/test_bug625187.html uses dispatchDOMEventViaPresShell

I also had to change test_bug397571.html because that checks whether a few things fail with and without privilege, and how exactly that works is different now.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: intercept QI, WIP

I fixed up the one test to actually check that the functions work when there is privilege.

Comment 18

[Bobby Holley (:bholley)]

FWIW, I think you can also just do SpecialPowers.DOMWindowUtils.

Comment 19

[Andrew McCreight [:mccr8]]

Ah, cool, maybe I will just do that.

For posterity, there's a previous round of "unprivileged content can use windows utils" wack-a-mole in bug 397571, from 2007.

Comment 20

[Alex Keybl [:akeybl]]

What versions of Firefox are affected here? I'm getting 16/17 but would like to know if 15 (or farther back) is similarly affected.

Comment 21

[Andrew McCreight [:mccr8]]

(In reply to Alex Keybl [:akeybl] from comment #20)
> What versions of Firefox are affected here? I'm getting 16/17 but would like
> to know if 15 (or farther back) is similarly affected.

Some of these functions are super old, going back to 2009 at least (like GetScrollXY).

Comment 22

[Alex Keybl [:akeybl]]

(In reply to Andrew McCreight [:mccr8] from comment #21)
> (In reply to Alex Keybl [:akeybl] from comment #20)
> > What versions of Firefox are affected here? I'm getting 16/17 but would like
> > to know if 15 (or farther back) is similarly affected.
> 
> Some of these functions are super old, going back to 2009 at least (like
> GetScrollXY).

Thanks Andrew. We'll track for FF15 - since this is internally reported though, if a low risk fix isn't found early in the cycle, we may not uplift that far.

Comment 23

[Justin Dolske [:Dolske]]

(In reply to Andrew McCreight [:mccr8] from comment #14)
> While looking at failures, I came across bug 435325, which despite its low
> number was only fixed last year.  This bug added a method
> nsDOMWindowUtils::GoOnline(), and it relies on it being available from
> untrusted content: "This is only allowed from about:neterror, which is
> unprivileged, so it can't access the io-service itself."

Eew. We should get rid of that.

Normally we've been doing this sort of thing by having unprivileged Firefox pages send events to trigger privileged actions, or by having browser chrome watch for click events on such pages... See browser.js's BrowserOnClick(), or attachment 602232 [details] [diff] [review] for an addition to it for about:home.

Want to deal with it here? Or file a blocking/followup bug to do so?

Comment 24

[Andrew McCreight [:mccr8]]

(In reply to Justin Dolske [:Dolske] from comment #23)
> Eew. We should get rid of that.

Great!

> Normally we've been doing this sort of thing by having unprivileged Firefox
> pages send events to trigger privileged actions, or by having browser chrome
> watch for click events on such pages... See browser.js's BrowserOnClick(),
> or attachment 602232 [details] [diff] [review] for an addition to it for
> about:home.
> 
> Want to deal with it here? Or file a blocking/followup bug to do so?

A blocking bug sounds good.  I filed bug 779680.

Comment 25

[Andrew McCreight [:mccr8]]

My game plan here is to first land the wack-a-mole approach, along with test fixes, then backport it to all branches, as appropriate.  Then once the about:neterror is fixed, I can land some of the more complete fixes just on m-c.

Comment 26

[Andrew McCreight [:mccr8]]

Attachment: part 1: fix tests

I removed some UniversalXPConnect calls that didn't seem to be necessary, even with the QI intercepting patch.  I can put them back in if you think they'll be needed with DOM_OBJECT.

Comment 27

[Andrew McCreight [:mccr8]]

Attachment: part 2: add more checks

This adds the UniversalXPConnectCapable() checks at the start of every call that is exposed in the IDL, except for GoOnline().  There are also a few functions that check nsContentUtils::IsCallerTrustedForRead() instead, which appears to be the same thing, but just to try to ensure that a bit I assert that they are capable.

Comment 28

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 648598 [details] [diff] [review]
part 1: fix tests

Nice

Comment 29

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 648599 [details] [diff] [review]
part 2: add more checks


> nsDOMWindowUtils::GetScreenPixelsPerCSSPixel(float* aScreenPixels)
> {
>   *aScreenPixels = 1;
> 
>   if (!nsContentUtils::IsCallerTrustedForRead())
>     return NS_ERROR_DOM_SECURITY_ERR;
>+  MOZ_ASSERT(IsUniversalXPConnectCapable());
>+
I wouldn't add this check. If someone has read access, but not universal, this
asserts hard.
either change nsContentUtils::IsCallerTrustedForRead to IsUniversalXPConnectCapable(),
or don't do anything to nsContentUtils::IsCallerTrustedForRead

Comment 30

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #29)
> I wouldn't add this check. If someone has read access, but not universal,
> this
> asserts hard.
> either change nsContentUtils::IsCallerTrustedForRead to
> IsUniversalXPConnectCapable(),
> or don't do anything to nsContentUtils::IsCallerTrustedForRead

Sure, I'll just remove it.  But I will say that IsCallerTrustedForRead() and IsCallerTrustedForWrite() just call nsContentUtils::CallerHasUniversalXPConnect(), which is identical to IsUniversalXPConnectCapable().  So maybe some followup can merge these a little. ;)

Comment 31

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #30)

> Sure, I'll just remove it.  But I will say that IsCallerTrustedForRead() and
> IsCallerTrustedForWrite() just call
> nsContentUtils::CallerHasUniversalXPConnect(), which is identical to
> IsUniversalXPConnectCapable().  So maybe some followup can merge these a
> little. ;)

Yeah, I did that in bug 713747. ;-)

Comment 32

[Andrew McCreight [:mccr8]]

This patch is causing tests/dom/browser-element/mochitest/test_browserElement_oop_SetVisibleFrames2.html to go perma-orange, but only on OSX. I'm not sure what the deal is.

If I locally change IsUniversalXPConnectCapable() to always just return true, the test passes, but if I change it to this, I get the same behavior, which is a hang:

static bool IsUniversalXPConnectCapable()
{
  bool hasCap = false;
  nsresult rv = nsContentUtils::GetSecurityManager()->
                  IsCapabilityEnabled("UniversalXPConnect", &hasCap);
  return true;
}

Just calling GetSecurityManager() is okay, and NULL checking it doesn't prevent the hang/crash/whatever from calling IsCapabilityEnabled().

Comment 33

[Andrew McCreight [:mccr8]]

It looks like what is happening is that the child process in the test is calling GetCurrentInnerWindowID, which does a security check using IsCapabilityEnabled, which seems to crash or hang the child process.

What is weird is that there are a number of successful calls to the security manager from the child before the failure. I put a printf in the destructor for the security manager, and I don't see that, so I don't think it is being destroyed.

Another odd thing is that the test runs just fine by itself, it just fails as part of the entire directory.

Comment 34

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

Andrew, what is the status on fixing this? Has the perma-orange been fixed?

Comment 35

[Andrew McCreight [:mccr8]]

No, I need to get back to looking into this. I may just split this out into multiple parts.

Comment 36

[Andrew McCreight [:mccr8]]

The weird failure I was getting seems to have gone away, but in the interim there have been some new functions that are unchecked: RemoteFrameFullscreenChanged, RemoteFrameFullscreenReverted, ExitFullscreen. These were added in bug 684620. I'll add checks for them and see if any tests break.

Comment 37

[Andrew McCreight [:mccr8]]

Attachment: part 3: fix more tests

As a bonus, I removed some more uses of enablePrivilege from one of these tests.

Comment 38

[Bobby Holley (:bholley)]

Comment on attachment 659734 [details] [diff] [review]
part 3: fix more tests

FWIW, SpecialPowers.getDOMWindowUtils(window) === SpecialPowers.DOMWindowUtils. No need to bother changing it here though.

r=bholley

Comment 39

[Andrew McCreight [:mccr8]]

Attachment: part 4: add checks to recently added fullscreen methods

Try passed with these checks added. Do you think this will break anything in B2G, Justin? They check to make sure they are only being called from chrome code.

Comment 40

[Justin Lebar (not reading bugmail)]

Comment on attachment 659735 [details] [diff] [review]
part 4: add checks to recently added fullscreen methods

Unfortunately we have no unit tests for this, so I can't say with confidence that this is fine.

But I say you should check this in, and we'll let cpearce know to be on the lookout for regressions.  That's the price you pay for not having tests.

Comment 41

[Justin Lebar (not reading bugmail)]

cc cpearce re comment 40.

Comment 42

[Andrew McCreight [:mccr8]]

For posterity, here is the try run: https://tbpl.mozilla.org/?tree=Try&rev=08498e6da3eb

Comment 43

[Andrew McCreight [:mccr8]]

Thanks for the reviews! Let's hope nothing else broke in the last few days.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bca1869f7ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/6276b65b64f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a99982afb20a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f43436067cb

Comment 44

[Andrew McCreight [:mccr8]]

We're not actually removing the QI yet, so we don't explicitly depend on removing neterror for this short-term solution. I filed bug 790034 to remove goOnline(), which will blocks further hardening of nsDOMWindowUtils.

Comment 45

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Justin Lebar [:jlebar] from comment #40)
> Comment on attachment 659735 [details] [diff] [review]
> part 4: add checks to recently added fullscreen methods
> 
> Unfortunately we have no unit tests for this, so I can't say with confidence
> that this is fine.
> 
> But I say you should check this in, and we'll let cpearce know to be on the
> lookout for regressions.  That's the price you pay for not having tests.

Thanks for the heads up. I tested fullscreen with this bug's changesets, and fullscreen still appears to work. Huzzah!

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4bca1869f7ac
https://hg.mozilla.org/mozilla-central/rev/6276b65b64f1
https://hg.mozilla.org/mozilla-central/rev/a99982afb20a
https://hg.mozilla.org/mozilla-central/rev/6f43436067cb

Should this have a test to confirm that attempting to use the old functionality doesn't work, or is that overkill?

Comment 47

[Andrew McCreight [:mccr8]]

(In reply to Ryan VanderMeulen from comment #46)
> Should this have a test to confirm that attempting to use the old
> functionality doesn't work, or is that overkill?

There's already a test like that for a few functions added by bug 397571. The basic problem here is that people add new functions without adding checks, so they will also not remember to add tests. Perhaps there could be some complicated scheme that checks every known function to make sure it throws, then iterates over nsDOMWindowUtils to ensure there are no extra functions there...

Comment 48

[Alex Keybl [:akeybl]]

Has this not been nominated for Aurora/Beta landing due to risk?

Comment 49

[Andrew McCreight [:mccr8]]

Oh, no, I just need to go back and backport it and make sure the problems I saw before aren't present, I just lost track of it.  The weirdness was with out of process browser element, so I think it shouldn't affect desktop. It shouldn't be too risky.

Comment 50

[Andrew McCreight [:mccr8]]

Attachment: folded patch for beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): many
User impact if declined: possible security risk
Testing completed (on m-c, etc.): it has been on Nightly for about 10 days
Risk to taking this patch (and alternatives if risky): Low, because we're just adding security checks to functions that webpages should never call.
String or UUID changes made by this patch: none

Comment 51

[Andrew McCreight [:mccr8]]

I should add that even though this is marked sec-critical, we're not 100% sure that there's actually anything bad. There's just a giant list of functions (see comment 0) that can be called from webpages that shouldn't be.

Comment 52

[Andrew McCreight [:mccr8]]

Attachment: folded patch for aurora

[Approval Request Comment]
Same as for the beta version.

These patches are basically the same as what was landed, aside from unbitrotting, and for beta, not all of the functions were present.

beta try run: https://tbpl.mozilla.org/?tree=Try&rev=a55d6cc01fce
aurora try run: https://tbpl.mozilla.org/?tree=Try&rev=fe58f79c3092

These try runs look pretty terrible, because Linux builds aren't working now, there are a bunch of 10.8 oranges that are failing even without these patches, and windows builds aren't working for some reason. But it will probably okay...

Comment 53

[Alex Keybl [:akeybl]]

Comment on attachment 663151 [details] [diff] [review]
folded patch for aurora

[Triage Comment]
Approving for Aurora in preparation for Beta consideration.

Comment 54

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d94b98f49426

Comment 55

[Alex Keybl [:akeybl]]

Comment on attachment 663149 [details] [diff] [review]
folded patch for beta

[Triage Comment]
We've now had the opportunity to discuss in triage - let's land on Beta 16 before Tuesday.

Comment 56

[Andrew McCreight [:mccr8]]

Thanks. I guess I should whip up an ESR version, too...

https://hg.mozilla.org/releases/mozilla-beta/rev/df803200c049

Comment 57

[Andrew McCreight [:mccr8]]

Comment on attachment 663149 [details] [diff] [review]
folded patch for beta

The ESR10 version of this is basically the same as the beta one, modulo a few tweaks.
 
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: possible unknown security risks
Fix Landed on Version: 18
Risk to taking this patch (and alternatives if risky): low, these functions shouldn't be called by webpages
String or UUID changes made by this patch: none

Comment 58

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/a06b73f39012

Comment 59

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/c0218c868070

Backed out for causing some additional test failures. One is just a test I need to fix, the other is 

9013 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/bugs/test_bug534149.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: SpecialPowers.wrap is not a function at http://mochi.test:8888/tests/dom/tests/mochitest/bugs/test_bug534149.html:56
JavaScript error: http://mochi.test:8888/tests/dom/tests/mochitest/bugs/test_bug534149.html, line 56: SpecialPowers.wrap is not a function

I'm not sure what that is about. I'll figure it out tomorrow.

Comment 60

[Andrew McCreight [:mccr8]]

Attachment: ESR 10, fix some tests

I just rolled back the stupid SpecialPowers conversion I gratuitiously added in one patch that apparently doesn't work on ESR10, and fixed a test for a function that was probably deleted later that I converted when backporting my patch. Looks like it should be green otherwise. Yay lack of try server. I guess I should have run Mochitest locally.

Comment 61

[Bobby Holley (:bholley)]

Comment on attachment 664311 [details] [diff] [review]
ESR 10, fix some tests

You gotta do what you gotta do (on esr1). r=bholley

Comment 62

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/877420b7c10f
https://hg.mozilla.org/releases/mozilla-esr10/rev/92fa61f340bd

Comment 63

[Daniel Veditz [:dveditz]]

sec-critical seems too severe, almost all of these functions return discreet information and not dangerous objects. The scary ones might be entering modal mode (but could that be worse than a DOS?) and the two that take objects, WrapDOMFile() and CheckAndClearPaintedState() since they might have unknown side-effects especially if an attacker passes in a fake or doctored object.

Nothing looks directly all that bad, this is perhaps more of a stepping stone to be combined with another flaw somewhere. Normally I'd rate that sec-moderate. Any objections? Is it possible to fool WrapDOMFile with a user-created object and then break us elsewhere with a malicious non-file? That seems like the wildcard that might make this sec-high or sec-critical.