762280 - (CVE-2012-3963) use after free in js::gc::MapAllocToTraceKind

Status: RESOLVED FIXED

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

Description

[Abhishek Arya]

Attachment: Testcase, from mutation of existing long test.

Reproduces on trunk.
20120605092342
http://hg.mozilla.org/mozilla-central/rev/c76497029f0d

=================================================================
==16916== ERROR: AddressSanitizer global-buffer-overflow on address 0x7fab8bdc5bb0 at pc 0x7fab8ab5e87d bp 0x7fab789bbdd0 sp 0x7fab789bbdc8
READ of size 4 at 0x7fab8bdc5bb0 thread T2
    #0 0x7fab8ab5e87d in js::gc::MapAllocToTraceKind(js::gc::AllocKind) /usr/local/google/home/aarya/firefox/src/js/src/jsfriendapi.cpp:0
    #1 0x7fab88c80099 in nsXMLHttpRequest::cycleCollection::Trace(void*, void (*)(void*, char const*, void*), void*) /usr/local/google/home/aarya/firefox/src/content/base/src/nsXMLHttpRequest.cpp:663
    #2 0x7fab88c7c9ca in nsXHREventTarget::cycleCollection::Traverse(void*, nsCycleCollectionTraversalCallback&) /usr/local/google/home/aarya/firefox/src/content/base/src/nsXMLHttpRequest.cpp:287
    #3 0x7fab8a2cf99e in nsCycleCollector::MarkRoots(GCGraphBuilder&) /usr/local/google/home/aarya/firefox/src/xpcom/base/nsCycleCollector.cpp:2054
    #4 0x7fab8a2d1db7 in nsCycleCollector::BeginCollection(nsICycleCollectorListener*) /usr/local/google/home/aarya/firefox/src/xpcom/base/nsCycleCollector.cpp:2760
    #5 0x7fab8a2d667e in nsCycleCollectorRunner::Run() /usr/local/google/home/aarya/firefox/src/xpcom/base/nsCycleCollector.cpp:3049
    #6 0x7fab8a2b0824 in nsThread::ProcessNextEvent(bool, bool*) /usr/local/google/home/aarya/firefox/src/xpcom/threads/nsThread.cpp:624
    #7 0x7fab8a21e16d in NS_ProcessNextEvent_P(nsIThread*, bool) /usr/local/google/home/aarya/firefox/src/objdir-ff-asan/xpcom/build/nsThreadUtils.cpp:213
    #8 0x7fab8a2aebbd in nsThread::ThreadFunc(void*) /usr/local/google/home/aarya/firefox/src/xpcom/threads/nsThread.cpp:256
    #9 0x7fab8edcf706 in _pt_root /usr/local/google/home/aarya/firefox/src/nsprpub/pr/src/pthreads/ptthread.c:158
0x7fab8bdc5bb0 is located 0 bytes to the right of global variable 'js::gc::MapAllocToTraceKind(js::gc::AllocKind)::map (/usr/local/google/home/aarya/firefox/src/js/src/jsfriendapi.cpp)' (0x7fab8bdc5b60) of size 80
  'js::gc::MapAllocToTraceKind(js::gc::AllocKind)::map (/usr/local/google/home/aarya/firefox/src/js/src/jsfriendapi.cpp)' is ascii string ''
==16916== ABORTING
Stats: 186M malloced (195M for red zones) by 404853 calls
Stats: 46M realloced by 21407 calls
Stats: 156M freed by 274895 calls
Stats: 24M really freed by 46335 calls
Stats: 412M (105532 full pages) mmaped in 103 calls
  mmaps   by size class: 8:311277; 9:57337; 10:24570; 11:22517; 12:4096; 13:2560; 14:1792; 15:512; 16:640; 17:160; 18:208; 19:56; 20:20;
  mallocs by size class: 8:298456; 9:55207; 10:22104; 11:19974; 12:4008; 13:2088; 14:1570; 15:393; 16:638; 17:146; 18:196; 19:55; 20:18;
  frees   by size class: 8:188758; 9:43497; 10:18577; 11:16765; 12:2858; 13:1836; 14:1370; 15:330; 16:533; 17:126; 18:181; 19:50; 20:14;
  rfrees  by size class: 8:37269; 9:2590; 10:2242; 11:3443; 12:169; 13:216; 14:139; 15:48; 16:159; 17:29; 18:21; 19:9; 20:1;
