GC hazard around mNativeScriptableSharedMap when creating compartments

RESOLVED FIXED in Firefox 13



5 years ago
5 years ago


(Reporter: bhackett, Assigned: billm)


({regression, sec-critical})

Mac OS X
regression, sec-critical
Dependency tree / graph

Firefox Tracking Flags

(firefox12 unaffected, firefox13+ fixed, firefox14+ fixed, firefox15 fixed, firefox-esr10 unaffected)


(Whiteboard: [sg:critical][advisory-tracking+])


(1 attachment)



5 years ago
Noticed this while building a browser with gczeal stuff baked in.  The entries in the mNativeScriptableSharedMap map in XPCJSRuntime are weakly held, and are cleared if there are no objects referring back to them.  These entries entrain JS classes, however, and if a GC is triggered between the point where the scriptable info is constructed and added to the map and the point where some JS thing is created which refers back to that map, then the JS class pointer becomes garbage.

A stack snippet is below for how the GC is triggered by construction of the JS-internal BaseShape which holds onto the class pointer, and that GC wipes out the class pointer for the BaseShape being constructed (presumably a GC at several other points under JS_NewGlobalObject could trigger the same thing, but I can't tell under which conditions the XPCNativeScriptableShared will actually be marked).

XPCWrappedNative::WrapNewGlobal should ensure that the XPCNativeScriptableInfo it constructs lives at least until the xpc_CreateGlobalObject call it makes finishes.

#0  JSClassSweeper (table=0x14b970, hdr=0x14b9f0, number=0, arg=0x0) at /Users/brianhackett/mozilla-inbound2/js/xpconnect/src/XPCJSRuntime.cpp:207
#1  0x03017307 in JS_DHashTableEnumerate (table=0x14b970, etor=0x20975db <JSClassSweeper(JSDHashTable*, JSDHashEntryHdr*, unsigned int, void*)>, arg=0x0) at /Users/brianhackett/mozilla-inbound2/js/src/jsdhash.cpp:745
#2  0x0209b937 in XPCNativeScriptableSharedMap::Enumerate (this=0x14b960, f=0x20975db <JSClassSweeper(JSDHashTable*, JSDHashEntryHdr*, unsigned int, void*)>, arg=0x0) at XPCMaps.h:590
#3  0x0209649d in XPCJSRuntime::FinalizeCallback (fop=0xbfffd920, status=JSFINALIZE_END) at /Users/brianhackett/mozilla-inbound2/js/xpconnect/src/XPCJSRuntime.cpp:819
#4  0x0303a5b8 in SweepPhase (rt=0x9d7200, gckind=js::GC_NORMAL, startBackgroundSweep=0xbfffd9df) at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:3245
#5  0x0303a9f5 in GCCycle (rt=0x9d7200, incremental=false, budget=0, gckind=js::GC_NORMAL) at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:3626
#6  0x0303ad83 in Collect () at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:3715
#7  0x0303afc9 in js::GC (rt=0x9d7200, gckind=js::GC_NORMAL, reason=js::gcreason::DEBUG_GC) at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:3736
#8  0x0303b289 in js::gc::RunLastDitchGC (cx=0x15d200, reason=js::gcreason::DEBUG_GC) at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:1659
#9  0x0303b2e1 in js::gc::RunDebugGC (cx=0x15d200) at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:3955
#10 0x031324f5 in js::gc::NewGCThing<js::BaseShape> (cx=0x15d200, kind=js::gc::FINALIZE_BASE_SHAPE, thingSize=32) at jsgcinlines.h:447
#11 0x031325e6 in js_NewGCBaseShape (cx=0x15d200) at jsgcinlines.h:537
#12 0x03128c00 in js::BaseShape::getUnowned (cx=0x15d200, base=@0xbfffdc4c) at /Users/brianhackett/mozilla-inbound2/js/src/jsscope.cpp:1206
#13 0x03128f20 in js::EmptyShape::getInitialShape (cx=0x15d200, clasp=0x15d704, proto=0x0, parent=0x0, kind=js::gc::FINALIZE_OBJECT16, objectFlags=0) at /Users/brianhackett/mozilla-inbound2/js/src/jsscope.cpp:1331
#14 0x030bf502 in NewObject (cx=0x15d200, clasp=0x15d704, type=0x19000020, parent=0x0, kind=js::gc::FINALIZE_OBJECT16) at /Users/brianhackett/mozilla-inbound2/js/src/jsobj.cpp:2754
#15 0x030c0874 in js::NewObjectWithGivenProto (cx=0x15d200, clasp=0x15d704, proto=0x0, parent=0x0, kind=js::gc::FINALIZE_OBJECT16) at /Users/brianhackett/mozilla-inbound2/js/src/jsobj.cpp:2806
#16 0x031786c2 in js::NewObjectWithGivenProto (cx=0x15d200, clasp=0x15d704, proto=0x0, parent=0x0) at jsobjinlines.h:1459
#17 0x031d7842 in js::GlobalObject::create (cx=0x15d200, clasp=0x15d704) at /Users/brianhackett/mozilla-inbound2/js/src/vm/GlobalObject.cpp:274
#18 0x02fb2010 in JS_NewGlobalObject (cx=0x15d200, clasp=0x15d704) at /Users/brianhackett/mozilla-inbound2/js/src/jsapi.cpp:3235
#19 0x02fb20f2 in JS_NewCompartmentAndGlobalObject (cx=0x15d200, clasp=0x15d704, principals=0x14f064) at /Users/brianhackett/mozilla-inbound2/js/src/jsapi.cpp:3268
#20 0x0205f565 in CreateNewCompartment (cx=0x15d200, clasp=0x15d704, principal=0x14f060, priv=0x15d800, global=0xbfffe094, compartment=0xbfffe090) at /Users/brianhackett/mozilla-inbound2/js/xpconnect/src/nsXPConnect.cpp:1153
#21 0x020606fd in xpc_CreateGlobalObject (cx=0x15d200, clasp=0x15d704, principal=0x14f060, ptr=0x0, wantXrays=false, global=0xbfffe094, compartment=0xbfffe090) at /Users/brianhackett/mozilla-inbound2/js/xpconnect/src/nsXPConnect.cpp:1255
#22 0x020bd5d7 in XPCWrappedNative::WrapNewGlobal (ccx=@0xbfffe11c, nativeHelper=@0xbfffe1d8, principal=0x14f060, initStandardClasses=false, wrappedGlobal=0xbfffe208) at /Users/brianhackett/mozilla-inbound2/js/xpconnect/src/XPCWrappedNative.cpp:377
Yeah, this is probably a regression from bug 720580, where I added WrapNewGlobal. I'll try to get a fix soon.

Do we generally consider timing-related GC bugs security bugs? Or are they too difficult to exploit?
Depends on: 715761
I think we need to add an AutoMarkingPtr for XPCNativeScriptableInfo. Bill has some patches to refactor that stuff over in bug 715761, and I don't want to bitrot them. Let me see if I can cajole him into landing them.

Comment 3

5 years ago
Created attachment 621681 [details] [diff] [review]

This seems to fix the problem.
Assignee: nobody → wmccloskey
Attachment #621681 - Flags: review?(bobbyholley+bmo)
Comment on attachment 621681 [details] [diff] [review]

> typedef TypedAutoMarkingPtr<XPCNativeInterface> AutoMarkingNativeInterfacePtr;
> typedef TypedAutoMarkingPtr<XPCNativeSet> AutoMarkingNativeSetPtr;
> typedef TypedAutoMarkingPtr<XPCWrappedNative> AutoMarkingWrappedNativePtr;
> typedef TypedAutoMarkingPtr<XPCWrappedNativeTearOff> AutoMarkingWrappedNativeTearOffPtr;
> typedef TypedAutoMarkingPtr<XPCWrappedNativeProto> AutoMarkingWrappedNativeProtoPtr;
> typedef TypedAutoMarkingPtr<XPCMarkableJSVal> AutoMarkingJSVal;
>+typedef TypedAutoMarkingPtr<XPCNativeScriptableInfo> AutoMarkingNativeScriptableInfoPtr;
> template<class T>
> class ArrayAutoMarkingPtr : public AutoMarkingPtr
> {
>   public:
>     ArrayAutoMarkingPtr(XPCCallContext& ccx)
>       : AutoMarkingPtr(ccx), mPtr(nsnull), mCount(0) {}
>     ArrayAutoMarkingPtr(XPCCallContext& ccx, T** ptr, PRUint32 count, bool clear)

Just below here, there's a comment explaining why there's no AutoMarkingNativeScriptableInfoPtr. Please remove it. :-)

r=bholley. Thanks for taking care of this one. :-)
Attachment #621681 - Flags: review?(bobbyholley+bmo) → review+

