Closed Bug 1363723 (CVE-2017-7818) Opened 7 years ago Closed 7 years ago

heap-use-after-free in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation

Categories

(Core :: Disability Access APIs, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 56+ verified
firefox55 --- wontfix
firefox56 + verified
firefox57 + fixed

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)

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
Attached file ASAN output
Group: core-security → dom-core-security
David, can you help us assign this?
Flags: needinfo?(dbolter)
Sorry I missed this bug somehow. Thanks for fuzzing!

Alex can you take a look?
Flags: needinfo?(dbolter) → needinfo?(surkov.alexander)
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)
(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.
note, there's one more ARIA related fix in bug 1372985, which may correlate here. Note, I cannot reproduce on bare trunk btw.
(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)
Flags: sec-bounty?
Frederik, this still reproduces on a very recent build (BuildID=20170711160010)
Flags: needinfo?(nils) → needinfo?(fbraun)
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)
On Linux the add-on is not required. Launching with GNOME_ACCESSIBILITY=1 will also do what you need.
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)
Eitan, are you able to reproduce it? Is any correlation possible between this stack and your work in bug 1378257?
I'm in this code right now, so I will look at it next.
Flags: needinfo?(eitan)
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)
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)
For the record, this is on linux. That seems pretty important here.
Assignee: nobody → eitan
Flags: needinfo?(eitan)
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
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.
Attached file Simplified testcase
Here is a simplified case that will crash your browser much more efficiently.
Attachment #8889642 - Flags: review?(surkov.alexander)
Priority: -- → P1
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?
Oops. Yeah, that wasn't against trunk.
Attachment #8889923 - Flags: review?(surkov.alexander)
Attachment #8889642 - Attachment is obsolete: true
Attachment #8889642 - Flags: review?(surkov.alexander)
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
(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.
(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 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+
Attachment #8889923 - Attachment is obsolete: true
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")
(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?
(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.
use after free. its my new security lingo i adapted in the last 2 weeks :)
(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.
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)
Attachment #8890563 - Attachment is obsolete: true
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?
Flags: needinfo?(eitan)
(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)
(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.
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)
Attachment #8893568 - Flags: review?(surkov.alexander)
Attachment #8890897 - Attachment is obsolete: true
Attachment #8890897 - Flags: review?(surkov.alexander)
Flags: needinfo?(eitan)
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+
(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.
Crap, I totally forgot to ask for sec approval here.
(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 :)
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?
Attached patch beta patch (obsolete) — Splinter Review
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)
Attached patch release patch (obsolete) — Splinter Review
same.
Attachment #8894003 - Flags: review?(surkov.alexander)
We'll need an ESR52 patch as well. I hope it'll be reasonably similar to the release patch.
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 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+
https://hg.mozilla.org/mozilla-central/rev/7faa02d1a6e2
https://hg.mozilla.org/mozilla-central/rev/a5d58cfbab42
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: dom-core-security → core-security-release
(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.
Attached patch release patchSplinter Review
Attachment #8894003 - Attachment is obsolete: true
Attached patch beta patchSplinter Review
Attachment #8894002 - Attachment is obsolete: true
Flags: sec-bounty? → sec-bounty+
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+
See Also: → 1387918
Eitan or Alexander, can you request uplift to beta? Thanks.
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(eitan)
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)
(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?
Flags: needinfo?(eitan)
(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)
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 on attachment 8894534 [details] [diff] [review]
beta patch

Fix a sec-high. Beta56+.
Attachment #8894534 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Please nominate this for ESR52 approval when you get a chance (and patches in other bugs it depends on if necessary).
Flags: needinfo?(eitan)
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?
Attachment #8894534 - Flags: approval-mozilla-esr52?
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)
Fine by me! You're the expert here :)
Flags: needinfo?(ryanvm)
Alias: CVE-2017-7818
Whiteboard: [adv-main56+][adv-esr52.4+]
Flags: qe-verify+
Whiteboard: [adv-main56+][adv-esr52.4+] → [adv-main56+][adv-esr52.4+][post-critsmash-triage]
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+
Depends on: 1388987
Group: core-security-release
No longer depends on: 1388987
Regressions: 1388987
You need to log in before you can comment on or make changes to this bug.