bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Fix compartment problems with nsXPConnect::VariantToJS et al

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [sg:needinfo][hardblocker][fixed-in-tracemonkey][has patch])

Attachments

(2 attachments, 5 obsolete attachments)

In theory, nsXPConnect::VariantToJS and all the methods it calls should use 'scope' to determine the correct parent/compartment of any gc-thing created.  Its not too hard to find places where it doesn't do this (where the current context is used, which can have an unrelated scope chain) and this can lead to compartment mismatches (see bug 625251).  Rather than following the transitive closure of this function, I'd like to just remove the 'scope' parameter and ensure that callers have entered the desired compartment with the desired scope chain (which was the fix for bug 625251).  This seems preferable to walking around with a loaded gun.

As a prerequisite, we need to push a dummy frame in AutoCompartment::enter when the origin/destination compartments are the same but the target scope chains' globals are different.
Created attachment 507524 [details] [diff] [review]
part 0: push dummy frame when scope chain changes (but compartment stays the same)
Attachment #507524 - Flags: review?(mrbkap)
Attachment #507524 - Attachment description: push dummy frame when scope chain changes (but compartment stays the same) → part 0: push dummy frame when scope chain changes (but compartment stays the same)
Created attachment 507527 [details] [diff] [review]
part 1: take out compartment hack (asserts b/c no request), just getPrivate()
Attachment #507527 - Flags: review?(mrbkap)
Created attachment 507528 [details] [diff] [review]
part 2: s/JS_GetGlobalObject/JS_GetGlobalForScopeChain/ in a few places
Attachment #507528 - Flags: review?(mrbkap)
Attachment #507527 - Attachment description: take out compartment hack (asserts b/c no request), just getPrivate() → part 1: take out compartment hack (asserts b/c no request), just getPrivate()
Created attachment 507532 [details] [diff] [review]
part 3: enter the right compartment for NativeData2JS/VariantData2JS

So actually removing the scope argument isn't right because sometimes the target scope is different than the current scope chain (although in the same compartment).  This can cause, e.g., QueryInterface to mutate the wrong XPCWrappedNative.  Thus, this patch keeps the explicit scope, but puts in AutoEnterCompartment where missing (so, fixes potential bugs) and asserts the  same-compartment-ness property stated above.
Attachment #507532 - Flags: review?(mrbkap)
Not sure this is exploitable, but if it is it's probably exploitable on the 1.9.x branches and we won't be able to use compartments to fix it there. CC'ing moz_bug_r_a4 in case he wants to play with this.
Whiteboard: [sg:needinfo]
Green on try.

Assuming some of the fixes in these patches can cause compartment mismatches, then this should be as exploitable as any compartment mismatch.  I don't believe branches are affected.
Summary: Remove explicit 'scope' parameter from nsXPConnect::VariantToJS et al; callers should enter compartment → Fix compartment problems with nsXPConnect::VariantToJS et al
Comment on attachment 507524 [details] [diff] [review]
part 0: push dummy frame when scope chain changes (but compartment stays the same)

I forgot that this patch isn't actually necessary once I backed off the 'explicit scope parameter removal' idea.  Also, until we remove dummy frames (bug 625199), it could add a non-trivial perf overhead.
Attachment #507524 - Attachment is obsolete: true
Attachment #507524 - Flags: review?(mrbkap)
Blocks: 629822
Created attachment 508953 [details] [diff] [review]
part 2: s/JS_GetGlobalObject/JS_GetGlobalForScopeChain/ in a few places (v.2)

Updating; a few new uses of GetNativeGlobal have popped up.  In theory, all these usage sites are potentially getting the wrong scope.
Attachment #507528 - Attachment is obsolete: true
Attachment #508953 - Flags: review?(mrbkap)
Attachment #507528 - Flags: review?(mrbkap)

Updated

8 years ago
Attachment #507527 - Flags: review?(mrbkap) → review+

Updated

8 years ago
Attachment #507532 - Flags: review?(mrbkap) → review+
As comment 6 says, this used to be green on try but now asserts at the
  PR_Assert("!scope || scope->compartment() == GetJSContext()->compartment")
added to NativeData2JS.

I think the problem is the recent change in GetWrappedNativeOfJSObject:
  JSObject* funObjParent = funobj->getParent()->unwrap();
which ultimately causes mCurrentJSObject to be the unwrapped object (i.e., mCurrentJSObject == mFlattenedJSObject).

It seems that we need to either fix the problem warranting
  http://hg.mozilla.org/tracemonkey/diff/a2825fbe23e3/js/src/xpconnect/src/xpcwrappednative.cpp
a different way or stop using GetCurrentJSObject as the scope passed to Native*To*.  Note: practically all uses of GetCurrentJSObject are for Native*To* functions, so this is suspicious.
Also, this assert is catching (much earlier) the compartment mismatch in bug 629822 which itself apparently is hitting a lot for sfink with Firebug.

Updated

8 years ago
blocking2.0: --- → betaN+
Whiteboard: [sg:needinfo] → [sg:needinfo][hardblocker]

Updated

