Closed Bug 777806 Opened 8 years ago Closed 8 years ago

XHR use-after-free of JS

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox14 --- wontfix
firefox15 + verified
firefox16 + verified
firefox17 + verified
firefox-esr10 15+ verified

People

(Reporter: johns, Assigned: mccr8)

References

Details

(Keywords: crash, csectype-uaf, sec-critical, Whiteboard: [advisory-tracking+])

Attachments

(6 files, 3 obsolete files)

When we landed the attached tweak to a test in 771666, this random orange started occuring - it doesn't seem to happen in ASAN builds, but seems to be 100% reproducible if you run the test *twice* in one session with EXTRA_TEST_ARGS="--repeat=1"

Assertion failure: allocated(), at ../../../dist/include/gc/Heap.h:497

Program received signal SIGSEGV, Segmentation fault.
js::gc::ArenaHeader::getAllocKind (this=0x7fffc645c000) at ../../../dist/include/gc/Heap.h:497
497             JS_ASSERT(allocated());
(gdb) bt
#0  js::gc::ArenaHeader::getAllocKind (this=0x7fffc645c000) at ../../../dist/include/gc/Heap.h:497
#1  0x00007ffff4cbc40d in js::gc::Cell::getAllocKind (this=0x7fffc645c020) at /home/johns/moz/moz-git/js/src/gc/Heap.h:935
#2  0x00007ffff4d415ad in js::gc::GetGCThingTraceKind (thing=0x7fffc645c020) at /home/johns/moz/moz-git/js/src/jsgcinlines.h:30
#3  0x00007ffff4d538d5 in js_GetGCThingTraceKind (thing=0x7fffc645c020) at /home/johns/moz/moz-git/js/src/jsgc.cpp:1760
#4  0x00007ffff36905b5 in xpc_GCThingIsGrayCCThing (thing=0x7fffc645c020)
    at /home/johns/moz/moz-git/js/xpconnect/src/nsXPConnect.cpp:574
#5  0x00007ffff41f551f in ChildFinder::NoteJSChild (this=0x7fffffffaeb0, child=0x7fffc645c020)
    at /home/johns/moz/moz-git/xpcom/base/nsCycleCollector.cpp:1991
#6  0x00007ffff414b8b8 in nsScriptObjectTracer::NoteJSChild (aScriptThing=0x7fffc645c020, name=0x7ffff52f0967 "mResultJSON", 
    aClosure=0x7fffffffaeb0) at /home/johns/moz/ff-dbg/xpcom/build/nsCycleCollectionParticipant.cpp:16
#7  0x00007ffff29ef110 in nsXMLHttpRequest::cycleCollection::TraceImpl(void*, void (*)(void*, char const*, void*), void*) ()
   from /home/johns/moz/ff-dbg/dist/bin/libxul.so
#8  0x00007ffff2b8c91f in nsDOMEventTargetHelper::cycleCollection::TraverseImpl(nsDOMEventTargetHelper::cycleCollection*, void*, nsCycleCollectionTraversalCallback&) () from /home/johns/moz/ff-dbg/dist/bin/libxul.so
#9  0x00007ffff29eb853 in nsXHREventTarget::cycleCollection::TraverseImpl(nsXHREventTarget::cycleCollection*, void*, nsCycleCollectionTraversalCallback&) () from /home/johns/moz/ff-dbg/dist/bin/libxul.so
#10 0x00007ffff29ee4d3 in nsXMLHttpRequest::cycleCollection::TraverseImpl(nsXMLHttpRequest::cycleCollection*, void*, nsCycleCollectionTraversalCallback&) () from /home/johns/moz/ff-dbg/dist/bin/libxul.so
#11 0x00007ffff28a4afe in nsCycleCollectionParticipant::Traverse (this=0x7ffff626e7f8, p=0x7fffc133dc00, cb=...)
    at ../../../dist/include/nsCycleCollectionParticipant.h:262
#12 0x00007ffff41f56cb in MayHaveChild (o=0x7fffc133dc00, cp=0x7ffff626e7f8)
    at /home/johns/moz/moz-git/xpcom/base/nsCycleCollector.cpp:2022
