The default bug view has changed. See this FAQ.

XHR use-after-free of JS

VERIFIED FIXED in Firefox 15

Status

()

Core
XPConnect
VERIFIED FIXED
5 years ago
4 months ago

People

(Reporter: johns, Assigned: mccr8)

Tracking

({crash, csectype-uaf, sec-critical})

Trunk
mozilla17
crash, csectype-uaf, sec-critical
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox14 wontfix, firefox15+ verified, firefox16+ verified, firefox17+ verified, firefox-esr1015+ verified)

Details

(Whiteboard: [advisory-tracking+])

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 646222 [details] [diff] [review]
Test change that triggers the crash

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

5 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

5 years ago
Bill said allocated() either means it has been freed already or hasn't been initialized.
(Assignee)

Comment 4

5 years ago
I can reproduce on OSX too.
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 5

5 years ago
Created attachment 647037 [details] [diff] [review]
slightly reduced

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?
(Assignee)

Updated

5 years ago
Summary: CC Crash in XHR Traverse -> NoteJSChild -> xpc_GCThingIsGrayCCThing -> Assertion failure: allocated() → XHR use-after-free of JS
(Assignee)

Comment 7

5 years ago
Created attachment 647667 [details] [diff] [review]
root moar, bz's idea

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

5 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
I don't think the code flow here changed in bug 732377.
(Reporter)

Comment 10

5 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

5 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

5 years ago
status-firefox-esr10: unaffected → ?
(Assignee)

Comment 12

5 years ago
Created attachment 648394 [details] [diff] [review]
part 1: root in CreateResponseParsedJSON

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

5 years ago
Created attachment 648396 [details] [diff] [review]
test case

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

5 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.
status-firefox-esr10: ? → affected
(Assignee)

Comment 15

5 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

5 years ago
It looks like this goes back to the initial landing of the JSON response type in Firefox 9. bug 655727
tracking-firefox-esr10: --- → 15+
tracking-firefox15: --- → +
tracking-firefox16: --- → +
tracking-firefox17: --- → +
(Assignee)

Comment 17

5 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

5 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

5 years ago
Attachment #648394 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 21

5 years ago
Comment on attachment 648394 [details] [diff] [review]
part 1: root in CreateResponseParsedJSON

Thanks!
Attachment #648394 - Flags: review?(peterv)
(Assignee)

Comment 22

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/7eb691b3ef12
Can we please give the rooting function a better name, since the name just started being a lie?
(Assignee)

Comment 24

5 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".
RootJSResultObjects?
(Assignee)

Comment 26

5 years ago
Created attachment 648829 [details] [diff] [review]
part 2: fix the name of the rooting function

SGTM
Attachment #648829 - Flags: review?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/7eb691b3ef12
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox17: affected → fixed
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+
(Assignee)

Updated

5 years ago
Attachment #648394 - Attachment description: root in CreateResponseParsedJSON → part 1: root in CreateResponseParsedJSON
(Assignee)

Comment 29

5 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

5 years ago
Created attachment 649103 [details] [diff] [review]
part 1+2 folded for backporting
(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?
(Assignee)

Comment 32

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

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/d68436b2cf7d
https://hg.mozilla.org/releases/mozilla-beta/rev/bc9dfaebe970
https://hg.mozilla.org/releases/mozilla-esr10/rev/736543cdf409
status-firefox-esr10: affected → fixed
status-firefox15: affected → fixed
status-firefox16: affected → fixed
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?]
(Assignee)

Comment 35

5 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...
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

5 years ago
Yes. It is probably better to test in a debug build.
Whiteboard: [advisory-tracking+][qa?] → [advisory-tracking+][qa+]
Keywords: verifyme
(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

5 years ago
Created attachment 656107 [details]
non-mochitest test, file 1
(Assignee)

Comment 40

5 years ago
Created attachment 656109 [details]
non-mochitest test, file 2

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
status-firefox-esr10: fixed → verified
status-firefox15: fixed → verified
status-firefox16: fixed → verified
status-firefox17: fixed → verified
QA Contact: anthony.s.hughes
Whiteboard: [advisory-tracking+][qa?] → [advisory-tracking+]
Group: core-security
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.