8 years ago
Whiteboard: [sg:needinfo][hardblocker] → [sg:needinfo][hardblocker][has patch]
Has patches, but missing the key patch.
Whiteboard: [sg:needinfo][hardblocker][has patch] → [sg:needinfo][hardblocker]
Blocks: 631409
Blocks: 629775
Blocks: 630875
Duplicate of this bug: 631409
Duplicate of this bug: 629822
Created attachment 510012 [details] [diff] [review]
part 3: WIP 1

I spent a bit of time trying to understand what is meant by all the JSObject*'s maintained by the XPCCallContext (viz., mCurrentJSObject, mOperandJSObject, mFlattenedJSObject, mCallee).  My conclusion is that 3 of these fields (mCallee, mCurrentJSObject, mOperandJSObject) can be replaced by a single new field mScopeForNewJSObjects which is mostly what mOperandJSObject was but it gets set in cases where it would currently be left null.

A nice resulting simplification is that I can go back and remove the explicit 'scope' param but, instead of using JS_GetScopeChainForGlobal (which was problematic), just use the new mScopeForNewJSObjects field.

I still need to try-server, but the patch seems to run a browser fine and fix the Firebug/AdBlock compartment asserts (once you comment out an unrelated compartment assert in JS_GetStringCharsAndLength that sfink patched in another bug that escapes me atm).
Attachment #507532 - Attachment is obsolete: true
Created attachment 510085 [details] [diff] [review]
part 2: use the right compartment for NativeData2JS et al

Removed a few overzealous asserts in the WIP patch.  Fixed TEST-UNEXPECTED-PASS whose assertion incidentally was fixed by the patch (for reference, its the crash test from bug 512815).  With that, looks green on try, but pushing again to verify.
Attachment #510012 - Attachment is obsolete: true
Attachment #510085 - Flags: review?(mrbkap)
Whiteboard: [sg:needinfo][hardblocker] → [sg:needinfo][hardblocker][has patch]
Comment on attachment 508953 [details] [diff] [review]
part 2: s/JS_GetGlobalObject/JS_GetGlobalForScopeChain/ in a few places (v.2)

>diff --git a/dom/base/nsIScriptContext.h b/dom/base/nsIScriptContext.h
>-// a7139c0e-962c-44b6-bec3-e4166bfe84eb
>+// 90989b94-f40e-4da7-939a-82fdfdc1c867
> #define NS_ISCRIPTCONTEXT_IID \
>-{ 0xa7139c0e, 0x962c, 0x44b6, \
>-  { 0xbe, 0xc3, 0xe4, 0x16, 0x6b, 0xfe, 0x84, 0xeb } }
>+{ 0x90989b94, 0xf40e, 0x4da7, \
>+  { 0x93, 0x9a, 0x82, 0xfd, 0xfd, 0xc1, 0xc8, 0x67 } }

We probably can't change nsIScriptContext at this late point. We need an nsIScriptContext_MOZILLA_2_0 or somesuch instead.

>diff --git a/dom/base/nsJSEnvironment.cpp b/dom/base/nsJSEnvironment.cpp
>-nsJSContext::GetNativeGlobal()
>+nsJSContext::GetGlobalForScopeChain()
>+{
>+    JSAutoRequest ar(mContext);
>+    return ::JS_GetGlobalForScopeChain(mContext);

Let's not add new ::JS junk where it's not needed. IMO it's pretty ugly.

>+void *
>+nsJSContext::GetContextGlobal()

We should file a bug on figuring out who really wants the outer window here. It's almost always not what you want.

Clearing the review request based on seeing the new interface.
Attachment #508953 - Flags: review?(mrbkap)
(In reply to comment #16)
> Let's not add new ::JS junk where it's not needed. IMO it's pretty ugly.

Agreed; just copying surrounding style.

> We should file a bug on figuring out who really wants the outer window here.
> It's almost always not what you want.

Sure; this patch isn't actually a pre-requisite, but just a set of related bugs found while rooting around.
Attachment #508953 - Attachment is obsolete: true
Attachment #510085 - Attachment description: part 3: use the right compartment for NativeData2JS et al → part 2: use the right compartment for NativeData2JS et al
Comment on attachment 510085 [details] [diff] [review]
part 2: use the right compartment for NativeData2JS et al

What's with the /* TODO */ comments?

Should probably do PR_ASSERT -> NS_ASSERTION (or at least NS_ABORT_IF_FALSE).

Looks good otherwise.
Attachment #510085 - Flags: review?(mrbkap) → review+
(In reply to comment #19)
> What's with the /* TODO */ comments?

Oh yeah, I put those in to remind myself to write comments explaining what they meant.

> Should probably do PR_ASSERT -> NS_ASSERTION (or at least NS_ABORT_IF_FALSE).

Ah, I see the style guideline now.  Bummer, PR_ASSERT was so much shorter...
Duplicate of this bug: 633149
http://hg.mozilla.org/tracemonkey/rev/d518bc36d7b4
http://hg.mozilla.org/tracemonkey/rev/02be97f9ef0d
Whiteboard: [sg:needinfo][hardblocker][has patch] → [sg:needinfo][hardblocker][fixed-in-tracemonkey]

Updated

8 years ago
Whiteboard: [sg:needinfo][hardblocker][fixed-in-tracemonkey] → [sg:needinfo][hardblocker][fixed-in-tracemonkey][has patch]
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 634594
Group: core-security
You need to log in before you can comment on or make changes to this bug.