Closed Bug 751454 Opened 8 years ago Closed 8 years ago

GC hazard around mNativeScriptableSharedMap when creating compartments

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox12 --- unaffected
firefox13 + fixed
firefox14 + fixed
firefox15 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: bhackett, Assigned: billm)

References

Details

(Keywords: regression, sec-critical, Whiteboard: [sg:critical][advisory-tracking+])

Attachments

(1 file)

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.
Attached patch patchSplinter Review
This seems to fix the problem.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #621681 - Flags: review?(bobbyholley+bmo)
Comment on attachment 621681 [details] [diff] [review]
patch


> 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+
https://hg.mozilla.org/mozilla-central/rev/3f5473a24f93
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
We should land this on Beta and Aurora so we will never have shipped with this security bug.
Comment on attachment 621681 [details] [diff] [review]
patch

[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]
patch

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+
Beta is open again, please go ahead and land.
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.