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)
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)
5.11 KB,
patch
|
yzen
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
906 bytes,
text/html
|
Details | |
2.71 KB,
patch
|
yzen
:
review+
bzbarsky
:
feedback-
|
Details | Diff | Splinter Review |
3.14 KB,
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
4.96 KB,
patch
|
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
4.75 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Comment 1•8 years ago
|
||
David: can you find someone to work on this?
Group: core-security → dom-core-security
Flags: needinfo?(dbolter)
Keywords: csectype-bounds,
sec-critical
Comment 2•8 years ago
|
||
Yes, Trevor can you take a first look?
Flags: needinfo?(dbolter) → needinfo?(tbsaunde+mozbugs)
Comment 4•8 years ago
|
||
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)
Updated•8 years ago
|
Assignee | ||
Comment 5•8 years ago
|
||
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)
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
(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)
Comment 8•8 years ago
|
||
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)
Comment 9•8 years ago
|
||
> 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.
Assignee | ||
Comment 10•8 years ago
|
||
(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).
Assignee | ||
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Comment 13•8 years ago
|
||
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
Updated•8 years ago
|
Keywords: regression
Updated•8 years ago
|
Flags: needinfo?(dveditz)
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 15•8 years ago
|
||
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
Assignee | ||
Comment 16•8 years ago
|
||
(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.
Comment 17•8 years ago
|
||
> 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.
Comment 18•8 years ago
|
||
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.
Comment 19•8 years ago
|
||
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()">
Assignee | ||
Comment 20•8 years ago
|
||
(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=''
Comment 21•8 years ago
|
||
> 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?
Assignee | ||
Comment 22•8 years ago
|
||
(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.
Comment 23•8 years ago
|
||
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.....
Assignee | ||
Comment 24•8 years ago
|
||
(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.
Assignee | ||
Comment 25•8 years ago
|
||
Attachment #8768039 -
Flags: review?(yzenevich)
Assignee | ||
Comment 26•8 years ago
|
||
Attachment #8768044 -
Flags: review?(yzenevich)
Assignee | ||
Comment 27•8 years ago
|
||
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?
Assignee | ||
Comment 28•8 years ago
|
||
Attachment #8768056 -
Flags: review?(yzenevich)
Assignee | ||
Updated•8 years ago
|
Attachment #8768039 -
Attachment is obsolete: true
Attachment #8768039 -
Flags: review?(yzenevich)
Comment 29•8 years ago
|
||
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 30•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Attachment #8768044 -
Flags: feedback?(bzbarsky)
Comment 31•8 years ago
|
||
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-
Assignee | ||
Comment 32•8 years ago
|
||
(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?
Comment 33•8 years ago
|
||
> 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.
Comment 34•8 years ago
|
||
s/caches/catches/ of course.
Assignee | ||
Comment 35•8 years ago
|
||
(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?
Comment 36•8 years ago
|
||
> 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...
Comment 37•8 years ago
|
||
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.
Assignee | ||
Comment 38•8 years ago
|
||
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?
Comment 39•8 years ago
|
||
> 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...
Comment 40•8 years ago
|
||
> > 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.
Assignee | ||
Comment 41•8 years ago
|
||
cc'ing wchen for comment #33, whether nsIMutationObserver::ContentReserved registered on a DOM document triggers for shadow DOM mutations.
Assignee | ||
Comment 42•8 years ago
|
||
(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.
Updated•8 years ago
|
Flags: sec-bounty? → sec-bounty+
Comment 43•8 years ago
|
||
> 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.
Comment 44•8 years ago
|
||
Does this also affect 48? (Bug 1279216 does and is mentioned as a top crash for 48. )
Flags: needinfo?(surkov.alexander)
Assignee | ||
Comment 45•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
Attachment #8764045 -
Flags: sec-approval?
Comment 46•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(surkov.alexander)
Attachment #8764045 -
Flags: sec-approval?
Assignee | ||
Comment 47•8 years ago
|
||
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?
Comment 48•8 years ago
|
||
sec-approval+ for trunk. Please create and nominate Aurora and Beta patches so we can avoid shipping the bug.
status-firefox47:
--- → unaffected
status-firefox48:
--- → affected
status-firefox-esr45:
--- → unaffected
tracking-firefox48:
--- → +
Updated•8 years ago
|
Attachment #8764045 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 49•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=296bcd845f58
Assignee | ||
Comment 50•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/63db8ccd9a6fccec310e81bfc4c145f34bb15959 Bug 1276857 - Shutdown an accessible if cannot be inserted into the tree, r=yzen
Comment 51•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/63db8ccd9a6f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 52•8 years ago
|
||
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)
Assignee | ||
Comment 53•8 years ago
|
||
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)
Assignee | ||
Comment 54•8 years ago
|
||
actually aurora needs own version (same reason as beta's one: logging methods are missed)
Assignee | ||
Comment 55•8 years ago
|
||
Do these patches need some extra flag requests or can I put a checking-needed whiteboard status and that's it?
Comment 56•8 years ago
|
||
Yes, best if you request uplift on each patch to the right channel for that patch.
Assignee | ||
Comment 57•8 years ago
|
||
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?
Assignee | ||
Comment 58•8 years ago
|
||
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 59•8 years ago
|
||
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 60•8 years ago
|
||
Comment on attachment 8771025 [details] [diff] [review] beta patch Crash fix for beta. Please uplift.
Attachment #8771025 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•8 years ago
|
Group: dom-core-security → core-security-release
Comment 62•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/6aadf9572f77373308a5c4319770958e4cbf5566
Comment 63•8 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•