Stats: malloc large: 415 small slow: 2219
Shadow byte and word:
  0x1ff5717b8b76: f9
  0x1ff5717b8b70: 00 00 00 00 00 00 f9 f9
More shadow bytes:
  0x1ff5717b8b50: 00 00 00 00 06 f9 f9 f9
  0x1ff5717b8b58: f9 f9 f9 f9 05 f9 f9 f9
  0x1ff5717b8b60: f9 f9 f9 f9 00 06 f9 f9
  0x1ff5717b8b68: f9 f9 f9 f9 00 00 00 00
=>0x1ff5717b8b70: 00 00 00 00 00 00 f9 f9
  0x1ff5717b8b78: f9 f9 f9 f9 00 00 00 00
  0x1ff5717b8b80: 00 00 00 00 00 00 00 00
  0x1ff5717b8b88: 00 00 00 00 00 00 00 00
  0x1ff5717b8b90: 00 00 00 00 00 00 00 00

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Here's what's happening here. nsXMLHttpRequest has a JSObject* field called mResultArrayBuffer. It's supposed to get traced via NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN. However, in this case we're not calling HoldJSObjects on the nsXMLHttpRequest, so the JS GC is freeing the JSObject while the XHR thinks it still owns it.

We're supposed to be calling HoldJSObjects from RootResultArrayBuffer, which calls nsContentUtils::PreserveWrapper, which calls HoldJSObjects. However, that function returns early if the wrapper is already preserved; that's what's happening here.

It's getting preserved from generated code, inside of mozilla::dom::XMLHttpRequestBinding::_addProperty, which looks like this:

  nsXMLHttpRequest* self = UnwrapDOMObject<nsXMLHttpRequest>(obj);

  JSCompartment* compartment = js::GetObjectCompartment(obj);
  xpc::CompartmentPrivate* priv =
    static_cast<xpc::CompartmentPrivate*>(JS_GetCompartmentPrivate(compartment));
  if (!priv->RegisterDOMExpandoObject(obj)) {
    return false;
  }
  self->SetPreservingWrapper(true);
  return true;

I'm not sure what the right solution is here. Should we stop relying on PreserveWrapper to hold the JS objects? Or should we fix the new DOM bindings so that they call HoldJSObjects? Or something else?

Anyway, this seems like a pretty serious bug to me. I'm not sure when the new XHR bindings landed, but I'm guessing that's when this regressed.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Those landed for Firefox 14.  See bug 740069.

Sounds like _addProperty should perhaps make the relevant HoldJSObjects call...

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oh, one more thing. If you run this test case in a debug build, it asserts in some layout code. I had to comment out the assertion to see the more serious problem. I think the assertion is unrelated, but someone in layout should probably look at it.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Sounds like _addProperty should perhaps make the relevant HoldJSObjects
> call...

If so, we should probably add a comment to nsWrapperCache::SetPreservingWrapper saying something like "Never call this directly; use nsContentUtils instead." I hate having to do stuff like that, though, so it would be nice if there were a better way.

Comment 5

[Andrew McCreight [:mccr8]]

I also see a call to SetPreservingWrapper(true) in ListBase<LC>::ensureExpandoObject.  It looks to me like that object is rooted by RegisterDOMExpandoObject.

Comment 6

[Andrew McCreight [:mccr8]]

Oh, right.  I misunderstood the problem.  To spell it out a bit more, the JSObject wrapper itself is held alive by the ensureExpandoObject, which serves the same role as NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER.

nsXMLHttpRequest's Trace function visits two other JS fields, mResultArrayBuffer and mResultJSON, which it expects to be kept alive, but the XHR isn't added as a JS holder, so the GC don't call the trace function, so the GC doesn't know to keep those two fields alive.

It is okay to set SetPreservingWrapper(true) on your wrapper if you aren't a JS holder, but only if you don't have a Trace method.  That seems to be the case for ListBase, or at least a quick search couldn't find a Trace method for that function.

Comment 7

[Boris Zbarsky [:bzbarsky]]

When you say "Trace" method, are you talking about a JSAPI class hook, or CC stuff, or something else?

Comment 8

[Andrew McCreight [:mccr8]]

Sorry, I meant the CC stuff.  NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN and NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED.  When a C++ thing is added as a JS holder, its CC trace function gets invoked to find what JS objects it keeps alive, is what I mean.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Sorry, I meant the CC stuff. 

