Closed
Bug 1363723
(CVE-2017-7818)
Opened 7 years ago
Closed 6 years ago
heap-use-after-free in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation
Categories
(Core :: Disability Access APIs, defect, P1)
Core
Disability Access APIs
Tracking
()
VERIFIED
FIXED
mozilla57
People
(Reporter: nils, Assigned: eeejay)
References
(Regressed 1 open bug)
Details
(4 keywords, Whiteboard: [adv-main56+][adv-esr52.4+][post-critsmash-triage])
Attachments
(6 files, 6 obsolete files)
1.03 KB,
text/html
|
Details | |
16.65 KB,
text/plain
|
Details | |
428 bytes,
text/html
|
Details | |
11.99 KB,
patch
|
surkov
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
2.95 KB,
patch
|
Details | Diff | Splinter Review | |
3.55 KB,
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The following testcase crashes the latest ASAN build of Firefox. <script> function start() { fuzzPriv.enableAccessibility(); o0=document; o11=document.createElement('input'); o11.id='id2'; o0.designMode='on'; o291=document.createElement("div"); o291.id="id2"; document.documentElement.appendChild(o291); o627=document.createElementNS('http://www.w3.org/2000/svg','svg'); o688=document.createElementNS('http://www.w3.org/2000/svg','filter'); o707=document.createElementNS('http://www.w3.org/2000/svg','feComponentTransfer'); o688.appendChild(o707); o712=document.createElementNS('http://www.w3.org/2000/svg','feDisplacementMap'); o712.setAttribute('id','id4'); o688.appendChild(o712); o627.appendChild(o688); o817=document.createElement('iframe'); document.documentElement.appendChild(o627); document.documentElement.appendChild(o11); document.documentElement.appendChild(o817); o707.setAttribute('aria-owns','id2 id4'); o817.setAttribute('aria-owns','id4'); location.reload(); } </script> <body onload="start()"></body> ASAN output: ================================================================= ==12988==ERROR: AddressSanitizer: heap-use-after-free on address 0x6020001bd7f0 at pc 0x7f4dbdc8cfaf bp 0x7ffff59e5d10 sp 0x7ffff59e5d08 READ of size 8 at 0x6020001bd7f0 thread T0 (Web Content) #0 0x7f4dbdc8cfae in Length /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:398:37 #1 0x7f4dbdc8cfae in RefPtr<mozilla::a11y::Accessible>* nsTArray_Impl<RefPtr<mozilla::a11y::Accessible>, nsTArrayInfallibleAllocator>::InsertElementAt<mozilla::a11y::Accessible*&, nsTArrayInfallibleAllocator>(unsigned long, mozilla::a11y::Accessible*&) /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:2133 #2 0x7f4dbdc8c880 in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation(mozilla::a11y::Accessible*) /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:2117:17 #3 0x7f4dbdbfc9ab in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) /home/worker/workspace/build/src/accessible/base/NotificationController.cpp:801:18 #4 0x7f4dbb64f2f5 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1789:12 #5 0x7f4dbb65da33 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:300:7 #6 0x7f4dbb65d704 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:322:5 #7 0x7f4dbb65fd2b in RunRefreshDrivers /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:752:5 #8 0x7f4dbb65fd2b in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:665 #9 0x7f4dbb65f92f in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:566:9 #10 0x7f4dbbe7dd62 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /home/worker/workspace/build/src/layout/ipc/VsyncChild.cpp:67:16 #11 0x7f4db6152219 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:155:20 #12 0x7f4db5e1dafd in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1541:28 #13 0x7f4db5d76ba4 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2021:25 #14 0x7f4db5d73a37 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1956:17 #15 0x7f4db5d75744 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1825:5 #16 0x7f4db5d75d76 in mozilla::ipc::MessageChannel::MessageTask::Run() /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1858:15 #17 0x7f4db4fe3b40 in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1270:14 #18 0x7f4db4fe0588 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:393:10 #19 0x7f4db5d7eb71 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21 #20 0x7f4db5ce1f90 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:10 #21 0x7f4db5ce1f90 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #22 0x7f4db5ce1f90 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #23 0x7f4dbafbe58f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:27 #24 0x7f4dbe5abf07 in XRE_RunAppShell() /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:893:22 #25 0x7f4db5ce1f90 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:10 #26 0x7f4db5ce1f90 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #27 0x7f4db5ce1f90 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #28 0x7f4dbe5aba35 in XRE_InitChildProcess(int, char**, XREChildData const*) /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:709:34 #29 0x4eb5c3 in content_process_main /home/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:64:30 #30 0x4eb5c3 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:286 #31 0x7f4dd076582f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291 #32 0x41cf18 in _start (/home/nils/fuzzer3/firefox/firefox+0x41cf18) 0x6020001bd7f0 is located 0 bytes inside of 8-byte region [0x6020001bd7f0,0x6020001bd7f8) freed by thread T0 (Web Content) here: #0 0x4bb44b in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:47:3 #1 0x7f4db4ed633d in RawRemove /home/worker/workspace/build/src/xpcom/ds/PLDHashTable.cpp:661:3 #2 0x7f4db4ed633d in PLDHashTable::Remove(void const*) /home/worker/workspace/build/src/xpcom/ds/PLDHashTable.cpp:631 #3 0x7f4dbdc8d1ed in RemoveEntry /home/worker/workspace/build/src/obj-firefox/dist/include/nsTHashtable.h:171:12 #4 0x7f4dbdc8d1ed in Remove /home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:165 #5 0x7f4dbdc8d1ed in mozilla::a11y::DocAccessible::MoveChild(mozilla::a11y::Accessible*, mozilla::a11y::Accessible*, int) /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:2199 #6 0x7f4dbdc8c7fd in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation(mozilla::a11y::Accessible*) /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:2115:9 #7 0x7f4dbdbfc9ab in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) /home/worker/workspace/build/src/accessible/base/NotificationController.cpp:801:18 #8 0x7f4dbb64f2f5 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1789:12 #9 0x7f4dbb65da33 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:300:7 #10 0x7f4dbb65d704 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:322:5 #11 0x7f4dbb65fd2b in RunRefreshDrivers /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:752:5 #12 0x7f4dbb65fd2b in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:665 #13 0x7f4dbb65f92f in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:566:9 #14 0x7f4dbbe7dd62 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /home/worker/workspace/build/src/layout/ipc/VsyncChild.cpp:67:16 #15 0x7f4db6152219 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:155:20 #16 0x7f4db5e1dafd in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1541:28 #17 0x7f4db5d76ba4 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2021:25 #18 0x7f4db5d73a37 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1956:17 #19 0x7f4db5d75744 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1825:5 #20 0x7f4db5d75d76 in mozilla::ipc::MessageChannel::MessageTask::Run() /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1858:15 #21 0x7f4db4fe3b40 in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1270:14 #22 0x7f4db4fe0588 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:393:10 #23 0x7f4db5d7eb71 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21 #24 0x7f4db5ce1f90 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:10 #25 0x7f4db5ce1f90 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #26 0x7f4db5ce1f90 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #27 0x7f4dbafbe58f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:27 #28 0x7f4dbe5abf07 in XRE_RunAppShell() /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:893:22 #29 0x7f4db5ce1f90 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:10 #30 0x7f4db5ce1f90 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #31 0x7f4db5ce1f90 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #32 0x7f4dbe5aba35 in XRE_InitChildProcess(int, char**, XREChildData const*) /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:709:34 #33 0x4eb5c3 in content_process_main /home/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:64:30 #34 0x4eb5c3 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:286 #35 0x7f4dd076582f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291 previously allocated by thread T0 (Web Content) here: #0 0x4bb79c in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3 #1 0x4ec75d in moz_xmalloc /home/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:83:17 #2 0x7f4dbdc8cd15 in operator new /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:194:12 #3 0x7f4dbdc8cd15 in nsTArray<RefPtr<mozilla::a11y::Accessible> >* nsClassHashtable<nsPtrHashKey<mozilla::a11y::Accessible>, nsTArray<RefPtr<mozilla::a11y::Accessible> > >::LookupOrAdd<>(mozilla::a11y::Accessible*) /home/worker/workspace/build/src/obj-firefox/dist/include/nsClassHashtable.h:153 #4 0x7f4dbdc8c163 in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation(mozilla::a11y::Accessible*) /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:2044:59 #5 0x7f4dbdbfc9ab in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) /home/worker/workspace/build/src/accessible/base/NotificationController.cpp:801:18 #6 0x7f4dbb64f2f5 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1789:12 #7 0x7f4dbb65da33 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:300:7 #8 0x7f4dbb65d704 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:322:5 #9 0x7f4dbb65fd2b in RunRefreshDrivers /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:752:5 #10 0x7f4dbb65fd2b in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:665 #11 0x7f4dbb65f92f in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:566:9 #12 0x7f4dbbe7dd62 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /home/worker/workspace/build/src/layout/ipc/VsyncChild.cpp:67:16 #13 0x7f4db6152219 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:155:20 #14 0x7f4db5e1dafd in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1541:28 #15 0x7f4db5d76ba4 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2021:25 #16 0x7f4db5d73a37 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1956:17 #17 0x7f4db5d75744 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1825:5 #18 0x7f4db5d75d76 in mozilla::ipc::MessageChannel::MessageTask::Run() /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1858:15 #19 0x7f4db4fe3b40 in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1270:14 #20 0x7f4db4fe0588 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:393:10 #21 0x7f4db5d7eb71 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21 #22 0x7f4db5ce1f90 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:10 #23 0x7f4db5ce1f90 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #24 0x7f4db5ce1f90 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #25 0x7f4dbafbe58f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:27 #26 0x7f4dbe5abf07 in XRE_RunAppShell() /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:893:22 #27 0x7f4db5ce1f90 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:10 #28 0x7f4db5ce1f90 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #29 0x7f4db5ce1f90 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #30 0x7f4dbe5aba35 in XRE_InitChildProcess(int, char**, XREChildData const*) /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:709:34 #31 0x4eb5c3 in content_process_main /home/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:64:30 #32 0x4eb5c3 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:286 #33 0x7f4dd076582f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291 SUMMARY: AddressSanitizer: heap-use-after-free /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:398:37 in Length Shadow bytes around the buggy address: 0x0c048002faa0: fa fa 00 00 fa fa 00 00 fa fa fd fd fa fa fd fd 0x0c048002fab0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fa 0x0c048002fac0: fa fa fd fd fa fa fd fa fa fa fd fd fa fa fd fd 0x0c048002fad0: fa fa fd fa fa fa fd fd fa fa fd fd fa fa fd fd 0x0c048002fae0: fa fa fd fd fa fa fd fd fa fa fd fa fa fa fd fd =>0x0c048002faf0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa[fd]fa 0x0c048002fb00: fa fa fd fd fa fa 00 00 fa fa 00 00 fa fa 00 00 0x0c048002fb10: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd 0x0c048002fb20: fa fa 00 00 fa fa fd fd fa fa fd fd fa fa 00 00 0x0c048002fb30: fa fa 00 00 fa fa fd fd fa fa fd fd fa fa fd fd 0x0c048002fb40: fa fa fd fd fa fa 00 00 fa fa 00 00 fa fa 00 fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==12988==ABORTING
Updated•7 years ago
|
Group: core-security → dom-core-security
Comment 3•7 years ago
|
||
Sorry I missed this bug somehow. Thanks for fuzzing! Alex can you take a look?
Flags: needinfo?(dbolter) → needinfo?(surkov.alexander)
Comment 4•7 years ago
|
||
it seems funfuzz installation process have changed over last weeks/months, and I cannot install a funfuzz.xpi addon I have on Nightly 55 anymore. Funfuzz repo [1] doesn't seem containing the extension any longer. Could you please update me on steps how to install the extension on my machine?
Flags: needinfo?(surkov.alexander) → needinfo?(nils)
This could be due to the disabling of legacy addons. try with: user_pref("extensions.allow-non-mpc-extensions", true);
Flags: needinfo?(nils)
Comment 6•7 years ago
|
||
(In reply to Nils from comment #5) > This could be due to the disabling of legacy addons. try with: > > user_pref("extensions.allow-non-mpc-extensions", true); this did a trick. cannot reproduce, might be correlating with bug 1369836.
Comment 7•7 years ago
|
||
note, there's one more ARIA related fix in bug 1372985, which may correlate here. Note, I cannot reproduce on bare trunk btw.
Comment 8•7 years ago
|
||
(In reply to alexander :surkov from comment #6) > (In reply to Nils from comment #5) > > This could be due to the disabling of legacy addons. try with: > > > > user_pref("extensions.allow-non-mpc-extensions", true); > > this did a trick. cannot reproduce, might be correlating with bug 1369836. Nils, can you retest It could be that your bug was also found by one of our own fuzzers and we fixed this in another bug.
Flags: needinfo?(nils)
Updated•7 years ago
|
Flags: sec-bounty?
Frederik, this still reproduces on a very recent build (BuildID=20170711160010)
Flags: needinfo?(nils) → needinfo?(fbraun)
Comment 10•7 years ago
|
||
I can confirm this still reproduces. Alexander, here some STR: 1) allow unsigned add-ons, i.e., set xpinstall.signatures.required to false 2) install https://www.squarefree.com/extensions/domFuzzLite3.xpi 3) visit the test HTML file This crash here has proper debug symbols and good links to the relevant source lines, <https://crash-stats.mozilla.com/report/index/ef1ba3ab-ee7d-4857-8db8-1351f0170712> (just like the ASAN output in attachment 8866330 [details]). Back to Alex..
Flags: needinfo?(fbraun) → needinfo?(surkov.alexander)
Comment 11•7 years ago
|
||
On Linux the add-on is not required. Launching with GNOME_ACCESSIBILITY=1 will also do what you need.
Comment 12•7 years ago
|
||
It doesn't crash with me on nightly debug build on os x with voice over running and commented out fuzzPriv.enableAccessibility(); line. Debug build not ASAN one for sure, but if the problem here is in bad indices, then InsertElementAt should catch that by [1]. Btw, line 2 from comment #10 says me that the plugin is not compatible with Nightly 56a01. Any missed step there? [1] https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsTArray.h?q=nsTArray&redirect_type=direct#2118
Flags: needinfo?(surkov.alexander)
Comment 13•7 years ago
|
||
Eitan, are you able to reproduce it? Is any correlation possible between this stack and your work in bug 1378257?
Assignee | ||
Comment 14•7 years ago
|
||
I'm in this code right now, so I will look at it next.
Flags: needinfo?(eitan)
Comment 15•7 years ago
|
||
Cannot install the fuzz extension: "DOM Fuzz Helper could not be installed because it is incompatible with Nightly 56.0a1" Frederik, do you have an update on it?
Flags: needinfo?(fbraun)
Comment 16•7 years ago
|
||
I really hope this back-and-forth is coming to an end, this is getting ridiculous. This reproduced very predictably 7 days ago. Please follow the steps from comment 10. If that does not work, use a Nightly from 7 days ago. (In reply to Frederik Braun [:freddyb] from comment #10) > I can confirm this still reproduces. > Alexander, here some STR: > > 1) allow unsigned add-ons, i.e., set xpinstall.signatures.required to false > 2) install https://www.squarefree.com/extensions/domFuzzLite3.xpi > 3) visit the test HTML file > > This crash here has proper debug symbols and good links to the relevant > source lines, > <https://crash-stats.mozilla.com/report/index/ef1ba3ab-ee7d-4857-8db8- > 1351f0170712> (just like the ASAN output in attachment 8866330 [details]). > > Back to Alex..
Flags: needinfo?(fbraun)
Comment 17•7 years ago
|
||
For the record, this is on linux. That seems pretty important here.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → eitan
Flags: needinfo?(eitan)
Assignee | ||
Comment 18•7 years ago
|
||
What is happening is that DoARIAOwnsRelocation gets an array for the container from mARIAOwnsHash[1] then calls MoveChild[2] that will delete the previous's container array if it is empty[3]. After MoveChild returns, DoARIAOwnsRelocation inserts an element into the new container's array[4]. If the old container, and the new container are identical, the array in DoARIAOwnsRelocation is deleted, and it tries to insert into a deleted array. 1. https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/accessible/generic/DocAccessible.cpp#2073 2. https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/accessible/generic/DocAccessible.cpp#2135 3. https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/accessible/generic/DocAccessible.cpp#2224 4. https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/accessible/generic/DocAccessible.cpp#2137
Assignee | ||
Comment 19•7 years ago
|
||
The reason this takes a while to crash, after an indeterminate amount of scripted reloads, is because there is a race between two containers doing DocAccessible::DoARIAOwnsRelocation, an SVG filter and an iframe. as for the SVG filter, DoARIAOwnsRelocation is scheduled four times: 1. When the container is bound to the document with aria-owns="id2 id4" 2. When div#id2 node is walked (the accessible is not created because it is referenced in aria-owns in another node, instead creation is scheduled via DoARIAOwnsRelocation). 3. When feDisplacementMap#id4 is walked (same case as (2)). 4. When input#id2 is walked (same case as (2)). (Usually the relocation would only be scheduled 3 times in such a case, but because there is an erroneous use of the same id in the document, it is scheduled 4 times. This is not a problem, but it increases the odds of this race) If the iframe's DoARIAOwnsRelocation is called between any of SVG filter's relocations, we get into a bad state. In part, this is because an iframe (OuterDocAccessible) does not accept relocated children. There are three things I think need to be fixed (any one of these will avoid the crash, but we should do all 3). 1. A container should not be allowed to steal a child via aria-owns if it is already aria-owned by another container. 2. MoveChild should fail in the case of the new parent not accepting the child before it resets the relocated state of the child. 3. We should be checking that we are not using a dead array from mARIAOwnsHash after MoveChild. Also, a side effect of bug 1378257 will be that this UAF will go away, I think. But I don't think that is the appropriate fix.
Assignee | ||
Comment 20•7 years ago
|
||
Here is a simplified case that will crash your browser much more efficiently.
Assignee | ||
Comment 21•7 years ago
|
||
Attachment #8889642 -
Flags: review?(surkov.alexander)
Updated•7 years ago
|
Priority: -- → P1
Comment 22•7 years ago
|
||
Comment on attachment 8889642 [details] [diff] [review] Prevent aria-owned nodes from getting into bad state. r?surkov Review of attachment 8889642 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/generic/DocAccessible.cpp @@ -2132,5 @@ > } > } > > if (MoveChild(child, aOwner, insertIdx)) { > - MOZ_ASSERT(mARIAOwnsHash.Get(aOwner) == owned, "We still have our array"); is this against trunk?
Assignee | ||
Comment 23•7 years ago
|
||
Oops. Yeah, that wasn't against trunk.
Attachment #8889923 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•7 years ago
|
Attachment #8889642 -
Attachment is obsolete: true
Attachment #8889642 -
Flags: review?(surkov.alexander)
Comment 24•7 years ago
|
||
Comment on attachment 8889923 [details] [diff] [review] Prevent aria-owned nodes from getting into bad state. r?surkov Review of attachment 8889923 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/generic/DocAccessible.cpp @@ +2124,5 @@ > if (child->Parent() != aOwner) { > + // Child is aria-owned by another container, skip. > + if (child->IsRelocated()) { > + continue; > + } if you moved this condition few lines above, then we could get rid of owned->IndexOf(child) < idx check @@ +2138,5 @@ > } > > if (MoveChild(child, aOwner, insertIdx)) { > + nsTArray<RefPtr<Accessible> >* relocated = mARIAOwnsHash.LookupOrAdd(aOwner); > + MOZ_ASSERT(relocated == owned); my impression was that this indeed may happen, if so, then why do you assert? @@ +2204,3 @@ > } > > aChildren->RemoveElementsAt(aStartIdx, aChildren->Length() - aStartIdx); aChildren may be dead after unsuccessful MoveChild, correct? also how do we want to proceed with rejected children, shutdown them? @@ +2224,5 @@ > #endif > > + if (!aNewParent->IsAcceptableChild(aChild->GetContent())) { > + return false; > + } it makes sense to move it before TreeInfo logging stuff
Assignee | ||
Comment 25•7 years ago
|
||
(In reply to alexander :surkov from comment #24) > Comment on attachment 8889923 [details] [diff] [review] > Prevent aria-owned nodes from getting into bad state. r?surkov > > Review of attachment 8889923 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/generic/DocAccessible.cpp > @@ +2124,5 @@ > > if (child->Parent() != aOwner) { > > + // Child is aria-owned by another container, skip. > > + if (child->IsRelocated()) { > > + continue; > > + } > > if you moved this condition few lines above, then we could get rid of > owned->IndexOf(child) < idx check I don't see how that would work. We still accept relocated children if they are moved within this container. > > @@ +2138,5 @@ > > } > > > > if (MoveChild(child, aOwner, insertIdx)) { > > + nsTArray<RefPtr<Accessible> >* relocated = mARIAOwnsHash.LookupOrAdd(aOwner); > > + MOZ_ASSERT(relocated == owned); > > my impression was that this indeed may happen, if so, then why do you assert? I can't think of any legitimate case where a single relocated child should be removed and added again to the array. Instead of UAFing, we should have an assertion failure. > > @@ +2204,3 @@ > > } > > > > aChildren->RemoveElementsAt(aStartIdx, aChildren->Length() - aStartIdx); > > aChildren may be dead after unsuccessful MoveChild, correct? > also how do we want to proceed with rejected children, shutdown them? I don't think that is possible. MoveChild is being called on children that are explicitly set to not-relocated, so they are not removed from the mARIAOwnsHash. > > @@ +2224,5 @@ > > #endif > > > > + if (!aNewParent->IsAcceptableChild(aChild->GetContent())) { > > + return false; > > + } > > it makes sense to move it before TreeInfo logging stuff Done.
Comment 26•7 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #25) > > if you moved this condition few lines above, then we could get rid of > > owned->IndexOf(child) < idx check > > > I don't see how that would work. We still accept relocated children if they > are moved within this container. you're right indeed, we don't clear the flag if the children are rearranged by aria-owns change. > > > if (MoveChild(child, aOwner, insertIdx)) { > > > + nsTArray<RefPtr<Accessible> >* relocated = mARIAOwnsHash.LookupOrAdd(aOwner); > > > + MOZ_ASSERT(relocated == owned); > > > > my impression was that this indeed may happen, if so, then why do you assert? > > I can't think of any legitimate case where a single relocated child should > be removed and added again to the array. Instead of UAFing, we should have > an assertion failure. > > > aChildren->RemoveElementsAt(aStartIdx, aChildren->Length() - aStartIdx); > > > > aChildren may be dead after unsuccessful MoveChild, correct? > > also how do we want to proceed with rejected children, shutdown them? > > I don't think that is possible. MoveChild is being called on children that > are explicitly set to not-relocated, so they are not removed from the > mARIAOwnsHash. fair enough, let's go with assertions
Comment 27•7 years ago
|
||
Comment on attachment 8889923 [details] [diff] [review] Prevent aria-owned nodes from getting into bad state. r?surkov Review of attachment 8889923 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/generic/DocAccessible.cpp @@ +2197,5 @@ > } > } > } > + > + if (!MoveChild(child, origContainer, idxInParent)) { do DebugOnly<bool> please
Attachment #8889923 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 28•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #8889923 -
Attachment is obsolete: true
Comment 29•6 years ago
|
||
Comment on attachment 8890563 [details] [diff] [review] Prevent aria-owned nodes from getting into bad state. Review of attachment 8890563 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/generic/DocAccessible.cpp @@ +2163,2 @@ > child->SetRelocated(true); > + relocated->InsertElementAt(idx, child); btw, if we are confident than 'relocated' always equals 'owned' then we don't need 'relocated' variable, and then MOZ_ASSERT(owned == mARIAOwnsHash.Get(aOwner), "MoveChild killed ARIA owned array")
Assignee | ||
Comment 30•6 years ago
|
||
(In reply to alexander :surkov from comment #29) > Comment on attachment 8890563 [details] [diff] [review] > Prevent aria-owned nodes from getting into bad state. > > Review of attachment 8890563 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/generic/DocAccessible.cpp > @@ +2163,2 @@ > > child->SetRelocated(true); > > + relocated->InsertElementAt(idx, child); > > btw, if we are confident than 'relocated' always equals 'owned' then we > don't need 'relocated' variable, and then > MOZ_ASSERT(owned == mARIAOwnsHash.Get(aOwner), "MoveChild killed ARIA owned > array") Oh crap, yeah thats not a good change then. The conditional in the previous patch made sure we wouldn't ever get to a UAF. Should I change it back?
Comment 31•6 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #30) > > btw, if we are confident than 'relocated' always equals 'owned' then we > > don't need 'relocated' variable, and then > > MOZ_ASSERT(owned == mARIAOwnsHash.Get(aOwner), "MoveChild killed ARIA owned > > array") > > Oh crap, yeah thats not a good change then. > > The conditional in the previous patch made sure we wouldn't ever get to a > UAF. Should I change it back? yeah (I'm just not sure what UAF is :) ) I'd love to have assertions though, in case if MoveChild changes ARIA owned array. It makes me really worry.
Assignee | ||
Comment 32•6 years ago
|
||
use after free. its my new security lingo i adapted in the last 2 weeks :)
Assignee | ||
Comment 33•6 years ago
|
||
(In reply to alexander :surkov from comment #29) > Comment on attachment 8890563 [details] [diff] [review] > Prevent aria-owned nodes from getting into bad state. > > Review of attachment 8890563 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/generic/DocAccessible.cpp > @@ +2163,2 @@ > > child->SetRelocated(true); > > + relocated->InsertElementAt(idx, child); > > btw, if we are confident than 'relocated' always equals 'owned' then we > don't need 'relocated' variable, and then > MOZ_ASSERT(owned == mARIAOwnsHash.Get(aOwner), "MoveChild killed ARIA owned > array") I take it back, again. So double back. I didn't change that block. We still get another reference of the array and compare to it.
Assignee | ||
Comment 34•6 years ago
|
||
Sorry to come back with another review request. I needed to change a test, and you should probably take a look.
Attachment #8890897 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•6 years ago
|
Attachment #8890563 -
Attachment is obsolete: true
Comment 35•6 years ago
|
||
Comment on attachment 8890897 [details] [diff] [review] Prevent aria-owned nodes from getting into bad state. Review of attachment 8890897 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/tests/mochitest/treeupdate/test_ariaowns.html @@ +343,5 @@ > > /** > + * Attempt to steal an element from other ARIA owns element. This should > + * not be possible. The only child that will get owned into this > + * container is a previously not aria-owned one. maybe: Unsuccessful attempt to steal an element from other ARIA owns element. ARIA owns reference to an element is ignored, if the element is still referenced by other ARIA owns element. @@ -352,5 @@ > ]; > > this.invoke = function stealFromOtherARIAOwns_invoke() > { > - getNode("t3_container2").setAttribute("aria-owns", "t3_child"); why do you have to change this? You could have same test case but having unexpectedInvokerChecker, no?
Updated•6 years ago
|
Flags: needinfo?(eitan)
Assignee | ||
Comment 36•6 years ago
|
||
(In reply to alexander :surkov from comment #35) > @@ -352,5 @@ > > ]; > > > > this.invoke = function stealFromOtherARIAOwns_invoke() > > { > > - getNode("t3_container2").setAttribute("aria-owns", "t3_child"); > > why do you have to change this? You could have same test case but having > unexpectedInvokerChecker, no? Mostly out of confusion and uncertainty. How can you be sure an event did *not* happen? What if the first invoker in a queue expects an event to not happen, but then the second invoker expects it to happen? How do you distinguish them correctly? It seems like a recipe for intermittent failures. Instead I chose to pair the denied aria-owns with one that would succeed, so that we will know that after we receive a REORDER that no other reorders are scheduled.
Flags: needinfo?(eitan)
Comment 37•6 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #36) > > why do you have to change this? You could have same test case but having > > unexpectedInvokerChecker, no? > > Mostly out of confusion and uncertainty. How can you be sure an event did > *not* happen? What if the first invoker in a queue expects an event to not > happen, but then the second invoker expects it to happen? How do you > distinguish them correctly? It seems like a recipe for intermittent failures. we used to wait for a timeout listening events, if the event was happening then we reported about error. I don't recall really any intermittent issues caused by timeout, so I have impression of it as a reliable enough approach. I'm not sure I get your question right about expected/unepxted event ordering problem. Could you rephrase it please? In general we wait for all expected events and then we wait for a certain time for unexpected events. > Instead I chose to pair the denied aria-owns with one that would succeed, so > that we will know that after we receive a REORDER that no other reorders are > scheduled. this definitely make sense, but as a matter of fact it is a different test case, and not necessary equivalent one taking account how complicated aria-owns implementation may be. For a sake of test coverage I wish we had both of them.
Comment 38•6 years ago
|
||
Eitan, are you ok to get back the original test to keep it together with your new test or rather not? While it's not huge deal I believe it is something nice to have.
Flags: needinfo?(eitan)
Assignee | ||
Comment 39•6 years ago
|
||
Attachment #8893568 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•6 years ago
|
Attachment #8890897 -
Attachment is obsolete: true
Attachment #8890897 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(eitan)
Comment 40•6 years ago
|
||
Comment on attachment 8893568 [details] [diff] [review] Prevent aria-owned nodes from getting into bad state. Review of attachment 8893568 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment #9 addressed ::: accessible/generic/DocAccessible.cpp @@ +2165,2 @@ > child->SetRelocated(true); > + relocated->InsertElementAt(idx, child); please don't forget to address comment #9 before landing ::: accessible/tests/browser/tree/browser_test_aria_owns.js @@ +81,5 @@ > + > + await onReorders; > + > + testChildrenIds(four, ["b", "e"]); > + testChildrenIds(two, ["d", "c"]); I'm good that the test was moved into a separate test entity (was separated from "aa" test), but not sure I understand why you switched from "three" to "four" testing. The test itself looks correct. ::: accessible/tests/mochitest/treeupdate/test_ariaowns.html @@ +353,5 @@ > ]; > > this.invoke = function stealFromOtherARIAOwns_invoke() > { > + getNode("t3_container3").setAttribute("aria-owns", "t3_child t3_child2"); the test looks fine, however not sure why you didn't come with unexpectedInvokerChecker, which is simpler. @@ +380,5 @@ > } > > this.getID = function stealFromOtherARIAOwns_getID() > { > + return "Steal! an element from other ARIA owns element"; the string looks funny, Steal! -> Attempt to steal?
Attachment #8893568 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 41•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7faa02d1a6e235009e284fd3e5a7411305689a8a
Assignee | ||
Comment 42•6 years ago
|
||
(In reply to alexander :surkov from comment #40) > @@ +380,5 @@ > > } > > > > this.getID = function stealFromOtherARIAOwns_getID() > > { > > + return "Steal! an element from other ARIA owns element"; > > the string looks funny, Steal! -> Attempt to steal? Oops! Fixed that. Also, I'll have better tests for this soon after a test refactor.
Assignee | ||
Comment 43•6 years ago
|
||
Crap, I totally forgot to ask for sec approval here.
Comment 44•6 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #41) > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > 7faa02d1a6e235009e284fd3e5a7411305689a8a #9, #9, #9, comment #9 doesn't seem addressed :)
Assignee | ||
Comment 45•6 years ago
|
||
Comment on attachment 8893568 [details] [diff] [review] Prevent aria-owned nodes from getting into bad state. [Security approval request comment] How easily could an exploit be constructed based on the patch? Medium to hard? I don't think it is a blueprint of how to write an exploit in JS, but the UAF protection is apparent in the patch. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, I don't think so. We change the behavior now where one container can't "steal" a aria-owned child from another, but I don't think it is apparent how that can be exploited. Which older supported branches are affected by this flaw? This was introduced in 48: https://bugzilla.mozilla.org/show_bug.cgi?id=1256461 If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I would split this patch, and only backport the part of this patch that avoids a UAF. That patch should be easy to rebase. How likely is this patch to cause regressions; how much testing does it need? That is why I would suggest the above partial fix. The patch as-is has some real behavioral changes that should not be backported, and will only increase the risk of regressions on stable branches.
Attachment #8893568 -
Flags: sec-approval?
Assignee | ||
Comment 46•6 years ago
|
||
Alex, can you verify how I split the patch here. Since this is a release channel, just want to make sure.
Attachment #8894002 -
Flags: review?(surkov.alexander)
Comment 48•6 years ago
|
||
We'll need an ESR52 patch as well. I hope it'll be reasonably similar to the release patch.
Blocks: 1256461
status-firefox55:
--- → wontfix
status-firefox56:
--- → affected
status-firefox57:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox56:
--- → +
tracking-firefox57:
--- → +
tracking-firefox-esr52:
--- → 56+
Keywords: regression
Comment 49•6 years ago
|
||
Comment on attachment 8894002 [details] [diff] [review] beta patch Review of attachment 8894002 [details] [diff] [review]: ----------------------------------------------------------------- I don't see "// Child is aria-owned by another container, skip." part from the nightly part, missed? Otherwise looks good, r=me with comments addressed ::: accessible/generic/DocAccessible.cpp @@ +2160,2 @@ > child->SetRelocated(true); > + relocated->InsertElementAt(idx, child); it doesn't have to go to a backport, right? @@ +2224,5 @@ > // after load: $("list").setAttribute("aria-owns", "a b"); > // later: $("list").setAttribute("aria-owns", ""); > if (origContainer != owner || child->IndexInParent() != idxInParent) { > + DebugOnly<bool> moved = MoveChild(child, origContainer, idxInParent); > + MOZ_ASSERT(moved, "Failed to put child back."); same here, just debug stuff, up to you @@ +2249,5 @@ > Accessible* curParent = aChild->Parent(); > > + if (!aNewParent->IsAcceptableChild(aChild->GetContent())) { > + return false; > + } I missed that in the nightly patch, but this condition should be moved one like up, it doesn't make sense to define 'curParent' variable if the child is not acceptable by a parent
Attachment #8894002 -
Flags: review?(surkov.alexander) → review+
Comment 50•6 years ago
|
||
Comment on attachment 8894003 [details] [diff] [review] release patch r=me with same comments addressed as for the beta patch
Attachment #8894003 -
Flags: review?(surkov.alexander) → review+
Comment 51•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7faa02d1a6e2 https://hg.mozilla.org/mozilla-central/rev/a5d58cfbab42
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•6 years ago
|
Group: dom-core-security → core-security-release
Assignee | ||
Comment 52•6 years ago
|
||
(In reply to alexander :surkov from comment #49) > Comment on attachment 8894002 [details] [diff] [review] > beta patch > > Review of attachment 8894002 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't see "// Child is aria-owned by another container, skip." part from > the nightly part, missed? Otherwise looks good, r=me with comments addressed > > ::: accessible/generic/DocAccessible.cpp > @@ +2160,2 @@ > > child->SetRelocated(true); > > + relocated->InsertElementAt(idx, child); > > it doesn't have to go to a backport, right? Using the newly retrieved array? Yes it does. This is the part that remedies the UAF. > > @@ +2224,5 @@ > > // after load: $("list").setAttribute("aria-owns", "a b"); > > // later: $("list").setAttribute("aria-owns", ""); > > if (origContainer != owner || child->IndexInParent() != idxInParent) { > > + DebugOnly<bool> moved = MoveChild(child, origContainer, idxInParent); > > + MOZ_ASSERT(moved, "Failed to put child back."); > > same here, just debug stuff, up to you Pretty benign, I'll leave it. > > @@ +2249,5 @@ > > Accessible* curParent = aChild->Parent(); > > > > + if (!aNewParent->IsAcceptableChild(aChild->GetContent())) { > > + return false; > > + } > > I missed that in the nightly patch, but this condition should be moved one > like up, it doesn't make sense to define 'curParent' variable if the child > is not acceptable by a parent Good call.
Assignee | ||
Comment 53•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #8894003 -
Attachment is obsolete: true
Assignee | ||
Comment 54•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #8894002 -
Attachment is obsolete: true
Updated•6 years ago
|
Flags: sec-bounty? → sec-bounty+
Comment 55•6 years ago
|
||
Comment on attachment 8893568 [details] [diff] [review] Prevent aria-owned nodes from getting into bad state. sec-approval+ at this point. What is the beta patch waiting for here to get nominated?
Attachment #8893568 -
Flags: sec-approval? → sec-approval+
Eitan or Alexander, can you request uplift to beta? Thanks.
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(eitan)
Assignee | ||
Comment 57•6 years ago
|
||
The patch in bug 1387918 solves this issue more holistically with minimal changes. Can we wait for August 28 to land the patches there instead of these? Bill said to have those patches nominated and ready on the 28th. Does that mean requesting uplift before they land on m-c?
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(eitan)
Flags: needinfo?(abillings)
Sure, that makes sense, and we can wait till August 28. Sorry, I missed your link to bug 1387918. Thanks Eitan.
Flags: needinfo?(abillings)
Comment 59•6 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #57) > The patch in bug 1387918 solves this issue more holistically with minimal > changes. The patches from this bug have some good changes which are not part of the minimal changes of bug 1387918, and may be important for stability. I'm worried that betas won't receive them. Eitan, are you sure we don't want these backported?
Updated•6 years ago
|
Flags: needinfo?(eitan)
Assignee | ||
Comment 60•6 years ago
|
||
(In reply to alexander :surkov from comment #59) > (In reply to Eitan Isaacson [:eeejay] from comment #57) > > The patch in bug 1387918 solves this issue more holistically with minimal > > changes. > > The patches from this bug have some good changes which are not part of the > minimal changes of bug 1387918, and may be important for stability. I'm > worried that betas won't receive them. Eitan, are you sure we don't want > these backported? OK. I'll nominate this patch, and rebase the one in bug 1387918.
Flags: needinfo?(eitan)
Assignee | ||
Comment 61•6 years ago
|
||
Comment on attachment 8894534 [details] [diff] [review] beta patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1256461 [User impact if declined]: A possible UAF [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: [List of other uplifts needed for the feature/fix]: [Is the change risky?]: No [Why is the change risky/not risky?]: This is pretty minimal and it has been on Nightly for a while now. [String changes made/needed]:
Attachment #8894534 -
Flags: approval-mozilla-beta?
Comment 62•6 years ago
|
||
Comment on attachment 8894534 [details] [diff] [review] beta patch Fix a sec-high. Beta56+.
Attachment #8894534 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 63•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a60be780c0d7
Comment 64•6 years ago
|
||
Please nominate this for ESR52 approval when you get a chance (and patches in other bugs it depends on if necessary).
Flags: needinfo?(eitan)
Assignee | ||
Comment 65•6 years ago
|
||
Comment on attachment 8894534 [details] [diff] [review] beta patch [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: Potential use after free Fix Landed on Version: 56 Risk to taking this patch (and alternatives if risky): This patch has been introduced in beta and nightly for a while with no regressions. String or UUID changes made by this patch: See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(eitan)
Attachment #8894534 -
Flags: approval-mozilla-esr52?
Assignee | ||
Updated•6 years ago
|
Attachment #8894534 -
Flags: approval-mozilla-esr52?
Assignee | ||
Comment 66•6 years ago
|
||
Come to think of it. I think it would be safer to just take the patch nominated in bug 1387918 that fixes the same UAF. The patch here fixes other functional issues, but I don't think it appropriate for ESR. Ryan, would that be OK?
Flags: needinfo?(ryanvm)
Updated•6 years ago
|
Updated•6 years ago
|
Alias: CVE-2017-7818
Whiteboard: [adv-main56+][adv-esr52.4+]
Updated•6 years ago
|
Flags: qe-verify+
Whiteboard: [adv-main56+][adv-esr52.4+] → [adv-main56+][adv-esr52.4+][post-critsmash-triage]
Comment 68•6 years ago
|
||
Reproduced this crash using the test case from comment 20, on an affected ASAN Nightly build (2017-05-10). Verified fixed on latest ASAN builds 56.0 (20170920224824) and esr 52.4.0 (20170920172358) under Ubuntu 16.04 x64. It seems that on the latest ASAN Nightly 57.0a1 build the fuzPriv extensions can't be installed even if "extensions.allow-non-mpc-extensions" is set to true and "xpinstall.signatures.required" is set to false. I've also tried to install it from about:debugging, but it did not work that way either. Since this crash requires fuzPriv installed I cannot verify this in 57.0a1 Nightly. I think it would be useful for QE to have some info on how to install this extension in Firefox 57 or make it compatibile with this version. Any thoughts about this?
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•6 years ago
|
Group: core-security-release
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•