Closed Bug 1470260 (CVE-2018-12377) Opened 6 years ago Closed 6 years ago

heap-use-after-free in mozilla::RefreshDriverTimer::TickRefreshDrivers

Categories

(Core :: Layout, defect, P2)

60 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 62+ verified
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- verified
firefox63 --- verified

People

(Reporter: nils, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage][adv-main62+][adv-esr60.2+])

Attachments

(9 files, 3 obsolete files)

607 bytes, text/html
Details
163 bytes, text/xml
Details
16.02 KB, text/plain
Details
18.17 KB, text/plain
Details
18.22 KB, text/plain
Details
16.51 KB, text/plain
Details
6.99 KB, patch
MatsPalmgren_bugz
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
1.66 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
7.05 KB, patch
Details | Diff | Splinter Review
The following testcase crashes the latest ASAN build of Firefox ESR 60 (BuildID=20180621121604). It requires the fuzzPriv extension and requires to be loaded from a HTTP server. I also requires the attached mini.xml in the same directory.

crash.html:
<script>
var xhrp=XMLHttpRequest;
function spin () {
    var x=new xhrp();
    try{x.open("POST","https://mozilla.org",false);}catch(e){}
    try{x.send("X");}catch(e){}
}
function start() {
        o5=document.createElementNS('http://www.w3.org/1999/xhtml','link');
        window.top.setTimeout(fun0, 400);
}
function fun0() {
        o87=this;
        window.top.requestAnimationFrame(fun1);
        o5.href='mini.xml';
}
function fun1() {
        fuzzPriv.trustedKeyEvent(o5,'press',false,false,false,false,13,0);
        spin();
        spin();
        window.fuzzPriv.callDrawWindow(0);
        xhrp=o87.XMLHttpRequest;
        spin();
}
</script>
<body onload="start()"></body>


ASAN output:
=================================================================
==28373==ERROR: AddressSanitizer: heap-use-after-free on address 0x608000059d30 at pc 0x7fac2c8bff34 bp 0x7ffe862054f0 sp 0x7ffe862054e8
READ of size 1 at 0x608000059d30 thread T0 (Web Content)
    #0 0x7fac2c8bff33 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:313:26
    #1 0x7fac2c8bf646 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:333:5
    #2 0x7fac2c8c24fe in RunRefreshDrivers /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:774:5
    #3 0x7fac2c8c24fe in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:687
    #4 0x7fac2c8c2125 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:588:9
    #5 0x7fac2d19016f in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /builds/worker/workspace/build/src/layout/ipc/VsyncChild.cpp:68:16
    #6 0x7fac263e7829 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:156:20
    #7 0x7fac262bddde in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1968:28
    #8 0x7fac25ede53e in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2134:25
    #9 0x7fac25edb594 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2064:17
    #10 0x7fac25edccbc in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1910:5
    #11 0x7fac25edd318 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1943:15
    #12 0x7fac2503b1b6 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1040:14
    #13 0x7fac25054970 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:517:10
    #14 0x7fac25ee5ef6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:125:5
    #15 0x7fac25e38889 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #16 0x7fac25e38889 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #17 0x7fac25e38889 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #18 0x7fac2c16544a in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #19 0x7fac2fa310bb in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:895:22
    #20 0x7fac25e38889 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #21 0x7fac25e38889 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #22 0x7fac25e38889 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #23 0x7fac2fa30aa4 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:721:34
    #24 0x4f6f2c in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #25 0x4f6f2c in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280
    #26 0x7fac4387f82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #27 0x4265bc in _start (/fuzzer3/esr60/firefox/firefox+0x4265bc)

0x608000059d30 is located 16 bytes inside of 88-byte region [0x608000059d20,0x608000059d78)
freed by thread T0 (Web Content) here:
    #0 0x4c6fc2 in __interceptor_free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:68:3
    #1 0x7fac2c8bd05a in Shutdown /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1057:3
    #2 0x7fac2c8bd05a in nsRefreshDriver::Disconnect() /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2451
    #3 0x7fac2cac69e6 in nsPresContext::DetachShell() /builds/worker/workspace/build/src/layout/base/nsPresContext.cpp:1112:21
    #4 0x7fac2c91e05a in mozilla::PresShell::Destroy() /builds/worker/workspace/build/src/layout/base/PresShell.cpp:1402:19
    #5 0x7fac2ca2d712 in nsDocumentViewer::DestroyPresShell() /builds/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:4614:15
    #6 0x7fac2ca2e20c in nsDocumentViewer::SetDocumentInternal(nsIDocument*, bool) /builds/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:1940:5
    #7 0x7fac2bc9def2 in nsXMLContentSink::OnTransformDone(nsresult, nsIDocument*) /builds/worker/workspace/build/src/dom/xml/nsXMLContentSink.cpp:380:20
    #8 0x7fac2bc9e0af in non-virtual thunk to nsXMLContentSink::OnTransformDone(nsresult, nsIDocument*) /builds/worker/workspace/build/src/dom/xml/nsXMLContentSink.cpp
    #9 0x7fac2bd39eaf in txMozillaXSLTProcessor::notifyError() /builds/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:1192:16
    #10 0x7fac2bd2518c in txMozillaXSLTProcessor::reportError(nsresult, char16_t const*, char16_t const*) /builds/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:1119:9
    #11 0x7fac2bd5e230 in txStylesheetCompiler::cancel(nsresult, char16_t const*, char16_t const*) /builds/worker/workspace/build/src/dom/xslt/xslt/txStylesheetCompiler.cpp:400:20
    #12 0x7fac2bd204e7 in HandleStartElement /builds/worker/workspace/build/src/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp:132:20
    #13 0x7fac2bd204e7 in non-virtual thunk to txStylesheetSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int) /builds/worker/workspace/build/src/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
    #14 0x7fac26eafb55 in nsExpatDriver::HandleStartElement(char16_t const*, char16_t const**) /builds/worker/workspace/build/src/parser/htmlparser/nsExpatDriver.cpp:324:7
    #15 0x7fac2da544cb in doContent /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:2442:11
    #16 0x7fac2da48faa in contentProcessor /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:2098:27
    #17 0x7fac2da48faa in doProlog /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:4078
    #18 0x7fac2da3f263 in prologProcessor /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:3812:10
    #19 0x7fac2da3f263 in prologInitProcessor /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:3629
    #20 0x7fac2da3d666 in MOZ_XML_Parse /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:1530:17
    #21 0x7fac26eb5add in nsExpatDriver::ParseBuffer(char16_t const*, unsigned int, bool, unsigned int*) /builds/worker/workspace/build/src/parser/htmlparser/nsExpatDriver.cpp:887:16
    #22 0x7fac26eb6def in nsExpatDriver::ConsumeToken(nsScanner&, bool&) /builds/worker/workspace/build/src/parser/htmlparser/nsExpatDriver.cpp:985:5
    #23 0x7fac26ec3259 in nsParser::Tokenize(bool) /builds/worker/workspace/build/src/parser/htmlparser/nsParser.cpp:1539:30
    #24 0x7fac26ebf006 in nsParser::ResumeParse(bool, bool, bool) /builds/worker/workspace/build/src/parser/htmlparser/nsParser.cpp:1056:41
    #25 0x7fac26ec46a7 in nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /builds/worker/workspace/build/src/parser/htmlparser/nsParser.cpp:1437:12
    #26 0x7fac2bd21276 in txStylesheetSink::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /builds/worker/workspace/build/src/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp:248:23
    #27 0x7fac25b05172 in nsCORSListenerProxy::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /builds/worker/workspace/build/src/netwerk/protocol/http/nsCORSListenerProxy.cpp:668:20
    #28 0x7fac25a8bfa3 in DoOnDataAvailable /builds/worker/workspace/build/src/netwerk/protocol/http/HttpChannelChild.cpp:1040:28
    #29 0x7fac25a8bfa3 in mozilla::net::HttpChannelChild::OnTransportAndData(nsresult const&, nsresult const&, unsigned long const&, unsigned int const&, nsTString<char> const&) /builds/worker/workspace/build/src/netwerk/protocol/http/HttpChannelChild.cpp:966
    #30 0x7fac25c93356 in mozilla::net::ChannelEventQueue::FlushQueue() /builds/worker/workspace/build/src/netwerk/ipc/ChannelEventQueue.cpp:93:12
    #31 0x7fac25c9cdbe in MaybeFlushQueue /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/net/ChannelEventQueue.h:329:5
    #32 0x7fac25c9cdbe in CompleteResume /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/net/ChannelEventQueue.h:306
    #33 0x7fac25c9cdbe in mozilla::net::ChannelEventQueue::ResumeInternal()::CompleteResumeRunnable::Run() /builds/worker/workspace/build/src/netwerk/ipc/ChannelEventQueue.cpp:161
    #34 0x7fac2501509a in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:413:25
    #35 0x7fac2503b1b6 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1040:14
    #36 0x7fac25054970 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:517:10

previously allocated by thread T0 (Web Content) here:
    #0 0x4c7303 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x4f7dcd in moz_xmalloc /builds/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:70:17
    #2 0x7fac2c8aae96 in operator new /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:156:12
    #3 0x7fac2c8aae96 in PVsyncActorCreated /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2318
    #4 0x7fac2c8aae96 in CreateContentVsyncRefreshTimer /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1007
    #5 0x7fac2c8aae96 in CreateVsyncRefreshTimer /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1034
    #6 0x7fac2c8aae96 in nsRefreshDriver::ChooseTimer() const /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1142
    #7 0x7fac2c8adaaf in nsRefreshDriver::EnsureTimerStarted(nsRefreshDriver::EnsureTimerStartedFlags) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1378:34
    #8 0x7fac2c8adf6a in nsRefreshDriver::AddRefreshObserver(nsARefreshObserver*, mozilla::FlushType) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1264:3
    #9 0x7fac2b9ec954 in mozilla::dom::CoalescedMouseMoveFlusher::StartObserver() /builds/worker/workspace/build/src/dom/ipc/CoalescedMouseData.cpp:92:23
    #10 0x7fac2b9ec2b5 in mozilla::dom::TabChild::ProcessPendingCoalescedMouseDataAndDispatchEvents() /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:1563:32
    #11 0x7fac2ba5981d in mozilla::dom::TabChild::RecvRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:1697:5
    #12 0x7fac2655264e in mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBrowserChild.cpp:3470:20
    #13 0x7fac266975bb in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PContentChild.cpp:5103:28
    #14 0x7fac25ede53e in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2134:25
    #15 0x7fac25edb594 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2064:17
    #16 0x7fac25edccbc in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1910:5
    #17 0x7fac25edd318 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1943:15
    #18 0x7fac2501509a in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:413:25
    #19 0x7fac2503b1b6 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1040:14
    #20 0x7fac25054970 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:517:10
    #21 0x7fac25ee5f0a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #22 0x7fac25e38889 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #23 0x7fac25e38889 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #24 0x7fac25e38889 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #25 0x7fac2c16544a in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #26 0x7fac2fa310bb in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:895:22
    #27 0x7fac25e38889 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #28 0x7fac25e38889 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #29 0x7fac25e38889 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #30 0x7fac2fa30aa4 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:721:34
    #31 0x4f6f2c in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #32 0x4f6f2c in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280
    #33 0x7fac4387f82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:313:26 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&)
Shadow bytes around the buggy address:
  0x0c1080003350: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
  0x0c1080003360: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
  0x0c1080003370: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
  0x0c1080003380: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
  0x0c1080003390: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
=>0x0c10800033a0: fa fa fa fa fd fd[fd]fd fd fd fd fd fd fd fd fa
  0x0c10800033b0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c10800033c0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c10800033d0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c10800033e0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c10800033f0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==28373==ABORTING
Attached file mini.xml
Attached file ASAN output
Group: core-security → dom-core-security
I looked up the source code on DXR for ESR60: https://dxr.mozilla.org/mozilla-esr60/source/layout/base/nsRefreshDriver.cpp?q=RefreshDriverTimer&redirect_type=direct#313

The code in that method looks like this:
   for (nsRefreshDriver* driver : drivers) {
      ...
      TickDriver(driver, aJsNow, aNow);
      mLastFireSkipped = mLastFireSkipped || driver->mSkippedPaints;

The second line is line 313. Based on the allocation stack, it looks like the object being touched in a UAF is a VsyncRefreshDriverTimer, though I don't see how that could be getting touched in line 313.

Line 313 is not present in trunk. After some digging around in the history, it looks like it was removed as being unused in bug 1461946.

Kats, could you take a look at this, or do you know somebody who could? Thanks.

I'm going to move this to layout, because the stacks involved look related to the refresh driver, and that's a little more layout than DOM. Hard to precisely say, though.
Group: dom-core-security → layout-core-security
Component: DOM → Layout
Flags: needinfo?(bugmail)
This from a Linux ASAN trunk build from Jun 2 I had laying around.
This rev to be precise:
changeset:   420868:cbf9ea7c5531
tag:         tip
parent:      420859:69d43b0517a9
parent:      420867:92f6a3f162e7
user:        Csoregi Natalia <ncsoregi@mozilla.com>
date:        Sat Jun 02 01:01:33 2018 +0300
summary:     Merge inbound to mozilla-central.  a=merge

I get an ASAN error when accessing mLastChildTick here:
https://searchfox.org/mozilla-central/rev/9a3f8590f807d449e790c8ba0e39eb14f41066d8/layout/base/nsRefreshDriver.cpp#677
because 'this' was deleted (in RunRefreshDrivers 2 lines above).

This stack is when 'this' (in stack frame #65) is deleted.
The crash is quite hard for me to reproduce but I think this should fix it.

Nils, can you try this ASAN Try build and see if it fixes it for you?
https://queue.taskcluster.net/v1/task/ezotLQz_QYad_K0a1oN5LA/runs/0/artifacts/public/build/target.tar.bz2
Flags: needinfo?(nils)
Keywords: sec-critical
Priority: -- → P2
IIUC, you're still reproducing a crash from after bug 1461946 landed? So all branches are affected?
Flags: needinfo?(mats)
Yeah, I suspect all supported versions are affected.
The issue might have been introduced by bug 1332226, which added
the code that uses 'this' after the RunRefreshDrivers call for
this testcase.  It /might/ have been OK before then but I can't
say for sure.
Flags: needinfo?(mats)
Hey, I was only ever able to reproduce on an esr60 build. I believe your try build is 62 on which it does not reproduce, however I wouldn't expect it to.
Flags: needinfo?(nils)
Clearing ni, it sounds like Mats has a good handle on what's going on. Feel free to ni me again if you need me to look into this more.
Flags: needinfo?(bugmail)
(In reply to Nils from comment #8)
> Hey, I was only ever able to reproduce on an esr60 build. I believe your try
> build is 62 on which it does not reproduce, however I wouldn't expect it to.

Here's the same fix on the ESR60 branch:
https://queue.taskcluster.net/v1/task/ZA_cFdbxQxSJsHAEVMqq2g/runs/0/artifacts/public/build/target.tar.bz2
Flags: needinfo?(nils)
Still reproduces on the ESR60 build you linked. Same ASAN stacks.
Flags: needinfo?(nils)
So the second issue is that we destroy the InactiveRefreshDriverTimer too
and then access its mNextDriverIndex member.
OK, here's a new esr60 asan build with a tentative fix for both issues:
https://queue.taskcluster.net/v1/task/aefX_zTwTPSa7QG_XI_scw/runs/0/artifacts/public/build/target.tar.bz2
Nils, can you try that one please?
Flags: needinfo?(nils)
Attached file asan.txt
Still crashes. Slightly different ASAN stacks (attached)
Flags: needinfo?(nils)
See bug 1452744, I think they're dupes, but we couldn't repro that one IIRC.
See Also: → 1452744
(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> See bug 1452744, I think they're dupes, but we couldn't repro that one IIRC.

Thanks for the pointer, I hadn't seen that bug before but Olli's WIP
there looks pretty much identical my fix to the second issue.
Assignee: nobody → mats
Can confirm that it does not reproduce on that build anymore.
Flags: needinfo?(nils)
This is pretty close to Olli's patch in bug 1452744 so I figured it'd
be better to have a fresh set of eyes reviewing this.
Attachment #8988495 - Flags: review?(emilio)
The commit messages / code changes reveal the nature of this issue,
but I think it's pretty much impossible to deduce anything about
how trigger it from that.
Should we mark this as a duplicate of bug 1452744?
Comment on attachment 8988493 [details] [diff] [review]
part 1 - Ensure that 'this' stays alive for the duration of the TickRefreshDriver call.

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

So, this is obviously harmless, but VsyncChild is supposed to hold a strong reference to the observer, which presumably is cleared before shutdown. That's probably the reason this is only reproducible on shutdown...

Perhaps VsyncChild::RecvNotify is the right place to hold that strong ref?f

Otherwise it feels pretty weird that NormalPriorityNotify assumes someone holds a ref to the object, but NotifyVSync doesn't... Though I guess that runs off a runnable all the time so it's fine.

WDYT mats?

In any case, this looks fine even if we decide it's a short term fix.
Attachment #8988493 - Flags: review?(emilio) → review+
Comment on attachment 8988495 [details] [diff] [review]
part 2 - Make RefreshDriverTimer ref-counted and hold a strong ref on it on the stack when nsRefreshDriver::Tick can be reached.

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

::: layout/base/nsRefreshDriver.cpp
@@ +342,4 @@
>    static void TimerTick(nsITimer* aTimer, void* aClosure)
>    {
> +    RefPtr<RefreshDriverTimer> timer =
> +      static_cast<RefreshDriverTimer*>(aClosure);

Hmm, what guarantees that the timer isn't already deleted by the time we get to the timer function? There's other kind of flushes that can end up in nsRefreshDriver::Shutdown, right?

Should the caller of InitWithNamedFuncCallback AddRef the timer, and change this to:

RefPtr<RefreshDriverTimer> timer =
  already_AddRefed(static_cast<RefreshDriverTimer*>(aClosure));

?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #23)
> Perhaps VsyncChild::RecvNotify is the right place to hold that strong ref?

I considered adding it there instead but I figured the other paths in
NotifyVsync already has strong refs through the Runnables and I didn't
want to add additional addrefs for perf reasons.  (IIRC, this code is
somewhat perf sensitive.)
AFAICT, the added RefPtr is the only path that doesn't have it.

Also, the other paths runs async anyway, so adding a strong ref on
the stack in VsyncChild::RecvNotify doesn't really help except for
the path I'm fixing.

> Otherwise it feels pretty weird that NormalPriorityNotify assumes someone
> holds a ref to the object, but NotifyVSync doesn't... Though I guess that
> runs off a runnable all the time so it's fine.

Right, it's somewhat implicit I guess.  I can add a comment in
NotifyVsync saying that all paths there must have a strong ref
on 'this' while running the callbacks.  Would that be enough?
(In reply to Mats Palmgren (:mats) from comment #25)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #23)
> > Perhaps VsyncChild::RecvNotify is the right place to hold that strong ref?
> 
> I considered adding it there instead but I figured the other paths in
> NotifyVsync already has strong refs through the Runnables and I didn't
> want to add additional addrefs for perf reasons.  (IIRC, this code is
> somewhat perf sensitive.)
> AFAICT, the added RefPtr is the only path that doesn't have it.
> 
> Also, the other paths runs async anyway, so adding a strong ref on
> the stack in VsyncChild::RecvNotify doesn't really help except for
> the path I'm fixing.

Yeah, fair enough.

> > Otherwise it feels pretty weird that NormalPriorityNotify assumes someone
> > holds a ref to the object, but NotifyVSync doesn't... Though I guess that
> > runs off a runnable all the time so it's fine.
> 
> Right, it's somewhat implicit I guess.  I can add a comment in
> NotifyVsync saying that all paths there must have a strong ref
> on 'this' while running the callbacks.  Would that be enough?

Yeah, that wfm, thanks :)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #24)
> Hmm, what guarantees that the timer isn't already deleted by the time we get
> to the timer function?

I thought about that, but my assumption was that the timer is cancelled
if that happens so there will never be a callback in that case.
We have:
  virtual ~SimpleTimerBasedRefreshDriverTimer() override
  {
    StopTimer();
  }

so that should take case of any all subclasses of that,
but interestingly:
class VsyncRefreshDriverTimer : public RefreshDriverTimer
which doesn't seem to have something like that...  hmm, looking...
VsyncRefreshDriverTimer doesn't do any InitWithNamedFuncCallback
though and doesn't have any callbacks like that, so I think we're fine.
Comment on attachment 8988495 [details] [diff] [review]
part 2 - Make RefreshDriverTimer ref-counted and hold a strong ref on it on the stack when nsRefreshDriver::Tick can be reached.

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

Yeah, sounds good, thanks for checking and bearing with me :)

::: layout/base/nsRefreshDriver.cpp
@@ +2292,2 @@
>    MOZ_ASSERT(!XRE_IsParentProcess());
>    auto* vsyncRefreshDriverTimer =

nit: Maybe change this to RefPtr<VsyncRefreshDriverTimer>, and use forget() below when assigning to sRegularRateTimer?

I think it's the only timer that isn't assigned directly to the StaticRefPtrs, and using objects with a zero refcount is kind of a footgun.
Attachment #8988495 - Flags: review?(emilio) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #30)
> Yeah, sounds good, thanks for checking and bearing with me :)

No problem, I appreciate questions like that.
In general I think people should ask more critical questions
like that during review.  It helps making sure all angles are
covered and also that everyone understands the issue in
the same way.  Another good side effect is that it leaves
a trail in the bug of how people were thinking for future
readers (something that I often miss in decade old bugs).

So please, never hesitate to ask any question, they are
always very welcome.

> nit: Maybe change this to RefPtr<VsyncRefreshDriverTimer>, and use forget()
> below when assigning to sRegularRateTimer?

Yup, good point.
Attachment #8988495 - Attachment is obsolete: true
Attachment #8988581 - Flags: review+
Comment on attachment 8988581 [details] [diff] [review]
part 2 - Make RefreshDriverTimer ref-counted and hold a strong ref on it on the stack when nsRefreshDriver::Tick can be reached.

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

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

The commit messages / code changes reveal the nature of this issue,
but I think it's pretty much impossible to deduce anything about
how trigger it from that.

> Which older supported branches are affected by this flaw?

All.

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

The same changes works for esr60.  I haven't tried esr52.

> How likely is this patch to cause regressions; how much testing does it need?

The patches just adds additional ref-counting to keep objects alive
for longer.  There are no functional changes besides that.
It's very unlikely to cause regressions.

No specific testing is needed.
Attachment #8988581 - Flags: sec-approval?
Added a code comment:
+      // IMPORTANT: All paths through this method MUST hold a strong ref on
+      // |this| for the duration of the TickRefreshDriver callback.
Attachment #8988493 - Attachment is obsolete: true
Attachment #8988799 - Flags: review+
Fwiw, from a stylistic point of view I would actually prefer this:
-  sRegularRateTimer = vsyncRefreshDriverTimer.forget();
+  sRegularRateTimer = std::move(vsyncRefreshDriverTimer);
but it seems StaticRefPtr doesn't have that assignment operator.

The reason I prefer std::move over .forget() is that the former is
something anyone can understand the semantics of whereas forget()
is a rather weird Mozilla thing.

I don't want to complicate this bug by adding the operator though.
Comment on attachment 8988581 [details] [diff] [review]
part 2 - Make RefreshDriverTimer ref-counted and hold a strong ref on it on the stack when nsRefreshDriver::Tick can be reached.

sec-approval+ for trunk. 
We'll want a beta and ESR60 patch made and nominated as well.
Attachment #8988581 - Flags: sec-approval? → sec-approval+
Comment on attachment 8988939 [details] [diff] [review]
part 2 for esr60 (with trivial merge conflict resolved)

Approval Request Comment
[Feature/Bug causing the regression]: part 1 - maybe bug 1332226?, part 2 - the introduction of VsyncRefreshDriverTimer probably, which I guess is bug 1121331

[User impact if declined]:potentially exploitable crashes

[Is this code covered by automated tests?]:AFAIK, not explicitly

[Has the fix been verified in Nightly?]:yes

[Needs manual test from QE? If yes, steps to reproduce]: no

[List of other uplifts needed for the feature/fix]: only part 1 + 2 in this bug

[Is the change risky?]:no

[Why is the change risky/not risky?]:
  The patches just adds additional ref-counting to keep objects alive
  for longer.  There are no functional changes besides that.

[String changes made/needed]: none

I haven't checked which "part 2" patch applies on Beta,
pick whichever one applies cleanly, or let me know if you
need a new patch.
Attachment #8988939 - Flags: approval-mozilla-esr60?
Attachment #8988939 - Flags: approval-mozilla-beta?
Nils, thank you very much for reporting this and for testing my
Try builds.  Much appreciated!
Flags: in-testsuite?
Comment on attachment 8988939 [details] [diff] [review]
part 2 for esr60 (with trivial merge conflict resolved)

Crash fix, let's take this for beta 5.
Attachment #8988939 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8988799 - Flags: approval-mozilla-beta+
Note for Aryx:  mats mentioned that the part 2 approved for beta may have merge issues and if so, the part 2 patch for trunk may apply instead.
Mats, why is a bug opened two months earlier a dupe of this bug instead of the other way around?
Flags: needinfo?(mats)
Because I was already working on a fix for this bug before I knew
about that one.  Duping to the bug where the work is done is common
practice to make tracking easier to follow.
Flags: needinfo?(mats)
Do both bugs cover the *exact* same flaw?

I ask for bounty purposes and for advisory purposes?
Nils will eventually ask for a bounty for this report and if it is a dupe of the same exact by an internal fuzzer from two months ago, he's not going to receive a bounty or an advisory in his name for this report.
Flags: needinfo?(mats)
(In reply to Al Billings [:abillings] from comment #47)
> Do both bugs cover the *exact* same flaw?

There are two separate flaws, each fixed by a separate patch here.
When I test the original rev in bug 1452744 (30d72755b174) I can
reproduce the "part 2" flaw there using Testcase #2.
After applying the "part 2" fix to that build I can reproduce
the "part 1" crash.  So I think it's fair to say that we would
likely have found both flaws in that bug eventually.

> I ask for bounty purposes and for advisory purposes?

I'm reasonably sure it's the same underlying flaws in both bugs.
For bounty purposes I would recommend a shared discovery because
the testcase here definitely helped me fix it since it was
immediately reproducible.
Flags: needinfo?(mats)
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Flags: sec-bounty?
Hi Mats, I'm trying to put together some solid STR from the this bug and the information provided in Bug 1452744 to be able to reproduce faulty behavior before verifying the fix. Please correct these steps as needed.

STR:
1. Use an ASAN build from Comment 4 or Comment 5
2. Install FuzzPriv extension from https://github.com/MozillaSecurity/fuzzpriv
3. Use the test case from https://bugzilla.mozilla.org/show_bug.cgi?id=1452744#c18 
4. Open several tabs with that attachment
5. Reload said tabs
6. Close Firefox through File
Flags: needinfo?(mats)
(In reply to Grover Wimberly IV [:Grover-QA] from comment #49)
> 1. Use an ASAN build from Comment 4 

Right, ASAN builds from rev cbf9ea7c5531 or earlier should crash.

> or Comment 5

Don't use that build since that fix was incomplete.
Use the latest Nightly ASAN build to verify it's fixed.

> 3. Use the test case from
> https://bugzilla.mozilla.org/show_bug.cgi?id=1452744#c18 

Right, the STR for that bug is in that comment.

The testcase in this bug should crash too.  It was more reproducible
for me, but if you have problems try opening / reloading it in tabs.
Flags: needinfo?(mats)
Comment on attachment 8988799 [details] [diff] [review]
part 1 - Ensure that 'this' stays alive for the duration of the TickRefreshDriver call.

Fixes a sec-crit. Approved for ESR 60.2.
Attachment #8988799 - Flags: approval-mozilla-esr60+
Attachment #8988939 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: sec-bounty? → sec-bounty+
Putting NeedInfo on myself. Planning on verifying on 7/31.
Flags: needinfo?(gwimberly)
Verified using the following ASAN build:
Mozilla/5.0 (X11; Linux x86_64; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180731105217
Flags: needinfo?(gwimberly)
Verified on esr60.0.2

Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180621121604
Verified on 62.0b13
Mozilla/5.0 (X11; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180730180407
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main62+][adv-esr60.2+]
Alias: CVE-2018-12377
sec-high is a more accurate rating
Keywords: sec-criticalsec-high
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: