Closed Bug 1317258 Opened 5 years ago Closed 5 years ago

VRFrameData does not root its JS objects


(Core :: Graphics, defect)

Not set



Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + fixed
firefox53 + fixed


(Reporter: nils, Assigned: mccr8)



(Keywords: csectype-uaf, regression, sec-critical)


(1 file)

The following testcase crashes the latest ASAN build of Firefox (BuildID=20161113140512) as follow after a few reloads:


var x=new VRFrameData();
for(var a in x) typeof x[a];

ASAN output:

==4777==ERROR: AddressSanitizer: SEGV on unknown address 0x008ea2417ffd (pc 0x7f0b50d63cfe bp 0x7ffde37bf9d0 sp 0x7ffde37bf800 T0)
    #0 0x7f0b50d63cfd in IsInsideNursery /home/worker/workspace/build/src/obj-firefox/dist/include/js/HeapAPI.h:333:21
    #1 0x7f0b50d63cfd in GCThingIsMarkedGray /home/worker/workspace/build/src/obj-firefox/dist/include/js/HeapAPI.h:362
    #2 0x7f0b50d63cfd in TraversalTracer::onChild(JS::GCCellPtr const&) /home/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:341
    #3 0x7f0b50d8e091 in JS::CallbackTracer::onShapeEdge(js::Shape**) /home/worker/workspace/build/src/obj-firefox/dist/include/js/TracingAPI.h:147:9
    #4 0x7f0b5c03b780 in dispatchToOnEdge /home/worker/workspace/build/src/obj-firefox/dist/include/js/TracingAPI.h:228:49
    #5 0x7f0b5c03b780 in js::Shape* DoCallback<js::Shape*>(JS::CallbackTracer*, js::Shape**, char const*) /home/worker/workspace/build/src/js/src/gc/Tracer.cpp:51
    #6 0x7f0b5bff1804 in DispatchToTracer<js::Shape *> /home/worker/workspace/build/src/js/src/gc/Marking.cpp:665:5
    #7 0x7f0b5bff1804 in TraceEdge<js::Shape *> /home/worker/workspace/build/src/js/src/gc/Marking.cpp:411
    #8 0x7f0b5bff1804 in js::Shape::traceChildren(JSTracer*) /home/worker/workspace/build/src/js/src/gc/Marking.cpp:1041
    #9 0x7f0b5c03cac9 in TraceChildren /home/worker/workspace/build/src/js/src/gc/Tracer.cpp:126:5
    #10 0x7f0b5c03cac9 in JS::TraceChildren(JSTracer*, JS::GCCellPtr) /home/worker/workspace/build/src/js/src/gc/Tracer.cpp:111
    #11 0x7f0b50d637f3 in NoteGCThingJSChildren /home/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:637:3
    #12 0x7f0b50d637f3 in mozilla::CycleCollectedJSContext::TraverseGCThing(mozilla::CycleCollectedJSContext::TraverseSelect, JS::GCCellPtr, nsCycleCollectionTraversalCallback&) /home/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:695
    #13 0x7f0b50d6353a in mozilla::JSGCThingParticipant::Traverse(void*, nsCycleCollectionTraversalCallback&) /home/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:305:3
    #14 0x7f0b50d7d355 in CCGraphBuilder::BuildGraph(js::SliceBudget&) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2282:21
    #15 0x7f0b50d838ce in nsCycleCollector::MarkRoots(js::SliceBudget&) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2879:23
    #16 0x7f0b50d891ef in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3655:9
    #17 0x7f0b50d8c7dc in nsCycleCollector_collectSlice(js::SliceBudget&, bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:4160:3
    #18 0x7f0b53c51698 in nsJSContext::RunCycleCollectorSlice() /home/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:1476:3
    #19 0x7f0b50ecc9d1 in nsTimerImpl::Fire() /home/worker/workspace/build/src/xpcom/threads/nsTimerImpl.cpp:477:7
    #20 0x7f0b50ea08f2 in nsTimerEvent::Run() /home/worker/workspace/build/src/xpcom/threads/TimerThread.cpp:289:3
    #21 0x7f0b50eaf31b in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1216:7
    #22 0x7f0b50f3235c in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/glue/nsThreadUtils.cpp:361:10
    #23 0x7f0b51cca75f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #24 0x7f0b51c3a828 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/
    #25 0x7f0b51c3a828 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/
    #26 0x7f0b51c3a828 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/
    #27 0x7f0b57309c7f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #28 0x7f0b594d5ea7 in XRE_RunAppShell /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:869:12
    #29 0x7f0b51c3a828 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/
    #30 0x7f0b51c3a828 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/
    #31 0x7f0b51c3a828 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/
    #32 0x7f0b594d53e3 in XRE_InitChildProcess /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:701:7
    #33 0x4dfb5b in content_process_main /home/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:197:19
    #34 0x4dfb5b in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:392
    #35 0x7f0b6c20282f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291
    #36 0x41ba38 in _start (/home/nils/fuzzer3/firefox/firefox+0x41ba38)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/worker/workspace/build/src/obj-firefox/dist/include/js/HeapAPI.h:333:21 in IsInsideNursery
Given the test case, I'd assume this is some kind of rooting or initialization issue with VRFrameData.
Component: JavaScript: GC → Graphics
The VRFrameData ctor doesn't call mozilla::HoldJSObjects.
[Tracking Requested - why for this release]: sec-critical
We appear to have no tests that use VRFrameData. I'd hope we'd notice this error with our assertions, but I should confirm that.
Summary: SEGV in IsInsideNursery → VRFrameData does not root its JS objects
[Tracking Requested - why for this release]:
Assignee: nobody → continuation
This test also reproduces a crash:

var frameData = new VRFrameData();
for(var a in frameData) typeof frameData[a];
for(var a in frameData) typeof frameData[a];

I'll post a patch, but it doesn't seem good that we have code in the tree, that runs in Nightly, that has no tests at all. Having tests gives us a chance to detect this in automation, and helps mutation-based fuzzers find test cases.
(In reply to Andrew McCreight [:mccr8] from comment #6)
> This test also reproduces a crash:

Sorry, I meant that this works in a build, like Mochitests, that allows the use of SpecialPowers.
Comment on attachment 8810646 [details] [diff] [review]
Call HoldJSObjects in VRFrameData's ctor.

crap. /me kicks himself.
Attachment #8810646 - Flags: review?(bugs) → review+
Comment on attachment 8810646 [details] [diff] [review]
Call HoldJSObjects in VRFrameData's ctor.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? probably pretty easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no

Which older supported branches are affected by this flaw? 52, though this feature is only enabled on Aurora and Nightly, so it won't be affected when it becomes beta.

If not all supported branches, which bug introduced the flaw? bug 1306415

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? trivial

How likely is this patch to cause regressions; how much testing does it need? very low risk
Attachment #8810646 - Flags: sec-approval?
Comment on attachment 8810646 [details] [diff] [review]
Call HoldJSObjects in VRFrameData's ctor.

sec-approval+. Please backport it too so we don't accidentally expose people.
Attachment #8810646 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
Comment on attachment 8810646 [details] [diff] [review]
Call HoldJSObjects in VRFrameData's ctor.

Approval Request Comment
[Feature/regressing bug #]: bug 1306415 
[User impact if declined]: sec-critical
[Describe test coverage new/current, TreeHerder]: none. I'll land a test after this is on m-c and Aurora.
[Risks and why]: Very low. This is a simple change.
[String/UUID change made/needed]: none
Attachment #8810646 - Flags: approval-mozilla-aurora?
Attachment #8810646 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: core-security → gfx-core-security
Needinfo on myself to land a test.
Flags: needinfo?(continuation)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: gfx-core-security
I guess I'm not going to get around to landing a test case, but hopefully we'll actually test this feature at some point...
Flags: needinfo?(continuation)
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
You need to log in before you can comment on or make changes to this bug.