Ah.  The ListBase itself doesn't have that, of course: it's a singleton proxy handler.  The actual objects are various things like nsContentList, which have a wrapper cache and should definitely have Trace stuff attached to their CC.

Comment 10

[Peter Van der Beken [:peterv]]

We could go two ways with this, either start using HoldJSObjects to root/trace non-wrapper members or add a trace hook that calls the CC participant's Trace. I personally am more in favor of the second, but ideally we'd then need to have a way to only add the trace hook if we want to trace more than just the wrapper, and that seems a bit hard to know without hardcoding it in the config file.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

+1 to everything in comment 10.  I think sticking this in the config file is fine.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't understand most of the discussion here, but I'd like to point out that getting tracing correct is really hard as it is. Especially since testing reliably is hard. So relying on people to additionally remember to put something in the config file in order to avoid exploitable crashes scares me.

Comment 13

[Andrew McCreight [:mccr8]]

Has anybody had a change to look at this?  Probably too late to fix for 14, but it looks bad.  (XHR isn't properly rooting a JS object it holds onto.)

Comment 14

[Andrew McCreight [:mccr8]]

change/chance

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Uh, it really sucks that we dropped the ball on this.

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: v1

Let's fix this the slow but safe way for now. I'll file a followup bug to figure out a better approach with a real trace hook.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 640985 [details] [diff] [review]
v1

r=me if you add a check on our class having the nsISupports bool set.  Or do we not even codegen this hook for non-nsISupports things?  If so, we could just have a comment about that here, and one at the place where that check is done...  Seems like we generate this for all concrete non-worker wrappercached things.  Does that imply nsISupports?

Comment 18

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/170854ffe163

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 640985 [details] [diff] [review]
v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 740069
User impact if declined: crashes
Testing completed (on m-c, etc.): landed on mozilla-inbound
Risk to taking this patch (and alternatives if risky): low risk, this reuses existing wrapper preservation code which we already use for all other XPConnect bindings
String or UUID changes made by this patch: none

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 640985 [details] [diff] [review]
v1

approving for aurora now, we'll need more testing before approving for beta.

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/170854ffe163

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 640985 [details] [diff] [review]
v1

Can we land this on branches now?  Let's get this into beta 2 for testing.

Comment 23

[Andrew McCreight [:mccr8]]

This was actually landed on Aurora on 7/18.

https://hg.mozilla.org/releases/mozilla-aurora/rev/85275cda0f19

Comment 24

[Andrew McCreight [:mccr8]]

Peter's Aurora patch applied cleanly to Beta, so I went ahead and landed it.

https://hg.mozilla.org/releases/mozilla-beta/rev/327b951ff6a8

Comment 26

[Ioana (away)]

I tried to verify this issue on Ubuntu 12.04 64 bit with the latest Firefox 17 debug build (ftp://ftp.mozilla.org/pub/firefox/nightly/2012-11-13-mozilla-beta-debug).

Unfortunately, when I try to see the bug on Firefox 14 by uploading Arya's testcase, I get this crash:
https://crash-stats.mozilla.com/report/index/bc7dc334-4b35-48ba-9566-6021b2121113

When I try to upload the same testcase in the Firefox 17 build mentioned above, I get the following crash:
Fx 17: https://crash-stats.mozilla.com/report/index/8043d663-0e5b-4746-96b3-856da2121113


Are there any workarounds that could allow QA to verify this bug?

Comment 27

[u279076]

This can't be tested with a normal debug build, afaik. It requires an ASAN build. I'm reassigning this to Matt to see if he can help verify.

Comment 28

[Matt Wobensmith [:mwobensmith][:matt:]]

I'm getting the same crash as Ioana is, above, using latest ASAN builds for Aurora and Nightly, on Ubuntu 11.10 from 2012-11-14.

AFAIC, it is specific to the testcase in this bug.

Comment 29

[Jesse Ruderman]

Matt, can you file a new bug for the crash you're hitting?

Peter, what would a reduced testcase for this bug look like?  I want to know if my fuzzer is missing something, and I'd like this bug to be regression-tested.

Comment 30

[Matt Wobensmith [:mwobensmith][:matt:]]

Filed new bug 821360 for the current crash.

Comment 31

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Please see comment 3 about the crash in CSS code. It's unrelated to this problem.

Comment 32

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719

Comment 33

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old