Comment 5

5 years ago
Target Milestone: --- → mozilla15

Comment 6

5 years ago
Last Resolved: 5 years ago
Resolution: --- → FIXED
We should land this on Beta and Aurora so we will never have shipped with this security bug.
Blocks: 720580
status-firefox-esr10: --- → unaffected
status-firefox12: --- → unaffected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → fixed
tracking-firefox13: --- → +
tracking-firefox14: --- → +
Keywords: regression, sec-critical
Whiteboard: [sg:critical]

Comment 8

5 years ago
Comment on attachment 621681 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 720580
User impact if declined: Possible crashes, but they're unlikely.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Very low risk.
String or UUID changes made by this patch: None
Attachment #621681 - Flags: approval-mozilla-beta?
Attachment #621681 - Flags: approval-mozilla-aurora?
Comment on attachment 621681 [details] [diff] [review]

low risk, sg:crit - approved for branches.
Attachment #621681 - Flags: approval-mozilla-beta?
Attachment #621681 - Flags: approval-mozilla-beta+
Attachment #621681 - Flags: approval-mozilla-aurora?
Attachment #621681 - Flags: approval-mozilla-aurora+

Comment 10

5 years ago
Landed on Aurora. Beta is closed for some reason.

Beta is open again, please go ahead and land.

Comment 12

5 years ago
status-firefox13: affected → fixed
status-firefox14: affected → fixed
Whiteboard: [sg:critical] → [sg:critical][advisory-tracking+]
Is there something QA can do to verify this fix? If not, @bhackett, can you help us with verifying this is fixed?
Brian, do you have an answer for QA in comment 13?
Group: core-security
You need to log in before you can comment on or make changes to this bug.