#13 0x00007ffff41f5625 in nsPurpleBuffer::RemoveSkippable (this=0x7fffe91b0090, removeChildlessNodes=true)
    at /home/johns/moz/moz-git/xpcom/base/nsCycleCollector.cpp:2042
#14 0x00007ffff41f57d0 in nsCycleCollector::ForgetSkippable (this=0x7fffe91b0000, removeChildlessNodes=true)
    at /home/johns/moz/moz-git/xpcom/base/nsCycleCollector.cpp:2066
#15 0x00007ffff41f818e in nsCycleCollector_forgetSkippable (aRemoveChildlessNodes=true)
    at /home/johns/moz/moz-git/xpcom/base/nsCycleCollector.cpp:3217
#16 0x00007ffff2ee544a in FireForgetSkippable (aSuspected=496, aRemoveChildless=true)
    at /home/johns/moz/moz-git/dom/base/nsJSEnvironment.cpp:3033
#17 0x00007ffff2ee5b5b in CCTimerFired (aTimer=0x7fffc6296dc0, aClosure=0x0)
    at /home/johns/moz/moz-git/dom/base/nsJSEnvironment.cpp:3268
Sounds like XHR isn't rooting some JS thing it holds onto properly, similar to bug 762280.
Keywords: sec-critical
Do we end up tracing an uninitialized jsval or something?
Bill said allocated() either means it has been freed already or hasn't been initialized.
I can reproduce on OSX too.
OS: Linux → All
Hardware: x86_64 → All
Attached patch slightly reduced (obsolete) — Splinter Review
This applies on top of the other patch.  I just removed a few of the tests.  It is a bit flakey, but you don't have to do the rerun thing any more.
> Sounds like XHR isn't rooting some JS thing it holds onto properly, similar to bug
> 762280.

That seems plausible, yes.  We call RootResultArrayBuffer() for cases when we set mResultArrayBuffer, but we have nothing along those lines for the case when we set mResultJSON.

Does calling RootResultArrayBuffer (yes, its name is silly at the moment) right before the CreateResponseParsedJSON help?
Summary: CC Crash in XHR Traverse -> NoteJSChild -> xpc_GCThingIsGrayCCThing -> Assertion failure: allocated() → XHR use-after-free of JS
Attached patch root moar, bz's idea (obsolete) — Splinter Review
I think this is what bz suggested (though maybe it would make more sense right inside CreateResponseParsedJSON).  I can't reproduce the crash with this patch.  John, if you want to try it out too that would be good.
If this is actually the fix, it looks like a regression from bug 732377.
I don't think the code flow here changed in bug 732377.
The patch appears to fix this for me - without the patch I crash within 2-3 iterations of the test with high certainty, without I just ran ten iterations of the test twice without issue.

I'm seeing if I can reproduce this at all on esr10 to see if a fix is needed there
Backporting this test isn't a simple matter, and I can't seem to make a smaller test case to easily reproduce this, so I'm not sure I can verify if this affects esr10 or not
I moved this into the actual function that is writing to mResultJSON as that seems to make more sense.
Assignee: nobody → continuation
Attachment #647667 - Attachment is obsolete: true
Attached patch test caseSplinter Review
From the patch, the test case was pretty easy to construct.  Which is a little worrying from the perspective of release.  But anywho, this test crashes without the patch and is okay with it.
Attachment #646222 - Attachment is obsolete: true
Attachment #647037 - Attachment is obsolete: true
On ESR-10, this test case produces this assertion failure:
Assertion failure: !hasLazyType(), at /Users/amccreight/mz/esr10/js/src/jsobj.h:911

Which doesn't show up on 17, but between that and looking at the code, I think everything is affected.
Comment on attachment 648394 [details] [diff] [review]
part 1: root in CreateResponseParsedJSON

I'm not sure what a good landing strategy for this is, given how obvious it seems to at least make crash.
Attachment #648394 - Flags: review?(peterv)
It looks like this goes back to the initial landing of the JSON response type in Firefox 9. bug 655727
Jesse, this test case could be an interesting fuzzing pattern.

