Last Comment Bug 775868 - (CVE-2012-3986) several nsDOMWindowUtils methods available to untrusted code
(CVE-2012-3986)
: several nsDOMWindowUtils methods available to untrusted code
Status: RESOLVED FIXED
[advisory-tracking+]
: sec-high
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-20 00:56 PDT by Johnny Stenback (:jst, jst@mozilla.com)
Modified: 2013-01-10 12:35 PST (History)
16 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
fixed
+
fixed
+
fixed
16+
fixed


Attachments
Beginning of a fix. (18.54 KB, patch)
2012-07-20 00:56 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
intercept QI, WIP (13.62 KB, patch)
2012-07-27 14:37 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
intercept QI, WIP (14.60 KB, patch)
2012-07-27 15:04 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 1: fix tests (15.07 KB, patch)
2012-08-02 20:11 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
part 2: add more checks (20.04 KB, patch)
2012-08-02 20:16 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
part 3: fix more tests (4.70 KB, patch)
2012-09-10 09:03 PDT, Andrew McCreight [:mccr8]
bobbyholley: review+
Details | Diff | Splinter Review
part 4: add checks to recently added fullscreen methods (1.61 KB, patch)
2012-09-10 09:06 PDT, Andrew McCreight [:mccr8]
justin.lebar+bug: review+
Details | Diff | Splinter Review
folded patch for beta (37.52 KB, patch)
2012-09-20 13:50 PDT, Andrew McCreight [:mccr8]
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review
folded patch for aurora (38.73 KB, patch)
2012-09-20 13:54 PDT, Andrew McCreight [:mccr8]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
ESR 10, fix some tests (3.83 KB, patch)
2012-09-24 18:24 PDT, Andrew McCreight [:mccr8]
bobbyholley: review+
Details | Diff | Splinter Review

Description Johnny Stenback (:jst, jst@mozilla.com) 2012-07-20 00:56:42 PDT
Created attachment 644216 [details] [diff] [review]
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) (busy with Stylo) 2012-07-20 01:08:41 PDT
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] 2012-07-20 03:48:35 PDT
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) (busy with Stylo) 2012-07-20 04:07:15 PDT
(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, jst@mozilla.com) 2012-07-20 09:41:52 PDT
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, jst@mozilla.com) 2012-07-20 09:43:35 PDT
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) (busy with Stylo) 2012-07-20 09:51:52 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) 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, jst@mozilla.com) 2012-07-20 10:03:04 PDT
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 [:bz] 2012-07-20 14:43:20 PDT
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] 2012-07-26 14:15:30 PDT
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) (busy with Stylo) 2012-07-26 14:21:09 PDT
(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] 2012-07-26 14:41:13 PDT
(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] 2012-07-27 10:48:52 PDT
Here's an attempt a bholley's suggestion from comment 1:
https://tbpl.mozilla.org/?tree=Try&rev=e76955c9f2b2
Comment 13 Andrew McCreight [:mccr8] 2012-07-27 10:53:02 PDT
I locally confirmed that test_bug61098.html fails with a NS_NOINTERFACE error, so that's something. ;)
Comment 14 Andrew McCreight [:mccr8] 2012-07-27 13:50:24 PDT
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] 2012-07-27 14:37:06 PDT
Created attachment 646724 [details] [diff] [review]
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] 2012-07-27 15:03:40 PDT
- 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] 2012-07-27 15:04:42 PDT
Created attachment 646735 [details] [diff] [review]
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) (busy with Stylo) 2012-07-27 15:23:39 PDT
FWIW, I think you can also just do SpecialPowers.DOMWindowUtils.
Comment 19 Andrew McCreight [:mccr8] 2012-07-27 17:31:03 PDT
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] 2012-07-30 09:21:17 PDT
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] 2012-07-30 09:34:57 PDT
(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] 2012-07-30 12:41:02 PDT
(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] 2012-08-01 16:24:29 PDT
(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] 2012-08-01 16:31:36 PDT
(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] 2012-08-02 18:32:28 PDT
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] 2012-08-02 20:11:57 PDT
Created attachment 648598 [details] [diff] [review]
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] 2012-08-02 20:16:07 PDT
Created attachment 648599 [details] [diff] [review]
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] 2012-08-03 02:16:44 PDT
Comment on attachment 648598 [details] [diff] [review]
part 1: fix tests

Nice
Comment 29 Olli Pettay [:smaug] 2012-08-03 02:21:23 PDT
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] 2012-08-03 09:20:45 PDT
(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) (busy with Stylo) 2012-08-03 10:00:39 PDT
(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] 2012-08-03 16:44:50 PDT
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] 2012-08-06 10:40:03 PDT
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] 2012-08-30 13:25:21 PDT
Andrew, what is the status on fixing this? Has the perma-orange been fixed?
Comment 35 Andrew McCreight [:mccr8] 2012-08-30 20:51:00 PDT
No, I need to get back to looking into this. I may just split this out into multiple parts.
Comment 36 Andrew McCreight [:mccr8] 2012-09-07 10:45:26 PDT
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] 2012-09-10 09:03:07 PDT
Created attachment 659734 [details] [diff] [review]
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) (busy with Stylo) 2012-09-10 09:05:45 PDT
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] 2012-09-10 09:06:13 PDT
Created attachment 659735 [details] [diff] [review]
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) 2012-09-10 09:14:18 PDT
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) 2012-09-10 09:14:39 PDT
cc cpearce re comment 40.
Comment 42 Andrew McCreight [:mccr8] 2012-09-10 09:23:13 PDT
For posterity, here is the try run: https://tbpl.mozilla.org/?tree=Try&rev=08498e6da3eb
Comment 44 Andrew McCreight [:mccr8] 2012-09-10 12:52:11 PDT
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) 2012-09-10 16:06:20 PDT
(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] 2012-09-10 18:50:48 PDT
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] 2012-09-10 18:58:22 PDT
(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] 2012-09-19 17:39:02 PDT
Has this not been nominated for Aurora/Beta landing due to risk?
Comment 49 Andrew McCreight [:mccr8] 2012-09-19 22:08:14 PDT
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] 2012-09-20 13:50:13 PDT
Created attachment 663149 [details] [diff] [review]
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] 2012-09-20 13:51:31 PDT
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] 2012-09-20 13:54:46 PDT
Created attachment 663151 [details] [diff] [review]
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] 2012-09-20 15:43:16 PDT
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] 2012-09-20 15:56:28 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/d94b98f49426
Comment 55 Alex Keybl [:akeybl] 2012-09-21 15:50:06 PDT
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] 2012-09-21 15:56:59 PDT
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] 2012-09-21 16:50:35 PDT
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] 2012-09-24 16:49:24 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/a06b73f39012
Comment 59 Andrew McCreight [:mccr8] 2012-09-24 17:41:27 PDT
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] 2012-09-24 18:24:23 PDT
Created attachment 664311 [details] [diff] [review]
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) (busy with Stylo) 2012-09-25 03:02:37 PDT
Comment on attachment 664311 [details] [diff] [review]
ESR 10, fix some tests

You gotta do what you gotta do (on esr1). r=bholley
Comment 63 Daniel Veditz [:dveditz] 2012-10-05 10:28:49 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.