Closed Bug 1276857 Opened 8 years ago Closed 8 years ago

heap-buffer-overflow in mozilla::a11y::Accessible::InsertChildAt

Categories

(Core :: Disability Access APIs, defect)

49 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 --- unaffected
firefox48 + verified
firefox49 + verified
firefox-esr45 --- unaffected
firefox50 + verified

People

(Reporter: nils, Assigned: surkov)

References

Details

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

Attachments

(6 files, 1 obsolete file)

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

Testcase:

<script>
function start() {
        o2=(new DOMParser()).parseFromString(' inputmode>','text/html');
        o5=o2.all[2];
        o9=document.createElementNS('http://www.w3.org/1999/xhtml','style');
        document.head.appendChild(o9);
        o13=(new DOMParser()).parseFromString("<header><i>aaaa<style>@font-face</style><link>",'text/html');
        o15=o13.all[1];
        o16=o13.all[2];
        o20=o13.all[6];
        o15.innerHTML="<style><image>{}\n* * *{; float: left;>";
        o25=o15.querySelectorAll('*')[0];
        document.documentElement.appendChild(o25);
        window.top.document.body=o16;
        o20.style.display='table-column';
        o16.style.display='table-header-group';
        setTimeout("next()",4);
}
function next() {
        window.top.document.body=o5;
        o9.appendChild(o20);
        document.location.reload(true);
}
</script>
<body onload="start()"></body>

ASAN crash:

=================================================================
==29022==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000255240 at pc 0x7f3b48a78abf bp 0x7fff714c9500 sp 0x7fff714c94f8
WRITE of size 18446744073709551608 at 0x603000255240 thread T0 (Web Content)
    #0 0x7f3b48a78abe in MoveElements /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:593
    #1 0x7f3b48a78abe in ShiftData<nsTArrayInfallibleAllocator> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray-inl.h:272
    #2 0x7f3b48a78abe in InsertElementAt<mozilla::a11y::Accessible *&, nsTArrayInfallibleAllocator> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:1406
    #3 0x7f3b48a78abe in mozilla::a11y::Accessible::InsertChildAt(unsigned int, mozilla::a11y::Accessible*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/accessible/generic/Accessible.cpp:2080
    #4 0x7f3b48a904b2 in InsertAfter /builds/slave/m-cen-l64-asan-000000000000000/build/src/accessible/generic/Accessible.h:389
    #5 0x7f3b48a904b2 in mozilla::a11y::DocAccessible::ProcessContentInserted(mozilla::a11y::Accessible*, nsIContent*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/accessible/generic/DocAccessible.cpp:1826
    #6 0x7f3b48a0dbf0 in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-000000000000000/build/src/accessible/base/NotificationController.cpp:319
    #7 0x7f3b478f8b0f in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsRefreshDriver.cpp:1701
    #8 0x7f3b47906a6b in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsRefreshDriver.cpp:246
    #9 0x7f3b47906650 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsRefreshDriver.cpp:265
    #10 0x7f3b47905b34 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsRefreshDriver.cpp:425
    #11 0x7f3b4823c660 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/ipc/VsyncChild.cpp:64
    #12 0x7f3b42180932 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:233
    #13 0x7f3b41cb520c in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1890
    #14 0x7f3b41be7c33 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessageChannel.cpp:1655
    #15 0x7f3b41be4765 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessageChannel.cpp:1593
    #16 0x7f3b41bd1fd2 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessageChannel.cpp:1560
    #17 0x7f3b41bf78d0 in applyImpl<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:707
    #18 0x7f3b41bf78d0 in apply<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:713
    #19 0x7f3b41bf78d0 in nsRunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:741
    #20 0x7f3b41bf839f in Run /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/ipc/MessageChannel.h:477
    #21 0x7f3b41bf839f in mozilla::ipc::MessageChannel::DequeueTask::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/ipc/MessageChannel.h:496
    #22 0x7f3b40e658db in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:1073
    #23 0x7f3b40edfe2a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290
    #24 0x7f3b41bef56e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessagePump.cpp:98
    #25 0x7f3b41b6422c in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:235
    #26 0x7f3b41b6422c in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #27 0x7f3b41b6422c in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #28 0x7f3b47278447 in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/widget/nsBaseAppShell.cpp:156
    #29 0x7f3b49293342 in XRE_RunAppShell /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:809
    #30 0x7f3b41b6422c in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:235
    #31 0x7f3b41b6422c in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #32 0x7f3b41b6422c in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #33 0x7f3b49292a21 in XRE_InitChildProcess /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:644
    #34 0x48df67 in content_process_main(int, char**) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:231
    #35 0x7f3b3e6a082f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291
    #36 0x48cb3c in _start (/home/nils/fuzzer3/firefox/plugin-container+0x48cb3c)

