Bug 1291665 (CVE-2016-5277)

heap-use-after-free in nsRefreshDriver::Tick

VERIFIED FIXED in Firefox 49

Status

()

P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: nils, Assigned: birtles)

Tracking

({csectype-uaf, sec-high})

50 Branch
mozilla51
csectype-uaf, sec-high
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?
qe-verify +

Firefox Tracking Flags

(firefox48 wontfix, firefox49+ verified, firefox-esr4549+ verified, firefox50+ verified, firefox51+ verified)

Details

(Whiteboard: [adv-main49+][adv-esr45.4+])

Attachments

(5 attachments)

(Reporter)

Description

3 years ago
testcase
The following testcase crashes the latest asan build of Firefox (BuildID=20160802030437):

<script>
function start() {
        o0=window.document;
        fuzzPriv.CC();
        requestAnimationFrame(f1);
}
function f1() {
        o856=document.documentElement['animate'](document.documentElement,Math.random()*10);
        o0.write('<html><body></body></html>');
        setTimeout(f2, 4);
}
function f2() {
        o856.currentTime = 183;
        o856=null;o0=null;
        fuzzPriv.forceGC();
        fuzzPriv.CC();
        location.reload();
}
</script>
<body onload="start()"></body>

ASAN output:
=================================================================
==1141==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000098598 at pc 0x7f794e60de76 bp 0x7fffb0f76f90 sp 0x7fffb0f76f88
READ of size 8 at 0x60e000098598 thread T0 (Web Content)
    #0 0x7f794e60de75 in AddRef /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:37:5
    #1 0x7f794e60de75 in AddRef /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:384
    #2 0x7f794e60de75 in RefPtr /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:111
    #3 0x7f794e60de75 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsRefreshDriver.cpp:1711
    #4 0x7f794e6173ec in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsRefreshDriver.cpp:251:7
    #5 0x7f794e6170b9 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsRefreshDriver.cpp:270:5
    #6 0x7f794e618b34 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsRefreshDriver.cpp:430:9
    #7 0x7f794ef61944 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/ipc/VsyncChild.cpp:64:5
    #8 0x7f7948b0ba9a in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:240:20
    #9 0x7f79485c01cf in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:2133:16
    #10 0x7f79484f0187 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessageChannel.cpp:1661:14
    #11 0x7f79484ecfc6 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessageChannel.cpp:1599:17
    #12 0x7f79484dad97 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessageChannel.cpp:1566:5
    #13 0x7f794850a6b2 in applyImpl<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:729:12
    #14 0x7f794850a6b2 in apply<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:735
    #15 0x7f794850a6b2 in mozilla::detail::RunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:764
    #16 0x7f7948509c9f in Run /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/ipc/MessageChannel.h:550:22
    #17 0x7f7948509c9f in mozilla::ipc::MessageChannel::DequeueTask::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/ipc/MessageChannel.h:569
    #18 0x7f79477256b6 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:1068:7
    #19 0x7f79477a3a9c in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290:10
    #20 0x7f79484f74ef in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessagePump.cpp:100:21
    #21 0x7f794846c1f8 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:232:3
    #22 0x7f794846c1f8 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:225
    #23 0x7f794846c1f8 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:205
    #24 0x7f794df7d49f in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/widget/nsBaseAppShell.cpp:156:3
    #25 0x7f7950025587 in XRE_RunAppShell /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:851:12
    #26 0x7f794846c1f8 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:232:3
    #27 0x7f794846c1f8 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:225
    #28 0x7f794846c1f8 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:205
    #29 0x7f7950024c23 in XRE_InitChildProcess /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:681:7
    #30 0x4dfb2b in content_process_main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:224:19
    #31 0x4dfb2b in main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/browser/app/nsBrowserApp.cpp:357
    #32 0x7f7962b8282f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291
    #33 0x41ba08 in _start (/home/nils/fuzzer3/firefox/firefox+0x41ba08)

0x60e000098598 is located 120 bytes inside of 160-byte region [0x60e000098520,0x60e0000985c0)
freed by thread T0 (Web Content) here:
    #0 0x4b215b in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:38:3
    #1 0x7f79476034c4 in SnowWhiteKiller::~SnowWhiteKiller() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/base/nsCycleCollector.cpp:2685:9
    #2 0x7f79476030b6 in nsCycleCollector::FreeSnowWhite(bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/base/nsCycleCollector.cpp:2859:3
    #3 0x7f79490e637e in AsyncFreeSnowWhite::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/xpconnect/src/XPCJSRuntime.cpp:142:34
    #4 0x7f79477256b6 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:1068:7
    #5 0x7f79477a3a9c in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290:10
    #6 0x7f79484f74ef in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessagePump.cpp:100:21
    #7 0x7f794846c1f8 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:232:3
    #8 0x7f794846c1f8 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:225
    #9 0x7f794846c1f8 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:205
    #10 0x7f794df7d49f in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/widget/nsBaseAppShell.cpp:156:3
    #11 0x7f7950025587 in XRE_RunAppShell /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:851:12
    #12 0x7f794846c1f8 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:232:3
    #13 0x7f794846c1f8 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:225
    #14 0x7f794846c1f8 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:205
    #15 0x7f7950024c23 in XRE_InitChildProcess /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:681:7
    #16 0x4dfb2b in content_process_main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:224:19
    #17 0x4dfb2b in main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/browser/app/nsBrowserApp.cpp:357
    #18 0x7f7962b8282f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291

previously allocated by thread T0 (Web Content) here:
    #0 0x4b247b in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x4e0bcd in moz_xmalloc /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/memory/mozalloc/mozalloc.cpp:83:17
    #2 0x7f794a431c94 in operator new /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:193:12
    #3 0x7f794a431c94 in nsDocument::Timeline() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/base/nsDocument.cpp:3166
    #4 0x7f794e8c9c69 in PresShell::Init(nsIDocument*, nsPresContext*, nsViewManager*, mozilla::StyleSetHandle) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsPresShell.cpp:969:3
    #5 0x7f794a438e1e in nsDocument::doCreateShell(nsPresContext*, nsViewManager*, mozilla::StyleSetHandle) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/base/nsDocument.cpp:3773:3
    #6 0x7f794caac247 in nsHTMLDocument::CreateShell(nsPresContext*, nsViewManager*, mozilla::StyleSetHandle) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/html/nsHTMLDocument.cpp:274:10
    #7 0x7f794e839bbb in nsDocumentViewer::InitPresentationStuff(bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsDocumentViewer.cpp:633:16
    #8 0x7f794e8394cf in nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsDocumentViewer.cpp:887:10
    #9 0x7f794e838667 in nsDocumentViewer::Init(nsIWidget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsDocumentViewer.cpp:617:10
    #10 0x7f794f5a5409 in nsDocShell::SetupNewViewer(nsIContentViewer*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/docshell/base/nsDocShell.cpp:9366:7
    #11 0x7f794f5a3b99 in nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/docshell/base/nsDocShell.cpp:7227:17
    #12 0x7f794f53a1a4 in nsDocShell::CreateContentViewer(nsACString_internal const&, nsIRequest*, nsIStreamListener**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/docshell/base/nsDocShell.cpp:9174:3
    #13 0x7f794f5375df in nsDSURIContentListener::DoContent(nsACString_internal const&, bool, nsIRequest*, nsIStreamListener**, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/docshell/base/nsDSURIContentListener.cpp:128:10
    #14 0x7f79495509bb in nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/uriloader/base/nsURILoader.cpp:720:17
    #15 0x7f794954d1ff in nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/uriloader/base/nsURILoader.cpp:398:30
    #16 0x7f794954c30b in nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/uriloader/base/nsURILoader.cpp:259:8
    #17 0x7f79478a7a2b in nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsBaseChannel.cpp:809:14
    #18 0x7f79478f0bde in nsInputStreamPump::OnStateStart() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsInputStreamPump.cpp:524:14
    #19 0x7f79478f00b8 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsInputStreamPump.cpp:426:25
    #20 0x7f79476e04e0 in nsInputStreamReadyEvent::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/io/nsStreamUtils.cpp:95:9
    #21 0x7f79477256b6 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:1068:7
    #22 0x7f79477a3a9c in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290:10
    #23 0x7f79484f74ef in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessagePump.cpp:100:21
    #24 0x7f794846c1f8 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:232:3
    #25 0x7f794846c1f8 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:225
    #26 0x7f794846c1f8 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:205
    #27 0x7f794df7d49f in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/widget/nsBaseAppShell.cpp:156:3
    #28 0x7f7950025587 in XRE_RunAppShell /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:851:12
    #29 0x7f794846c1f8 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:232:3
    #30 0x7f794846c1f8 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:225
    #31 0x7f794846c1f8 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:205
    #32 0x7f7950024c23 in XRE_InitChildProcess /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:681:7
    #33 0x4dfb2b in content_process_main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:224:19
    #34 0x4dfb2b in main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/browser/app/nsBrowserApp.cpp:357
    #35 0x7f7962b8282f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291

SUMMARY: AddressSanitizer: heap-use-after-free /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:37:5 in AddRef
Shadow bytes around the buggy address:
  0x0c1c8000b060: 00 00 00 02 fa fa fa fa fa fa fa fa 00 00 00 00
  0x0c1c8000b070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02
  0x0c1c8000b080: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c1c8000b090: 00 00 00 00 00 00 00 00 00 00 00 02 fa fa fa fa
  0x0c1c8000b0a0: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c1c8000b0b0: fd fd fd[fd]fd fd fd fd fa fa fa fa fa fa fa fa
  0x0c1c8000b0c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1c8000b0d0: 00 00 00 02 fa fa fa fa fa fa fa fa 00 00 00 00
  0x0c1c8000b0e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02
  0x0c1c8000b0f0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c1c8000b100: 00 00 00 00 00 00 00 00 00 00 00 02 fa fa fa 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
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  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
==1141==ABORTING
Brian, can somebody look at this? Thanks.
Flags: needinfo?(bbirtles)
Keywords: csectype-uaf, sec-high
(Assignee)

Comment 2

3 years ago
It looks like we're destroying the timeline without unregistering from the refresh driver. Presumably this happens because we release the document created with Document.write (and we don't go through the usual presshell/refreshdriver destruction that we would for regular documents).
(Assignee)

Comment 3

3 years ago
I wonder if we fail this test here: http://searchfox.org/mozilla-central/rev/740bb4ed16d070cf5d466231b30e80b9204aec54/dom/base/nsDocument.cpp#2133

Or if we simply fail to call Reset at all.
(Assignee)

Comment 4

3 years ago
It's going to take a few days to triage and assign this properly (it always takes me a while to get an ASAN build going, especially with the fuzzing extension, but this time it appears I have to rebuild clang too). Anyway, it's at the top of my list.
Flags: needinfo?(bbirtles)
(Assignee)

Updated

3 years ago
Priority: -- → P1
Assignee: nobody → bbirtles
I have a handy ASAN build, so, I tried.

(In reply to Brian Birtles (:birtles) from comment #3)
> I wonder if we fail this test here:
> http://searchfox.org/mozilla-central/rev/
> 740bb4ed16d070cf5d466231b30e80b9204aec54/dom/base/nsDocument.cpp#2133
> 
> Or if we simply fail to call Reset at all.

Actually this works correctly, but after it,

        o856.currentTime = 183;

here, we re-observed the same refresh driver in DocumentTimeline::NotifyAnimationUpdated.

http://hg.mozilla.org/mozilla-central/file/0ba72e8027cf/dom/animation/DocumentTimeline.cpp#l130

Should we nullify all animations' mTimeline when nsDocument::Reset?
You can confirm the crash without fuzzing addon.
(Assignee)

Comment 7

3 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Created attachment 8777970 [details] [diff] [review]
> Crash test based on  attachment 8777297 [details]
> 
> You can confirm the crash without fuzzing addon.

Great! Thanks for that. Can you verify if this affects release channel too?
Flags: needinfo?(hiikezoe)
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
Flags: needinfo?(hiikezoe)
(Assignee)

Comment 8

3 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> I have a handy ASAN build, so, I tried.
> 
> (In reply to Brian Birtles (:birtles) from comment #3)
> > I wonder if we fail this test here:
> > http://searchfox.org/mozilla-central/rev/
> > 740bb4ed16d070cf5d466231b30e80b9204aec54/dom/base/nsDocument.cpp#2133
> > 
> > Or if we simply fail to call Reset at all.
> 
> Actually this works correctly, but after it,
> 
>         o856.currentTime = 183;
> 
> here, we re-observed the same refresh driver in
> DocumentTimeline::NotifyAnimationUpdated.

Oh, so the reset happens in f1 when we call document.write()? That makes sense.

> Should we nullify all animations' mTimeline when nsDocument::Reset?

I haven't looked into this properly, but I'm not sure if that works. I think we only store a reference to animations that are currently active (e.g. when NeedsTicks() returns true or some other changes has happened causing them to request a tick) so we couldn't get to them all by iterating over AnimationTimeline::mAnimations. If an animation was paused it might not be in the timeline's list of animations but if it was resumed it would end up calling NotifyAnimationUpdated(). Unless you mean to traverse the document and get all the animations that way?

I wonder if instead, after we get a call to NotifyRefreshDriverDestroying we could set a flag on the timeline. Then in NotifyAnimationUpdated() we could just skip re-adding to the refresh driver in that case. I'm not sure if we should clear that flag on a subsequent call to NotifyRefreshDriverCreated, but I suppose we should.

Would that work?
(In reply to Brian Birtles (:birtles) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > I have a handy ASAN build, so, I tried.
> > 
> > (In reply to Brian Birtles (:birtles) from comment #3)
> > > I wonder if we fail this test here:
> > > http://searchfox.org/mozilla-central/rev/
> > > 740bb4ed16d070cf5d466231b30e80b9204aec54/dom/base/nsDocument.cpp#2133
> > > 
> > > Or if we simply fail to call Reset at all.
> > 
> > Actually this works correctly, but after it,
> > 
> >         o856.currentTime = 183;
> > 
> > here, we re-observed the same refresh driver in
> > DocumentTimeline::NotifyAnimationUpdated.
> 
> Oh, so the reset happens in f1 when we call document.write()? That makes
> sense.
> 
> > Should we nullify all animations' mTimeline when nsDocument::Reset?
> 
> I haven't looked into this properly, but I'm not sure if that works. I think
> we only store a reference to animations that are currently active (e.g. when
> NeedsTicks() returns true or some other changes has happened causing them to
> request a tick) so we couldn't get to them all by iterating over
> AnimationTimeline::mAnimations. If an animation was paused it might not be
> in the timeline's list of animations but if it was resumed it would end up
> calling NotifyAnimationUpdated(). Unless you mean to traverse the document
> and get all the animations that way?

I meant to traverse all the animations.  I did't think it's a good idea though.

> I wonder if instead, after we get a call to NotifyRefreshDriverDestroying we
> could set a flag on the timeline. Then in NotifyAnimationUpdated() we could
> just skip re-adding to the refresh driver in that case. I'm not sure if we
> should clear that flag on a subsequent call to NotifyRefreshDriverCreated,
> but I suppose we should.

Yes, this should work.  A remaining concern is that when we can change the old timeline to a new one.  Is it unnecessary?  I don't remember we set timeline when the animation is associated with a document.
Group: core-security → dom-core-security
(Assignee)

Comment 10

3 years ago
Adding some debugging output to attachment 8777970 [details] [diff] [review] I see the following:

  REFTEST TEST-START | file:///home/brian/src/dom/animation/test/crashtests/1291665-1.html
  REFTEST TEST-LOAD | file:///home/brian/src/dom/animation/test/crashtests/1291665-1.html | 0 / 1 (0%)
  C++: >> DocumentTimeline(0x60e000064dc0): NotifyRefreshDriverCreated
          (refresh driver: 0x61800004b080)
  C++: << DocumentTimeline(0x60e000064dc0): NotifyRefreshDriverCreated

  JS: Calling animate

  C++: Animation constructed (0x6120000aa5c0)
  C++: >> DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
          (animation: 0x6120000aa5c0)
  C++:    Got refresh driver: 0x61800004b080
  C++:    Added ourselves as refresh driver observer
  C++: << DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
  C++: >> DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
          (animation: 0x6120000aa5c0)
  C++: << DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
  C++: >> DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
          (animation: 0x6120000aa5c0)
  C++: << DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated

  JS: Calling document.write

  C++: >> DocumentTimeline(0x60e000064dc0): NotifyRefreshDriverDestroying
          (refresh driver: 0x61800004b080)
  C++:    Unregistering as refresh observer
  C++: << DocumentTimeline(0x60e000064dc0): NotifyRefreshDriverDestroying

  JS: Calling setTimeout
  JS: Setting currentTime

  C++: >> DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
          (animation: 0x6120000aa5c0)
  C++:    Got refresh driver: 0x61800004b080
  C++:    Added ourselves as refresh driver observer
  C++: << DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated

  JS: Nulling objects
  JS: Triggering GC
  JS: Calling location.reload

  C++: ~DocumentTimeline(0x60e000064dc0) [mIsObservingRefreshDriver: 1]
  C++: ~Animation(0x6120000aa5c0)
  =================================================================
  ==10913==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000064e38 at pc 0x7f965371dc02 bp 0x7ffc97611db0 sp 0x7ffc97611da8
  READ of size 8 at 0x60e000064e38 thread T0 (Web Content)
  REFTEST TEST-PASS | file:///home/brian/src/dom/animation/test/crashtests/1291665-1.html | (LOAD ONLY)
  REFTEST TEST-END | file:///home/brian/src/dom/animation/test/crashtests/1291665-1.html
      #0 0x7f965371dc01 in AddRef /home/brian/src/objdir-ff-asan/dist/include/mozilla/RefPtr.h:37:5
      #1 0x7f965371dc01 in AddRef /home/brian/src/objdir-ff-asan/dist/include/mozilla/RefPtr.h:384
      #2 0x7f965371dc01 in RefPtr /home/brian/src/objdir-ff-asan/dist/include/mozilla/RefPtr.h:111
      #3 0x7f965371dc01 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/brian/src/layout/base/nsRefreshDriver.cpp:1711
      #4 0x7f9653727aac in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/brian/src/layout/base/nsRefreshDriver.cpp:251:7
      #5 0x7f9653727779 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/brian/src/layout/base/nsRefreshDriver.cpp:270:5
      ...

So there appear to be a few problems here:

a. We can have a situation where we have a cycle between a DocumentTimeline and
   an Animation (i.e. where they are the only things keeping each other alive).
   When we break that cycle, there is nothing that causes the DocumentTimeline
   to be removed from its refresh driver. (The assertion in ~DocumentTimeline
   that checks mIsObservingRefreshDriver is false should fail in this case but
   this is an opt build so it doesn't show up above.)

b. Even after calling NotifyRefreshDriverDestroying() we can still end up
   calling GetRefreshDriver() from DocumentTimeline::NotifyAnimationUpdated and
   re-registering with the (supposedly) destroyed refresh driver.
   
   This is surprising, I would have thought that the pres context would not
   return a valid refresh driver after we call NotifyRefreshDriverDestroying()
   but I *think* what happens is that we don't actually destroy the refresh
   driver in nsDocument::Reset but we simply call that method as a means of
   getting the timeline to disassociate itself from the refresh driver before
   it is destroyed. The version history seems to show this too:

   https://hg.mozilla.org/mozilla-central/rev/84169b40100b4248f0cbdfbd0e42e27e358d5e80

So I think there are a few things we can do here:

1. Set a flag in DocumentTimeline::NotifyRefreshDriverDestroying to indicate
   "refresh driver has been destroyed" and, in
   DocumentTimeline::GetRefreshDriver(), return nullptr when that flag is set.
   In DocumentTimeline::NotifyRefreshDriverCreated we would clear the flag.

2. Make DocumentTimeline::Unlink unregister from the refresh driver.

3. Make Animation::Unlink call DocumentTimeline->RemoveAnimation.

4. In ~DocumentTimeline unregister from the refresh driver.

(1) alone would fix this bug, but I think we should probably do something
along the lines of (2) or (3) or both as well. We probably don't need (4) and
can just leave the assertion there---or should we convert it to a MOZ_CRASH?

For now I'll just do (1) and (2).
(Assignee)

Comment 11

3 years ago
(In reply to Brian Birtles (:birtles) from comment #10)
> (1) alone would fix this bug, but I think we should probably do something
> along the lines of (2) or (3) or both as well. We probably don't need (4) and
> can just leave the assertion there---or should we convert it to a MOZ_CRASH?
> 
> For now I'll just do (1) and (2).

On further thought, while (1) would fix *this* bug, we could have a very similar bug which would not be fixed by (1) if instead of using the default document timeline, we use a separately constructed timeline (i.e. using `new DocumentTimeline`) since we won't call NotifyRefreshDriverDestroying on that. So probably the minimum requirement for fixing this is (2). I'll make these up as separate patches and we can decide which to land where.
Status: NEW → ASSIGNED
(Assignee)

Comment 12

3 years ago
This is approach (2) from comment 10. I have verified that attachment 8777970 [details] [diff] [review] no longer crashes and ASAN build with this patch applied but I have not run it through try. This patch, I believe, also fixes the case when we have a timeline created with `new DocumentTimeline(...)`.
Attachment #8781392 - Flags: review?(hiikezoe)
(Assignee)

Updated

3 years ago
Attachment #8781392 - Attachment is patch: true
(Assignee)

Comment 13

3 years ago
This is approach (1) from comment 10. I think attachment 8781392 [details] [diff] [review] is enough and we don't need to land this patch to fix this bug.

However, I think it's a bug that we call NotifyRefreshDriverDestroyed and then sometimes re-register with it anyway.

We should either:

i.  Not call NotifyRefreshDriverDestroyed from nsDocument::Reset
ii. Land this patch sometime.

(i) seems simpler but I'm not sure if eagerly disassociating from the refresh driver might have other benefits such as releasing resources sooner and stopping the refresh driver from spinning. On the other hand, perhaps it could be argued that the default document should keep functioning even after the document is destroyed (like document timelines created using `new DocumentTimeline`).

So I'm not quite sure what to do here, but I'm posting this patch for now since we might use it later.
Attachment #8781395 - Flags: review?(hiikezoe)
Comment on attachment 8777970 [details] [diff] [review]
Crash test based on  attachment 8777297 [details]

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

It would be nice that we have a utility function which does;

  refreshDriver->RemoveRefreshObserver(this, Flush_Style);                            
  mIsObservingRefreshDriver = false;

and reuse the function in NotifyRefreshDriverDestroying() or rename NotifyRefreshDriverDestroying in a later bug?
Attachment #8777970 - Flags: review+
Comment on attachment 8781395 [details] [diff] [review]
Ignore a refresh driver marked as destroyed in DocumentTimeline

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

I am stamping this patch as r+ too.

(In reply to Brian Birtles (:birtles) from comment #13)
> However, I think it's a bug that we call NotifyRefreshDriverDestroyed and
> then sometimes re-register with it anyway.
> 
> We should either:
> 
> i.  Not call NotifyRefreshDriverDestroyed from nsDocument::Reset
> ii. Land this patch sometime.
> 
> (i) seems simpler but I'm not sure if eagerly disassociating from the
> refresh driver might have other benefits such as releasing resources sooner
> and stopping the refresh driver from spinning. On the other hand, perhaps it
> could be argued that the default document should keep functioning even after
> the document is destroyed (like document timelines created using `new
> DocumentTimeline`).
> 
> So I'm not quite sure what to do here, but I'm posting this patch for now
> since we might use it later.

+1 to land this later.
Attachment #8781395 - Flags: review?(hiikezoe) → review+
Comment on attachment 8781392 [details] [diff] [review]
Unregister from refresh driver in DocumentTimeline::Unlink

I am sorry, I accidentally set r+ against the test case.
Attachment #8781392 - Flags: review?(hiikezoe) → review+
Attachment #8777970 - Flags: review+
(Assignee)

Comment 17

3 years ago
Comment on attachment 8781392 [details] [diff] [review]
Unregister from refresh driver in DocumentTimeline::Unlink

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not too easily. It's clear the bug relates to failing to unregister from the refresh driver, but you have to be lucky with GC timing to trigger the bug (or find a way of forcing GC to happen at the right time--is there a way, other than using a chrome-privileges API to do that?).

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, however they make it clear that sometimes we fail to unregister from the refresh driver (which one could easily guess implies a UAF).

> Which older supported branches are affected by this flaw?
At least up to 48. I believe this was introduced in bug 1195180 which landed in 44, so I expect ESR45 is affected.

> If not all supported branches, which bug introduced the flaw?
(Bug 1195180)

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Not yet but I can easily prepare them. They should not be very different, very difficult to create, or risky.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely to cause regressions. I have yet to run it through try for fear of exposing the vulnerability. Please advise how to proceed.
Attachment #8781392 - Flags: sec-approval?
sec-approval+ for trunk.
We'll want Aurora, Beta, and ESR45 patches made and nominated so they can land after this is good on trunk.
status-firefox48: affected → wontfix
status-firefox-esr45: --- → affected
tracking-firefox49: --- → +
tracking-firefox50: --- → +
tracking-firefox51: --- → +
tracking-firefox-esr45: --- → 49+
Attachment #8781392 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 20

3 years ago
[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
n/a (sec-high)
> User impact if declined: 
Potentially exploitable use-after-free
> Fix Landed on Version:
Has just landed on m-i (50) now.
> Risk to taking this patch (and alternatives if risky): 
Only the usual risk of unforeseen side effects although the changes are limited.
> String or UUID changes made by this patch: 
None

Approval Request Comment
[Feature/regressing bug #]: bug 1195180
[User impact if declined]: Potentially exploitable use-after-free
[Describe test coverage new/current, TreeHerder]: Just landed on m-i
[Risks and why]: Only the usual risk of unforeseen side effects (should be mitigated by watching results of landing on m-c).
[String/UUID change made/needed]: None
Attachment #8781814 - Flags: approval-mozilla-release?
Attachment #8781814 - Flags: approval-mozilla-esr45?
(Assignee)

Comment 21

3 years ago
Comment on attachment 8781392 [details] [diff] [review]
Unregister from refresh driver in DocumentTimeline::Unlink

Approval Request Comment
[Feature/regressing bug #]: bug 1195180
[User impact if declined]: Potentially exploitable use-after-free
[Describe test coverage new/current, TreeHerder]: Just landed on m-i
[Risks and why]: Only the usual risk of unforeseen side effects (should be mitigated by watching results of landing on m-c).
[String/UUID change made/needed]: None
Attachment #8781392 - Flags: approval-mozilla-beta?
Attachment #8781392 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 22

3 years ago
Comment on attachment 8781814 [details] [diff] [review]
Backport of fix for ESR45 / Release (48); r=hiro

Clearing release flag since I notice that in comment 18 this bug is marked as wontfix for 48.
Attachment #8781814 - Flags: approval-mozilla-release?
(Assignee)

Updated

3 years ago
status-firefox-esr45: affected → checkin-pending
https://hg.mozilla.org/mozilla-central/rev/3f9686fc43a6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Group: dom-core-security → core-security-release
Attachment #8781392 - Flags: approval-mozilla-beta?
Attachment #8781392 - Flags: approval-mozilla-beta+
Attachment #8781392 - Flags: approval-mozilla-aurora?
Attachment #8781392 - Flags: approval-mozilla-aurora+
Attachment #8781814 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/a55ede08a8ba
https://hg.mozilla.org/releases/mozilla-beta/rev/11354102d2d2
status-firefox49: affected → fixed
status-firefox50: affected → fixed
Flags: in-testsuite?
Keywords: checkin-needed
Flags: qe-verify+
Reproduced the crash on FX 48.0.2, Win 7.
Verified fixed FX 49.0, 50.0a2 (2016-09-06), 51.0a1 (2016-09-06), 45.4.0 ESR.
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
status-firefox50: fixed → verified
status-firefox51: fixed → verified
status-firefox-esr45: fixed → verified
Whiteboard: [adv-main49+][adv-esr45.4+]
Alias: CVE-2016-5277
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.