Closed
Bug 1291665
(CVE-2016-5277)
Opened 9 years ago
Closed 9 years ago
heap-use-after-free in nsRefreshDriver::Tick
Categories
(Core :: DOM: Animation, defect, P1)
Tracking
()
People
(Reporter: nils, Assigned: birtles)
Details
(Keywords: csectype-uaf, reporter-external, sec-high, Whiteboard: [adv-main49+][adv-esr45.4+])
Attachments
(5 files)
|
421 bytes,
text/html
|
Details | |
|
1.62 KB,
patch
|
Details | Diff | Splinter Review | |
|
4.39 KB,
patch
|
hiro
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
|
3.21 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
|
3.72 KB,
patch
|
abillings
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
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
|
||
Comment 1•9 years ago
|
||
Brian, can somebody look at this? Thanks.
Flags: needinfo?(bbirtles)
Keywords: csectype-uaf,
sec-high
|
Assignee | |
Comment 2•9 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•9 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•9 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•9 years ago
|
Priority: -- → P1
|
|
||
Updated•9 years ago
|
Assignee: nobody → bbirtles
|
||
Comment 5•9 years ago
|
||
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?
|
|
||
Comment 6•9 years ago
|
||
You can confirm the crash without fuzzing addon.
|
|
Assignee | |
Comment 7•9 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)
|
|
||
Updated•9 years ago
|
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox51:
--- → affected
Flags: needinfo?(hiikezoe)
|
|
Assignee | |
Comment 8•9 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?
|
|
||
Comment 9•9 years ago
|
||
(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.
|
||
Updated•9 years ago
|
Group: core-security → dom-core-security
|
|
Assignee | |
Comment 10•9 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•9 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•9 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•9 years ago
|
Attachment #8781392 -
Attachment is patch: true
|
|
Assignee | |
Comment 13•9 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 14•9 years ago
|
||
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 15•9 years ago
|
||
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 16•9 years ago
|
||
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+
|
|
||
Updated•9 years ago
|
Attachment #8777970 -
Flags: review+
|
|
Assignee | |
Comment 17•9 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?
|
||
Comment 18•9 years ago
|
||
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-firefox-esr45:
--- → affected
tracking-firefox49:
--- → +
tracking-firefox50:
--- → +
tracking-firefox51:
--- → +
tracking-firefox-esr45:
--- → 49+
|
|
||
Updated•9 years ago
|
Attachment #8781392 -
Flags: sec-approval? → sec-approval+
|
|
Assignee | |
Comment 19•9 years ago
|
||
|
|
Assignee | |
Comment 20•9 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•9 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•9 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•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
|
|
||
Updated•9 years ago
|
Group: dom-core-security → core-security-release
|
|
||
Updated•9 years ago
|
Attachment #8781392 -
Flags: approval-mozilla-beta?
Attachment #8781392 -
Flags: approval-mozilla-beta+
Attachment #8781392 -
Flags: approval-mozilla-aurora?
Attachment #8781392 -
Flags: approval-mozilla-aurora+
|
|
||
Updated•9 years ago
|
Attachment #8781814 -
Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
|
|
||
Updated•9 years ago
|
Keywords: checkin-needed
|
||
Comment 24•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/a55ede08a8ba
https://hg.mozilla.org/releases/mozilla-beta/rev/11354102d2d2
Flags: in-testsuite?
Keywords: checkin-needed
|
|
||
Comment 25•9 years ago
|
||
|
||
Updated•9 years ago
|
Flags: qe-verify+
|
||
Comment 26•8 years ago
|
||
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
|
|
||
Updated•8 years ago
|
Whiteboard: [adv-main49+][adv-esr45.4+]
|
|
||
Updated•8 years ago
|
Alias: CVE-2016-5277
|
|
||
Updated•8 years ago
|
Flags: sec-bounty?
|
|
||
Updated•8 years ago
|
Flags: sec-bounty? → sec-bounty+
|
|
||
Updated•8 years ago
|
Group: core-security-release
|
||
Updated•9 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•