0x603000255240 is located 32 bytes inside of 32-byte region [0x603000255220,0x603000255240)
allocated by thread T0 (Web Content) here:
    #0 0x4753cb in realloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:95
    #1 0x48e9fd in moz_xrealloc /builds/slave/m-cen-l64-asan-000000000000000/build/src/memory/mozalloc/mozalloc.cpp:105
    #2 0x7f3b40c943bf in Realloc /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:181
    #3 0x7f3b40c943bf in nsTArrayInfallibleAllocator::ResultTypeProxy nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity<nsTArrayInfallibleAllocator>(unsigned long, unsigned long) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray-inl.h:183
    #4 0x7f3b48a786eb in AppendElement<mozilla::a11y::Accessible *&, nsTArrayInfallibleAllocator> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:1584
    #5 0x7f3b48a786eb in mozilla::a11y::Accessible::InsertChildAt(unsigned int, mozilla::a11y::Accessible*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/accessible/generic/Accessible.cpp:2076
    #6 0x7f3b48a904b2 in InsertAfter /builds/slave/m-cen-l64-asan-000000000000000/build/src/accessible/generic/Accessible.h:389
    #7 0x7f3b48a904b2 in mozilla::a11y::DocAccessible::ProcessContentInserted(mozilla::a11y::Accessible*, nsIContent*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/accessible/generic/DocAccessible.cpp:1826
    #8 0x7f3b48a0dbf0 in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-000000000000000/build/src/accessible/base/NotificationController.cpp:319
    #9 0x7f3b478f8b0f in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsRefreshDriver.cpp:1701
    #10 0x7f3b47906a6b in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsRefreshDriver.cpp:246
    #11 0x7f3b47906650 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsRefreshDriver.cpp:265
    #12 0x7f3b47905b34 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsRefreshDriver.cpp:425
    #13 0x7f3b4823c660 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/ipc/VsyncChild.cpp:64
    #14 0x7f3b42180932 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:233
    #15 0x7f3b41cb520c in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1890
    #16 0x7f3b41be7c33 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessageChannel.cpp:1655
    #17 0x7f3b41be4765 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessageChannel.cpp:1593
    #18 0x7f3b41bd1fd2 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessageChannel.cpp:1560
    #19 0x7f3b41bf78d0 in applyImpl<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:707
    #20 0x7f3b41bf78d0 in apply<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:713
    #21 0x7f3b41bf78d0 in nsRunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:741
    #22 0x7f3b41bf839f in Run /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/ipc/MessageChannel.h:477
    #23 0x7f3b41bf839f in mozilla::ipc::MessageChannel::DequeueTask::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/ipc/MessageChannel.h:496
    #24 0x7f3b40e658db in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:1073
    #25 0x7f3b40edfe2a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290
    #26 0x7f3b41bef56e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessagePump.cpp:98
    #27 0x7f3b41b6422c in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:235
    #28 0x7f3b41b6422c in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #29 0x7f3b41b6422c in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #30 0x7f3b47278447 in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/widget/nsBaseAppShell.cpp:156
    #31 0x7f3b49293342 in XRE_RunAppShell /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:809
    #32 0x7f3b41b6422c in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:235
    #33 0x7f3b41b6422c in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #34 0x7f3b41b6422c in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #35 0x7f3b49292a21 in XRE_InitChildProcess /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:644
    #36 0x48df67 in content_process_main(int, char**) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:231
    #37 0x7f3b3e6a082f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291

SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:593 MoveElements
Shadow bytes around the buggy address:
  0x0c06800429f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0680042a00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0680042a10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0680042a20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0680042a30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c0680042a40: fa fa fa fa 00 00 00 00[fa]fa 00 00 00 06 fa fa
  0x0c0680042a50: fd fd fd fd fa fa fd fd fd fd fa fa fd fd fd fd
  0x0c0680042a60: fa fa fd fd fd fd fa fa fd fd fd fa fa fa fd fd
  0x0c0680042a70: fd fd fa fa fd fd fd fa fa fa fd fd fd fd fa fa
  0x0c0680042a80: fd fd fd fa fa fa fd fd fd fd fa fa fd fd fd fd
  0x0c0680042a90: fa fa fd fd fd fd fa fa fd fd fd fd fa fa fd fd
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
==29022==ABORTING
David: can you find someone to work on this?
Group: core-security → dom-core-security
Flags: needinfo?(dbolter)
Yes, Trevor can you take a first look?
Flags: needinfo?(dbolter) → needinfo?(tbsaunde+mozbugs)
Alex, thoughts?
Flags: needinfo?(surkov.alexander)
The crash happens because  mChildren.Length() is 1 but we call mChildren.InsertElementAt(2, child).  That's because InsertChildAt() is called with index 2 from InsertChildAfter().  The problem there seems to be that in InsertChildAfter aRefChild.mParent != this so inserting at aRefChild->IndexInParent() is bogus.  I'm not really sure why that's happening though.
Flags: needinfo?(tbsaunde+mozbugs)
I don't pretend to understand the test, but here's the problem description. When we receive the document loaded event, the DOM tree is:

: 0x12b13b000, document
      : 0x126399a00, html@id='', idx in parent: 0
        : 0x126bdad70, head@id='', idx in parent: 0
          : 0x126c91040, script@id='', idx in parent: 0
            : 0x128cef700, text node, idx in parent: 0
          : 0x129d53300, text node, idx in parent: 1
          : 0x126d7ccc0, style@id='', idx in parent: 2
        : 0x1272c1c40, body@id='', idx in parent: 1
          : 0x1353da0d0, header@id='', idx in parent: 0
            : 0x1353da160, i@id='', idx in parent: 0
              : 0x12a7e4f00, text node, idx in parent: 0
              : 0x127131270, style@id='', idx in parent: 1
                : 0x12a7e5000, text node, idx in parent: 0
              : 0x129870940, link@id='', idx in parent: 2
        : 0x1271329d0, style@id='', idx in parent: 2
          : 0x12a7e5f00, text node, idx in parent: 0

then, we receive content removed for HTML element and the DOM tree is:

: 0x12b13b000, document
      : 0x126399a00, html@id='', idx in parent: 0
        : 0x126bdad70, head@id='', idx in parent: 0
          : 0x126c91040, script@id='', idx in parent: 0
            : 0x128cef700, text node, idx in parent: 0
          : 0x129d53300, text node, idx in parent: 1
          : 0x126d7ccc0, style@id='', idx in parent: 2
        : 0x1271329d0, style@id='', idx in parent: 1
          : 0x12a7e5f00, text node, idx in parent: 0

so when we process this notification, the DOM tree was heavy altered, thus we fail to find accessible DOM nodes by DOM traversal (for example, accessible for HTML:header), and thus we fail to update the accessible tree properly.

Then we operate on out of dated accessible tree and eventually crash, as we get a text leaf accessible insertion with document as a parent, which has previous (allegedly sibling) accessible that has different (outdated) parent.

I'd say we need some help from layout folks to understand if the accessible notification can be made earlier before the DOM tree has been was changed that way.
Flags: needinfo?(surkov.alexander)
alexander: who on the layout team do you need help from? If you don't know, who on your team can you needinfo to figure that out?