1. access some property
2. don't touch that object
3. access the property again

I'm not sure how common this pattern is, where the first access creates some value and saves away a reference, and further references just look at that existing value. As we see in this particular instance, if the object doesn't root itself properly, there can be badness.
I managed to catch bug 778179 in gdb and it looks like a dupe of this. Here's the stack.

#0  0x00007ffff1af1912 in js::gc::ArenaHeader::getAllocKind (this=0x7fffb2d0c000)
    at ../../../dist/include/gc/Heap.h:506
#1  0x00007ffff3d7b9aa in js::gc::Cell::getAllocKind (this=0x7fffb2d0c040)
    at /home/billm/mozilla/in0/js/src/gc/Heap.h:982
#2  0x00007ffff3e0c1d1 in js::gc::GetGCThingTraceKind (thing=0x7fffb2d0c040)
    at /home/billm/mozilla/in0/js/src/jsgcinlines.h:30
#3  0x00007ffff3e1fe24 in js_GetGCThingTraceKind (thing=0x7fffb2d0c040)
    at /home/billm/mozilla/in0/js/src/jsgc.cpp:1832
#4  0x00007ffff2a8627c in xpc_GCThingIsGrayCCThing (thing=0x7fffb2d0c040)
    at /home/billm/mozilla/in0/js/xpconnect/src/nsXPConnect.cpp:574
#5  0x00007ffff33c6e76 in GCGraphBuilder::NoteJSChild (this=0x7fffe43d4ba0, child=0x7fffb2d0c040)
    at /home/billm/mozilla/in0/xpcom/base/nsCycleCollector.cpp:1890
#6  0x00007ffff333fac5 in nsScriptObjectTracer::NoteJSChild (aScriptThing=0x7fffb2d0c040, 
    name=0x7ffff43d3e3e "mResultJSON", aClosure=0x7fffe43d4ba0)
    at /home/billm/mozilla/in0/objdir-ff-dbg/xpcom/build/nsCycleCollectionParticipant.cpp:16
#7  0x00007ffff2121c35 in nsXMLHttpRequest::cycleCollection::TraceImpl (p=0x1b7f830, 
    aCallback=0x7ffff333fa54 <nsScriptObjectTracer::NoteJSChild(void*, char const*, void*)>, 
    aClosure=0x7fffe43d4ba0) at /home/billm/mozilla/in0/content/base/src/nsXMLHttpRequest.cpp:680
#8  0x00007ffff22607ae in nsDOMEventTargetHelper::cycleCollection::TraverseImpl (
    that=0x7ffff55b38a0, p=0x1b7f830, cb=...)
    at /home/billm/mozilla/in0/content/events/src/nsDOMEventTargetHelper.cpp:59
#9  0x00007ffff211f43c in nsXHREventTarget::cycleCollection::TraverseImpl (that=0x7ffff55b38a0, 
    p=0x1b7f830, cb=...) at /home/billm/mozilla/in0/content/base/src/nsXMLHttpRequest.cpp:287

We should get this landed soon, because that bug has really exploded on inbound.
(In reply to Bill McCloskey (:billm) from comment #18)
> I managed to catch bug 778179 in gdb and it looks like a dupe of this.
> Here's the stack.

Thank for looking into that!

> We should get this landed soon, because that bug has really exploded on
> inbound.

The proximate cause of that was apparently johns landing bug 771666, which was just a test change that initially found this bug.  My suggested hack around to avoid this crash was apparently ineffective. :)
Attachment #648394 - Flags: review?(bent.mozilla)
Blocks: 778179
Comment on attachment 648394 [details] [diff] [review]
part 1: root in CreateResponseParsedJSON

Review of attachment 648394 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok, mainly because nsContentUtils::PreserveWrapper guards against double-preserving.
Attachment #648394 - Flags: review?(bent.mozilla) → review+
Comment on attachment 648394 [details] [diff] [review]
part 1: root in CreateResponseParsedJSON

Thanks!
Attachment #648394 - Flags: review?(peterv)
Can we please give the rooting function a better name, since the name just started being a lie?
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #23)
> Can we please give the rooting function a better name, since the name just
> started being a lie?

Good point. I was thinking about that but didn't manage to put that into action. How about "RootResultBuffers"?  Or maybe "RootJSResultBuffers".
RootJSResultObjects?
SGTM
Attachment #648829 - Flags: review?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/7eb691b3ef12
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment on attachment 648829 [details] [diff] [review]
part 2: fix the name of the rooting function

r=me
Attachment #648829 - Flags: review?(bzbarsky) → review+
Attachment #648394 - Attachment description: root in CreateResponseParsedJSON → part 1: root in CreateResponseParsedJSON
Comment on attachment 648829 [details] [diff] [review]
part 2: fix the name of the rooting function

Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2d271ce54ff
Attachment #648829 - Attachment description: fix the name of the rooting function → part 2: fix the name of the rooting function
(In reply to Andrew McCreight [:mccr8] from comment #29)
> Comment on attachment 648829 [details] [diff] [review]
> part 2: fix the name of the rooting function
> 
> Thanks.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a2d271ce54ff

https://hg.mozilla.org/mozilla-central/rev/a2d271ce54ff
Flags: in-testsuite-
Flags: in-testsuite- → in-testsuite?
Comment on attachment 649103 [details] [diff] [review]
part 1+2 folded for backporting

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 655727
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: possible security problems, crash is easy to reproduce
Testing completed (on m-c, etc.): it has been on m-c for a few days
Fix Landed on Version: 17
Risk to taking this patch (and alternatives if risky): Low. This patch just adds a call to PreserveWrapper (via RootJSResultObjects), which can be safely called whenever. It mainly just can cause things to live slightly longer. The rest of the patch is just renaming a function.
String or UUID changes made by this patch: none
Attachment #649103 - Flags: approval-mozilla-esr10?
Attachment #649103 - Flags: approval-mozilla-beta?
Attachment #649103 - Flags: approval-mozilla-aurora?
Attachment #649103 - Flags: approval-mozilla-esr10?
Attachment #649103 - Flags: approval-mozilla-esr10+
Attachment #649103 - Flags: approval-mozilla-beta?
Attachment #649103 - Flags: approval-mozilla-beta+
Attachment #649103 - Flags: approval-mozilla-aurora?
Attachment #649103 - Flags: approval-mozilla-aurora+
Whiteboard: [advisory-tracking+]
What needs to be done to satisfy in-testsuite? Does it just need Andrew's testcase patch checked in?
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa?]
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #34)
> What needs to be done to satisfy in-testsuite? Does it just need Andrew's
> testcase patch checked in?

Yup.  I'm waiting until 15 is released or maybe until this bug is made public.  The test shows the problem pretty explicitly...
Okay, so this needs manual verification before Firefox 15 is released? I would assume verification is applying the patch locally and running with mochitest?
Yes. It is probably better to test in a debug build.
Whiteboard: [advisory-tracking+][qa?] → [advisory-tracking+][qa+]
(In reply to Andrew McCreight [:mccr8] from comment #37)
> Yes. It is probably better to test in a debug build.

Can I get some guidance on how to test this? I know that the test is contained in the patch but I'm not sure which changesets to apply the patch to and how to run the test. Thanks in advance.
Keywords: verifyme
Whiteboard: [advisory-tracking+][qa+] → [advisory-tracking+][qa?]
Here's a new version of the test that should be easier to run.  Download both files and put them in the same directory.  Install Jesse's fuzzpriv addon from here: https://www.squarefree.com/extensions/domFuzzLite3.xpi   Load the html file.  It should crash after a few seconds.  I think a debug build is better.
Thanks Andrews, that does reproduce with the 2012-07-26 debug Nightly.

Verified fixed with:
 * 2012-08-25 Firefox 15.0 Release Debug
 * 2012-08-28 Firefox 16.0 Beta Debug
 * 2012-08-28 Firefox 17.0 Aurora Debug
 * 2012-08-25 Firefox 10.0.7esrpre Debug
Status: RESOLVED → VERIFIED
QA Contact: anthony.s.hughes
Whiteboard: [advisory-tracking+][qa?] → [advisory-tracking+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.