Bug 762280 (CVE-2012-3963)

use after free in js::gc::MapAllocToTraceKind

RESOLVED FIXED in Firefox 15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Abhishek Arya, Assigned: peterv)

Tracking

(Blocks: 1 bug, {regression, sec-critical})

Trunk
mozilla17
x86_64
Linux
regression, sec-critical
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox13 unaffected, firefox14+ affected, firefox15+ fixed, firefox16+ fixed, firefox17 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [asan][advisory-tracking+] new in Fx14)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 630740 [details]
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
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Whiteboard: [asan]
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.
Assignee: general → nobody
Component: JavaScript Engine → DOM
QA Contact: general → general
Keywords: sec-critical
Those landed for Firefox 14.  See bug 740069.

Sounds like _addProperty should perhaps make the relevant HoldJSObjects call...
Blocks: 740069
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
status-firefox13: --- → unaffected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
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.
(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.
I also see a call to SetPreservingWrapper(true) in ListBase<LC>::ensureExpandoObject.  It looks to me like that object is rooted by RegisterDOMExpandoObject.
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.
When you say "Trace" method, are you talking about a JSAPI class hook, or CC stuff, or something else?
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.
> 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.
(Assignee)

Comment 10

5 years ago
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.
+1 to everything in comment 10.  I think sticking this in the config file is fine.
tracking-firefox14: ? → +
tracking-firefox15: ? → +
tracking-firefox16: ? → +
Keywords: regression
Whiteboard: [asan] → [asan] new in Fx14
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.
Assignee: nobody → peterv
status-firefox-esr10: --- → unaffected
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.)
change/chance
Uh, it really sucks that we dropped the ball on this.
(Assignee)

Comment 16

5 years ago
Created attachment 640985 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Attachment #640985 - Flags: review?(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?
Attachment #640985 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/170854ffe163
Target Milestone: --- → mozilla17
(Assignee)

Comment 19

5 years ago
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
Attachment #640985 - Flags: approval-mozilla-beta?
Attachment #640985 - Flags: approval-mozilla-aurora?
Comment on attachment 640985 [details] [diff] [review]
v1

approving for aurora now, we'll need more testing before approving for beta.
Attachment #640985 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/170854ffe163
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox17: --- → fixed
Resolution: --- → FIXED
Comment on attachment 640985 [details] [diff] [review]
v1

Can we land this on branches now?  Let's get this into beta 2 for testing.
Attachment #640985 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This was actually landed on Aurora on 7/18.

https://hg.mozilla.org/releases/mozilla-aurora/rev/85275cda0f19
status-firefox16: affected → fixed
Peter's Aurora patch applied cleanly to Beta, so I went ahead and landed it.

https://hg.mozilla.org/releases/mozilla-beta/rev/327b951ff6a8
status-firefox15: affected → fixed
Whiteboard: [asan] new in Fx14 → [asan][advisory-tracking+] new in Fx14
Summary: Global-buffer-overflow in js::gc::MapAllocToTraceKind → use after free in js::gc::MapAllocToTraceKind
Alias: CVE-2012-3963
Keywords: verifyme
Group: core-security

Updated

5 years ago
QA Contact: ioana.budnar

Comment 26

5 years ago
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?

Updated

5 years ago
Keywords: verifyme
Whiteboard: [asan][advisory-tracking+] new in Fx14 → [asan][advisory-tracking+] new in Fx14 [qa?]
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.
Keywords: verifyme
QA Contact: ioana.budnar → mwobensmith
Whiteboard: [asan][advisory-tracking+] new in Fx14 [qa?] → [asan][advisory-tracking+] new in Fx14
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

5 years ago
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.
Blocks: 821360
Filed new bug 821360 for the current crash.
Please see comment 3 about the crash in CSS code. It's unrelated to this problem.
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.