We've got an unassigned sec-critical bug here. Until we find an owner we're all responsible to pass this along to the next party and not let it just drop.
Flags: sec-bounty?
Flags: needinfo?(surkov.alexander)
(In reply to Daniel Veditz [:dveditz] from comment #6)
> alexander: who on the layout team do you need help from?

not sure, bz and roc were helpful on layout stuff

> If you don't know,
> who on your team can you needinfo to figure that out?

maybe david bolter is a good candidate for that
Flags: needinfo?(surkov.alexander)
Defaulting to Alex as assignee for this one. Do we want a band-aid patch on hand (*holds nose*)?

Boris, I'm doubtful you have time to help here? (see comment 5 and comment 7)

Dan is there someone who could find the regression range for this (requires ASAN I assume)?
Assignee: nobody → surkov.alexander
Flags: needinfo?(dveditz)
Flags: needinfo?(bzbarsky)
> Dan is there someone who could find the regression range for this (requires
> ASAN I assume)?

it asserts in debug builds so that's enough.
(In reply to David Bolter [:davidb] from comment #8)
> Defaulting to Alex as assignee for this one. Do we want a band-aid patch on
> hand (*holds nose*)?

I think I can provide a quick fix for that, possibly with todo mochitest. That will fix the crash and address the security risk I think, but the root problem will remain (out of dated accessible tree which is correctness and memory issue).
Attached patch crash fix onlySplinter Review
the root problem is not solved, but in case if want to address the security problem before the root problem fixed, here's a fix.
Attachment #8764045 - Flags: review?(yzenevich)
Comment on attachment 8764045 [details] [diff] [review]
crash fix only

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

This looks good, just a couple of nits

::: accessible/generic/DocAccessible.cpp
@@ +1680,5 @@
>     * Iterates to a next accessible within the inserted content.
>     */
>    bool Next();
>  
> +  void Rejected()

nit: comment.

@@ +1819,5 @@
>        CreateSubtree(iter.Child());
>        continue;
>      }
>  
>      MOZ_ASSERT_UNREACHABLE("accessible was rejected");

We are basically MOZ_ASSERT_UNREACHABLE twice (here and in aContainer->InsertAfter)
Attachment #8764045 - Flags: review?(yzenevich) → review+
I was able to reproduce on a Mac using debug builds with "Voice Over" turned on. Maybe there's a less annoying way to enable accessibility, but that works.

crashes:
https://hg.mozilla.org/mozilla-central/rev/d5d53a3b4e50b94cdf85d20690526e5a00d5b63e

doesn't crash:
https://hg.mozilla.org/mozilla-central/rev/971ee95dade06b20742e94d58c471e76705ca478

one merge from inbound
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=971ee95dade0&tochange=d5d53a3b4e50

of those Trevor's fix for bug 1259023 seem the most likely regressor
Keywords: regression
Flags: needinfo?(dveditz)
I'm not sure what help is being asked from me here...

Comment 5 says that first we have a document with a <body> tag in it.  Then there is a ContentRemoved notification and at this point the <body> is gone.  Well, sure.  The <body> is the content that was removed!  I would expect the a11y code to remove the entire branch of the accessible tree corresponding to descendants of the <body>.  If it's not doing that, why not?  Why doesn't this explode all the time?  Removing things from the DOM is a bog-standard thing to do on web pages, and I'd certainly hope we have tests for it in our a11y test suite...
Flags: needinfo?(bzbarsky)
It is not regular body removal, which is handled2 by a11y well. The problem here is the a11y engine is notified *after* all nodes are gone, while we are expected to be notified right before. That's why we fail to update the accessible tree.

Here's a list where a11y called in https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsAccessibilityService%3A%3AContentRemoved%28nsIPresShell+%2A%2C+nsIContent+%2A%29%22
(In reply to Daniel Veditz [:dveditz] from comment #13)

> one merge from inbound
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=971ee95dade0&tochange=d5d53a3b4e50
> 
> of those Trevor's fix for bug 1259023 seem the most likely regressor

there's one more in that list, the bug 1255009, it is guilty I think.
> The problem here is the a11y engine is notified *after* all nodes are gone

That's totally normal.  A normal removeChild() call will first remove the child from the parent, then call ContentRemoved, then null out the pointer from child to parent.  At least on the DOM level.  And that notification is the one that will land you in nsCSSFrameConstructor::ContentRemoved.  So by the time you get ContentRemoved the nodes are gone from the DOM.  Assuming it's an actual DOM removal, of course.
I talked to alexander offline about this a bit.  The testcase is basically interleaving html.replaceChild(newBody, body) calls with some style changes that should cause a reframe of various things, possibly including the <html>.  The question is what the actual notifications to a11y look like as a result, in a lot more detail than is shown above.
OK, more offline conversation.  What happens is that the SetBody() call happens.  This removes the <body> from the DOM (as expected), then notified ContentRemoved to the frame constructor (also as expected).  The thing is, some removals require reconstruction of the ancestor frame.  So nsCSSFrameConstructor::ContentRemoved calls MaybeRecreateContainerForFrameRemoval which decides that yes we need to reconstruct the ancestor and calls RecreateFramesForContent on the <html>.  This then calls into nsCSSFrameConstructor::ContentRemoved and this ends up sending the a11y notification.

So if a11y is assuming that its ContentRemoved notifications correspond to any particular state of the DOM... that's a problem.  They are listening to what's happening with the _frame_ tree, and what the frame tree does on ContentRemoved is destroy everything under the thing being removed and recreate it.  I had thought that's what the a11y code does too, but it sounds like whatever mechanism is there for finding "everything under the thing being removed" is not self-contained, and it needs to be.

Here's a fairly simple testcase for this situation:

  <!DOCTYPE html>
  <div style="display: table">
  Some text
  <span style="display: table-cell">something with accessibles goes here</span>
  More text
  </div>
  <input type="button" value="remove stuff" onclick="document.querySelector('span').remove()">
(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #19)

> So if a11y is assuming that its ContentRemoved notifications correspond to
> any particular state of the DOM... that's a problem.  They are listening to
> what's happening with the _frame_ tree, and what the frame tree does on
> ContentRemoved is destroy everything under the thing being removed and
> recreate it.  I had thought that's what the a11y code does too, but it
> sounds like whatever mechanism is there for finding "everything under the
> thing being removed" is not self-contained, and it needs to be.

I think we could walk frame tree too to find an accessible subtree corresponding to the removing frame tree, if there are no implications like performance downgrade.

> Here's a fairly simple testcase for this situation:
> 
>   <!DOCTYPE html>
>   <div style="display: table">
>   Some text
>   <span style="display: table-cell">something with accessibles goes
> here</span>
>   More text
>   </div>
>   <input type="button" value="remove stuff"
> onclick="document.querySelector('span').remove()">

this one feels different since I get ContentRemoved/Inserted for div element under a body element, and we end up with a tree that misses some text leafs:

container: 0x11e424790; role: document, name: 'file:///Users/heh/mozilla/tests/crashmut2.html', idx: 0, document node: 0x11e99a000
    child: 0x11f212080; role: section, idx: 0, element node: 0x11e6cf500, div@id=''
    child: 0x11e749e80; role: pushbutton, name: 'remove stuff', idx: 1, element node: 0x11faf9160, input@id=''
> since I get ContentRemoved/Inserted for div element under a body element

Yes, but at the point when you get that ContentRemoved the <span> is gone from the tree and all its kids are as well.  So accessibles for those kids would not be found, right?  I thought that was your problem.  Or did I misunderstand?
(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #21)
> > since I get ContentRemoved/Inserted for div element under a body element
> 
> Yes, but at the point when you get that ContentRemoved the <span> is gone
> from the tree and all its kids are as well.  So accessibles for those kids
> would not be found, right?

I didn't look into this, you're possibly be right. It seems we somehow remove children of the DIV, but anyway we end up with a wrong tree.

>  I thought that was your problem.  Or did I
> misunderstand?

that has to be a problem, right. I'll make sure to turn your testcase into mochitest.

what is the best way to fix the problem? should we rely on a frame tree? if so what methods can we use? Alternatively, I guess we could have DOM mutation observer additionally to update the accessible tree on DOM removals.
I assume you don't want to create nodes in the accessible tree that correspond to all DOM nodes, right?

One thing you could do is just have each accessible watch its target DOM node for ParentChainChanged and deal if so.....
(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #23)
> I assume you don't want to create nodes in the accessible tree that
> correspond to all DOM nodes, right?

correct, the accessible tree is a combination of DOM tree and frame tree, DOM nodes having no semantics shouldn't be presented in the accessible tree.

> One thing you could do is just have each accessible watch its target DOM
> node for ParentChainChanged and deal if so.....

ok, thank you, I should try that.
Attached patch mochitest (obsolete) — Splinter Review
Attachment #8768039 - Flags: review?(yzenevich)
Attached patch insertcrashSplinter Review
Attachment #8768044 - Flags: review?(yzenevich)
Tried to request feedback from bz, but have been told, that he's not accepting feedback requests currently. 

Boris, could you take a look at the latest patch, when you get some time?
Attached patch mochitest2Splinter Review
Attachment #8768056 - Flags: review?(yzenevich)
Attachment #8768039 - Attachment is obsolete: true
Attachment #8768039 - Flags: review?(yzenevich)
Comment on attachment 8768044 [details] [diff] [review]
insertcrash

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

the changes look reasonable

::: accessible/base/nsAccessibilityService.cpp
@@ +590,3 @@
>    }
>  #endif
> +  // nsIMutationObserver::ContentRemoved on a document accessible takes care to

nit: takes care to update the tree -> takes care of the tree update
Attachment #8768044 - Flags: review?(yzenevich) → review+
Comment on attachment 8768056 [details] [diff] [review]
mochitest2

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

Looks good, maybe a comment about table children reorder will re-create the table
Attachment #8768056 - Flags: review?(yzenevich) → review+
Attachment #8768044 - Flags: feedback?(bzbarsky)
Comment on attachment 8768044 [details] [diff] [review]
insertcrash

This needs a commit message describing what it's doing.  I have no way to tell what it's trying to do and hence whether it's succeeding.
Attachment #8768044 - Flags: feedback?(bzbarsky) → feedback-
(In reply to Boris Zbarsky [:bz] from comment #31)
> Comment on attachment 8768044 [details] [diff] [review]
> insertcrash
> 
> This needs a commit message describing what it's doing.  I have no way to
> tell what it's trying to do and hence whether it's succeeding.

not sure if that should go into commit message, but the patch basically makes 2 things:
1) accessible tree updates (removals) on nsIMutationObserver::ContentRemoved registered for a DOM document
2) no accessible tree removals on layout's ContentRemoved notifications (nsAccessibilityService::ContentRemoved), if the node is unattached from DOM (it should be handled by 1).

Any other details?
> not sure if that should go into commit message

Yes, it should.  The commit message should describe what the functional change is, unless you think it would say too much about the security issue.

> 1) accessible tree updates (removals) on nsIMutationObserver::ContentRemoved registered for a DOM document

OK, why is this good enough?  Do we send ContentRemoved notifications to the document for various anonymous content changes (and especially for shadow trees being added/removed)?

> 2) no accessible tree removals on layout's ContentRemoved notifications
> (nsAccessibilityService::ContentRemoved), if the node is unattached from DOM (it should be handled by 1).

OK.  That seems reasonable, assuming the document bit caches all the cases we care about.
s/caches/catches/ of course.
(In reply to Boris Zbarsky [:bz] from comment #33)
> > not sure if that should go into commit message
> 
> Yes, it should.  The commit message should describe what the functional
> change is, unless you think it would say too much about the security issue.

I'm up to give short descriptive messages about what was done, leaving all details about how it was fixed in the bug. Those 1 and 2 feel a way long with me for the commit message. I guess I would go with something like 'update accessible tree on DOM removals via nsIMutationObserver' instead.

> > 1) accessible tree updates (removals) on nsIMutationObserver::ContentRemoved registered for a DOM document
> 
> OK, why is this good enough?  Do we send ContentRemoved notifications to the
> document for various anonymous content changes (and especially for shadow
> trees being added/removed)?

I don't know, that's why I asked for your feedback. You suggested as alternative to register ParentChainChanged for each accessible, but I wasn't sure about good implementation of it. It should take more memory to keep all those references to observers, and the observer will be triggered for each node in a subtree if I understand right, which leave questions open about notifications ordering and dupe processing.

> > 2) no accessible tree removals on layout's ContentRemoved notifications
> > (nsAccessibilityService::ContentRemoved), if the node is unattached from DOM (it should be handled by 1).
> 
> OK.  That seems reasonable, assuming the document bit caches all the cases
> we care about.

how can we ensure/check?
> I don't know, that's why I asked for your feedback

I don't know either, for shadow DOM bits.

I _think_ it's presumably OK if the frame constructor ends up doing the right thing...
Oh.  But IsInUncomposedDoc() will _always_ be false for shadow DOM stuff.  So it looks to me like that will get mishandled by the attached patch, no?

And for actual removals from the DOM, IsInUncomposedDoc would still test true when nsAccessibilityService::ContentRemoved is called, I would think, because we do that notification before doing UnbindFromTree.
so that I can safely remove that IsInUncomposedDoc optimization check from nsAccessibilityService::ContentRemoved.

How can check those shadow DOM bits? do you know somebody who might know that or is there a simple use case I can run on my machine?

can you see better approaches to update the tree?
> How can check those shadow DOM bits?

wchen might know.

For the rest, is the code going to be OK with first getting notified on removal of an ancestor via nsAccessibilityService::ContentRemoved (for the frame tree) and then being notified for removal of a descendant via DocAccessible::ContentRemoved?

> can you see better approaches to update the tree?

I would have thought that the simple thing, if you have a way to get the container accessible for a thing being removed in nsAccessibilityService::ContentRemoved, would be to just rebuild the accessible tree under that container.  But you're not doing that.  Presumably because it's too slow?  You could scope it down to accessible kids whose content nodes are descendants of the node being removed in nsAccessibilityService::ContentRemoved...
> > can you see better approaches to update the tree?
> 
> I would have thought that the simple thing, if you have a way to get the
> container accessible for a thing being removed in
> nsAccessibilityService::ContentRemoved, would be to just rebuild the
> accessible tree under that container.  But you're not doing that. 
> Presumably because it's too slow?  You could scope it down to accessible
> kids whose content nodes are descendants of the node being removed in
> nsAccessibilityService::ContentRemoved...

yeah, I've argued for doing it that way for a long time, but Alex is worried about the performance which I guess might be valid.  On the other hand you'd basically be rebuilding the same subtree that is getting reframed so I'm kind of dubious.
cc'ing wchen for comment #33, whether nsIMutationObserver::ContentReserved registered on a DOM document triggers for shadow DOM mutations.
(In reply to Boris Zbarsky [:bz] from comment #39)
> > How can check those shadow DOM bits?
> 
> wchen might know.
> 
> For the rest, is the code going to be OK with first getting notified on
> removal of an ancestor via nsAccessibilityService::ContentRemoved (for the
> frame tree) and then being notified for removal of a descendant via
> DocAccessible::ContentRemoved?

this is ok from tree update perspective logic, but that will make us to traverse DOM tree twice, and I'm not sure I have ideas how to avoid that

> > can you see better approaches to update the tree?
> 
> I would have thought that the simple thing, if you have a way to get the
> container accessible for a thing being removed in
> nsAccessibilityService::ContentRemoved, would be to just rebuild the
> accessible tree under that container.  But you're not doing that. 
> Presumably because it's too slow? 

if you suggest to destroy everything under a container, then, yes, we may end up recreating a large portions of a document (when removed node is inaccessible). I don't have data about how slow it is, but accessible tree creation is not fast operations, and may cause problems like bug 1256706.

> You could scope it down to accessible
> kids whose content nodes are descendants of the node being removed in
> nsAccessibilityService::ContentRemoved...

would it look like? run through all accessible children of a container, walk their DOM parent chain to see if the removed node is in the chain, and if so then shutdown the subtree of an accessible child.

that should work, and maybe even will be better than traversing the removed DOM subtree twice on removals for now, but it seems that makes me further from fixing bug 1285862, where I wanted to catch ContentRemoved/ContentInserted notifications and ignore them in frame type of an accessible was not changed.
Flags: sec-bounty? → sec-bounty+
> run through all accessible children of a container, walk their DOM parent chain to see if the removed node is in the chain, and if so then shutdown the subtree of an accessible child.

Yes, that was my proposal.
Blocks: 1279216
Does this also affect 48? (Bug 1279216 does and is mentioned as a top crash for 48. )
Flags: needinfo?(surkov.alexander)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #44)
> Does this also affect 48? (Bug 1279216 does and is mentioned as a top crash
> for 48. )

I didn't run the testcase on 48, but I think the bug 1279216 should be a dupe of this one as they have same stack. I would suggest to take 'crash fix only' patch (to stop bleeding), and continue work on the bug in follow up as I'm not yet sure, what is a best way to fix the problem.
Flags: needinfo?(surkov.alexander)
Attachment #8764045 - Flags: sec-approval?
Alexander, we need to have the sec-approval? template questions answered before this can get sec-approval+. Can you reset sec-approval on the patch and answer the template questions?
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(surkov.alexander)
Attachment #8764045 - Flags: sec-approval?
Comment on attachment 8764045 [details] [diff] [review]
crash fix only

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

I'm not a guru on that, and I would prefer to have somebody more experienced to answer this questions, but I think I would fail to do that myself.

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

rather not, I will do a commit like 'Shutdown an accessible if cannot be inserted into the tree', it looks like ordinal commit.

Which older supported branches are affected by this flaw?

48 I think

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

bug 1255009

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

the patch should be applied to 48 more or less cleanly

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

it shouldn't, because it's just rejecting unsafe insertions into accessible children array, but it'd be good to have it running for a few days by QA.
Attachment #8764045 - Flags: sec-approval?
sec-approval+ for trunk. Please create and nominate Aurora and Beta patches so we can avoid shipping the bug.
Attachment #8764045 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/63db8ccd9a6fccec310e81bfc4c145f34bb15959
Bug 1276857 - Shutdown an accessible if cannot be inserted into the tree, r=yzen
https://hg.mozilla.org/mozilla-central/rev/63db8ccd9a6f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
I'd like to get a patch for beta as soon as we can, it will likely miss beta 8 build today but can still get into beta 9 next week (for this issue and I hope that will also help with bug 1279216)
Flags: needinfo?(surkov.alexander)
Attached patch beta patchSplinter Review
the original patch applies cleanly against aurora, beta needs this version (removed some logging code as some logging methods are missed on beta)
Flags: needinfo?(surkov.alexander)
Attached patch autora patchSplinter Review
actually aurora needs own version (same reason as beta's one: logging methods are missed)
Do these patches need some extra flag requests or can I put a checking-needed whiteboard status and that's it?
Yes, best if you request uplift on each patch to the right channel for that patch.
Comment on attachment 8771028 [details] [diff] [review]
autora patch

Approval Request Comment
[Feature/regressing bug #]:bug 1255009
[User impact if declined]:security issue, crashes
[Describe test coverage new/current, TreeHerder]: the code is more or less covered by mochitests, the patch itself doesn't have an automated test
[Risks and why]: low-moderate, accurate/small changes in generally complicated logic of accessible tree update code
[String/UUID change made/needed]:no
Attachment #8771028 - Flags: approval-mozilla-aurora?
Comment on attachment 8771025 [details] [diff] [review]
beta patch

Approval Request Comment
[Feature/regressing bug #]:bug 1255009
[User impact if declined]:security issue, crashes
[Describe test coverage new/current, TreeHerder]: the code is more or less covered by mochitests, the patch itself doesn't have an automated test
[Risks and why]: low-moderate, accurate/small changes in generally complicated logic of accessible tree update code
[String/UUID change made/needed]:no
Attachment #8771025 - Flags: approval-mozilla-beta?
Comment on attachment 8771028 [details] [diff] [review]
autora patch

Crash fix, has sec approval, please land this patch on aurora
Attachment #8771028 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8771025 [details] [diff] [review]
beta patch

Crash fix for beta. Please uplift.
Attachment #8771025 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: dom-core-security → core-security-release
Reproduced tab crashes by loading the attached testcase while VoiceOver was turned on (as mentioned in comment 13) with Nightly debug build (from 2016-05-23) under Mac OS X 10.11.1. 
No crashes encountered with Firefox 48 beta 10, latest Dev Edition 49.0a2 and Nightly 50.0a1, under Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: