Closed
Bug 777806
Opened 12 years ago
Closed 12 years ago
XHR use-after-free of JS
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
People
(Reporter: johns, Assigned: mccr8)
References
Details
(Keywords: crash, csectype-uaf, sec-critical, Whiteboard: [advisory-tracking+])
Attachments
(6 files, 3 obsolete files)
(deleted),
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
lsblakk
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
text/html
|
Details |
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
Assignee | ||
Comment 1•12 years ago
|
||
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?
Assignee | ||
Comment 3•12 years ago
|
||
Bill said allocated() either means it has been freed already or hasn't been initialized.
Assignee | ||
Comment 5•12 years ago
|
||
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.
Comment 6•12 years ago
|
||
> 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?
Assignee | ||
Updated•12 years ago
|
Summary: CC Crash in XHR Traverse -> NoteJSChild -> xpc_GCThingIsGrayCCThing -> Assertion failure: allocated() → XHR use-after-free of JS
Assignee | ||
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 8•12 years ago
|
||
If this is actually the fix, it looks like a regression from bug 732377.
status-firefox-esr10:
--- → unaffected
status-firefox14:
--- → wontfix
status-firefox15:
--- → affected
status-firefox16:
--- → affected
status-firefox17:
--- → affected
Comment 9•12 years ago
|
||
I don't think the code flow here changed in bug 732377.
Reporter | ||
Comment 10•12 years ago
|
||
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
Reporter | ||
Comment 11•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 12•12 years ago
|
||
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
Assignee | ||
Comment 13•12 years ago
|
||
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
Assignee | ||
Comment 14•12 years ago
|
||
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.
Assignee | ||
Comment 15•12 years ago
|
||
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)
Assignee | ||
Comment 16•12 years ago
|
||
It looks like this goes back to the initial landing of the JSON response type in Firefox 9. bug 655727
Updated•12 years ago
|
tracking-firefox-esr10:
--- → 15+
tracking-firefox15:
--- → +
tracking-firefox16:
--- → +
tracking-firefox17:
--- → +
Assignee | ||
Comment 17•12 years ago
|
||
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.
Assignee | ||
Comment 19•12 years ago
|
||
(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. :)
Assignee | ||
Updated•12 years ago
|
Attachment #648394 -
Flags: review?(bent.mozilla)
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+
Assignee | ||
Comment 21•12 years ago
|
||
Comment on attachment 648394 [details] [diff] [review]
part 1: root in CreateResponseParsedJSON
Thanks!
Attachment #648394 -
Flags: review?(peterv)
Assignee | ||
Comment 22•12 years ago
|
||
Comment 23•12 years ago
|
||
Can we please give the rooting function a better name, since the name just started being a lie?
Assignee | ||
Comment 24•12 years ago
|
||
(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".
Comment 25•12 years ago
|
||
RootJSResultObjects?
Comment 27•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 28•12 years ago
|
||
Comment on attachment 648829 [details] [diff] [review]
part 2: fix the name of the rooting function
r=me
Attachment #648829 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #648394 -
Attachment description: root in CreateResponseParsedJSON → part 1: root in CreateResponseParsedJSON
Assignee | ||
Comment 29•12 years ago
|
||
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
Assignee | ||
Comment 30•12 years ago
|
||
Comment 31•12 years ago
|
||
(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-
Updated•12 years ago
|
Flags: in-testsuite- → in-testsuite?
Assignee | ||
Comment 32•12 years ago
|
||
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?
Updated•12 years ago
|
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+
Assignee | ||
Comment 33•12 years ago
|
||
Updated•12 years ago
|
Whiteboard: [advisory-tracking+]
Comment 34•12 years ago
|
||
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?]
Assignee | ||
Comment 35•12 years ago
|
||
(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...
Comment 36•12 years ago
|
||
Okay, so this needs manual verification before Firefox 15 is released? I would assume verification is applying the patch locally and running with mochitest?
Assignee | ||
Comment 37•12 years ago
|
||
Yes. It is probably better to test in a debug build.
Comment 38•12 years ago
|
||
(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?]
Assignee | ||
Comment 39•12 years ago
|
||
Assignee | ||
Comment 40•12 years ago
|
||
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.
Comment 41•12 years ago
|
||
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+]
Updated•12 years ago
|
Group: core-security
Updated•8 years ago
|
Keywords: csectype-uaf
You need to log in
before you can comment on or make changes to this bug.
Description
•