Closed Bug 871849 Opened 8 years ago Closed 8 years ago

Crash with iframe, contenteditable, location.replace, GC


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

23 Branch
Not set



Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: jruderman, Assigned: peterv)


(Blocks 1 open bug)


(5 keywords, Whiteboard: [adv-main23-])

Crash Data


(3 files)

1. Install
2. Load the testcase

Result: crash with this=0xdadadadadadadada within js::assertSameCompartment

This bug is similar to bug 869038 in many ways.
Attached file stack (gdb)
Blocks: 869027
(In reply to Jesse Ruderman from comment #0)
> This bug is similar to bug 869038 in many ways.

Yeah, I'd guess it's a dupe.
In bug 869027, Jesse said that fixing the other bug exposed this one, which implies it isn't a dupe.
On Windows: bp-921aa2e9-2da0-46c4-aeb5-6479b2130514.
Crash Signature: [@ js::EncapsulatedPtr<js::BaseShape, unsigned long>::get()] [@ JS_HasPropertyById(JSContext*, JSObject*, int, int*) ]
Keywords: regression
OS: Mac OS X → All
Hardware: x86_64 → All
Version: Trunk → 23 Branch
Assignee: nobody → peterv
Blocks: 871294
Marking sec-critical based on the apparent UAF.
Attached patch v1Splinter Review
Comment on attachment 751041 [details] [diff] [review]

This uses PreserveWrapper (which uses HoldJSObjects) and DropJSObjects for HTMLDocument proxies. It's a bit of a mess, but it'll do for now. I'll file a followup to clean this up.

I've been trying to create a crashtest or a mochitest, but haven't succeeded yet.
Attachment #751041 - Attachment description: WIP → v1
Attachment #751041 - Flags: review?(bzbarsky)
Comment on attachment 751041 [details] [diff] [review]

>+    nsISupports* native = UnwrapDOMObject<nsISupports>(obj);

Do we already have codegen-time python exceptions if someone tries to do a proxy with ownership other than isupports?  If not, please add them.

I guess we already had code in EnsureExpandoObject that relied on this.

Why do we no longer need to RegisterDOMExpandoObject in the overridebuiltins case?  I guess the point of that was tracing and now we handle tracing via the cycle collection mechanism?

Why was this a problem in general?  Was it that adding an expando prop did not preserve the wrapper for this case because we're a proxy binding?  But I'm looking at the expando object registration bits, and we seem to only RemoveDOMExpandoObject from GetAndClearExpandoObject, and that's only called from ReleaseWrapper and reparenting.  So if we're not preserving wrappers for proxies, how do we end up doing RemoveDOMExpandoObject?

Or put more simply, what's the actual problem this patch is trying to fix?
Flags: needinfo?(peterv)
The way it used to work for proxy-based bindings is that we called RegisterDOMExpandoObject with the binding JS object. That puts the JS binding object in a hash in XPConnect which declares it to the GC and the CC. The JS binding object holds the expando object in a reserved slot. XPConnect code traces the binding JS object during a GC, which will trace its slots (tracing the expando object).
Now, for OverrideBuiltins we're storing the expando object in the native (through the ExpandoAndGeneration struct). Registering the binding JS object will not make the expando object be traced, because the reserved slot now holds a private pointer to the ExpandoAndGeneration struct. If nothing traces the expando object then we end up with a stale pointer when it's finalized.

The solution I've chosen is to use the generic CC machinery for registering objects that hold JS objects, but only for OverrideBuiltin proxies.

Eventually we should either make the Register/RemoveDOMExpandoObject mechanism work for all bindings, or remove it and rely on the generic CC mechanism. The Register/RemoveDOMExpandoObject mechanism had the advantage of storing only one pointer in the hash whereas the generic CC mechanism stores two (native and its CC participant).
Flags: needinfo?(peterv)
Comment on attachment 751041 [details] [diff] [review]

Thank you for the explanation.  That helps a lot.  I understand the new setup now, at least.  r=me if we add asserts to the python that proxies are nsISupports and wrappercached, since we're assuming that all over.

But I still don't understand one thing.  For the non-overridebuiltins bindings, when an expando is added we call RegisterDOMExpandoObject with the expando object.  What calls RemoveDOMExpandoObject on that pointer later?  The only caller I see is GetAndClearExpandoObject, which is only called from two places: reparenting and nsContentUtils::ReleaseWrapper.  Do we end up reaching that even though we never called PreserveWrapper?  If so, is that because of the SetPreservingWrapper() in EnsureExpandoObject?
Attachment #751041 - Flags: review?(bzbarsky) → review+
Blocks: 876316
ReleaseWrapper itself only does something if SetPreservingWrapper was called (which is not the same as calling nsContentUtils::PreserveWrapper). There's a number of places where we call ReleaseWrapper but I think all of them call it unconditionally. The goal was certainly to be able to call it like that, to avoid spreading the logic of when it needed to do something over a number of callsites. In particular NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER calls it always.
Like I said, it all should be cleaned up.
Comment on attachment 751041 [details] [diff] [review]

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Probably not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The code that's changed in the patch is fairly obscure. It could be deduced that it's related to expandos.

Which older supported branches are affected by this flaw?

Only aurora and trunk are affected.

If not all supported branches, which bug introduced the flaw?

Bug 855971.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Patch should just apply to aurora.

How likely is this patch to cause regressions; how much testing does it need?

Low risk.
Attachment #751041 - Flags: sec-approval?
Comment on attachment 751041 [details] [diff] [review]

sec-approval+ for trunk. Auto-mail on the approval request got lost somewhere.
Attachment #751041 - Flags: sec-approval? → sec-approval+
Comment on attachment 751041 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 855971
User impact if declined: crashes
Testing completed (on m-c, etc.): landed on inbound
Risk to taking this patch (and alternatives if risky): Should be fairly low risk. Makes us actually mark a live object, instead of allowing it to be GCed.
String or IDL/UUID changes made by this patch: none
Attachment #751041 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 871294
Duplicate of this bug: 876316
Flags: in-testsuite?
Target Milestone: --- → mozilla24
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #751041 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: dom-core-security
Whiteboard: [adv-main23-]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.