Closed Bug 1326084 Opened 8 years ago Closed 7 years ago

IPC - Use After Free in a11y::DocAccessibleParent::RemoveChildDoc()

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Version:
53 Branch
Platform:
x86
Windows 10
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- disabled
firefox-esr52 --- disabled
firefox53 --- disabled
firefox54 --- disabled
firefox55 --- verified

People

(Reporter: loobenyang, Assigned: handyman)

References

Details

(Keywords: csectype-uaf, sec-high, topcrash-win, Whiteboard: [post-critsmash-triage] aes+)

Crash Data

Attachments

(4 files, 4 obsolete files)

UAF_RemoveChildDoc_Repro.html
665 bytes, text/plain
Details
ClearChildDoc() should leave the proxy marked as being an outerdoc
1.38 KB, patch
bugzilla
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
rework ProxyAccessible handling of outer docs
2.30 KB, patch
Details | Diff | Splinter Review
1337370 patch with test v2
3.30 KB, patch
yzen
: review+
RyanVM
: feedback+
Details | Diff | Splinter Review 
Reproduction test case:

	<html><body></body><script>
	var iframe = document.body.appendChild(document.createElement("iframe"));
	var element1 = document.createElement("content");
	var element2 = document.createElement("OPTION");
	var element3 = document.createElement("wbr");
	var range = document.createRange();
	range.setStart(element2, 0);
	setTimeout(function(){var select = window.getSelection();}, 50);
	var player0 = document.documentElement.animate([{"position": "relative","offset":"0"},], {duration:34,iterations: 69,delay: 50});
	player0.onfinish = function(){range.insertNode(element1);range.setStartBefore(element3); };
	setTimeout(function(){location.reload(true)},200);</script></html>


Steps to reproduce: 
	1. Open the test case UAF_RemoveChildDoc_Repro.html in Firefox browser.
	2. Firefox crashes at corrupted memory in a11y::DocAccessibleParent::RemoveChildDoc():


		(14e8.374c): Access violation - code c0000005 (!!! second chance !!!)
		eax=1fa0a648 ebx=00000008 ecx=17a0f000 edx=179f61c4 esi=1fa0a630 edi=7f7198ec
		eip=5987e676 esp=00afe268 ebp=00afe274 iopl=0         nv up ei pl nz na pe nc
		cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010206
		xul!nsDefaultComparator<mozilla::a11y::ProxyAccessible *,mozilla::a11y::DocAccessibleParent *>::Equals+0x12 [inlined in xul!nsTArray_Impl<mozilla::a11y::ProxyAccessible *,nsTArrayInfallibleAllocator>::IndexOf<mozilla::a11y::DocAccessibleParent *,nsDefaultComparator<mozilla::a11y::ProxyAccessible *,mozilla::a11y::DocAccessibleParent *> >+0x29]:
		5987e676 3901            cmp     dword ptr [ecx],eax  ds:002b:17a0f000=????????


Firefox version: 53.0a1 (2016-12-28) (32-bit)
OS: Windows 10


Stack trace:




	0:000> !analyze -v
	*******************************************************************************
	*                                                                             *
	*                        Exception Analysis                                   *
	*                                                                             *
	*******************************************************************************


	FAULTING_IP: 
	xul!nsTArray_Impl<mozilla::a11y::ProxyAccessible *,nsTArrayInfallibleAllocator>::IndexOf<mozilla::a11y::DocAccessibleParent *,nsDefaultComparator<mozilla::a11y::ProxyAccessible *,mozilla::a11y::DocAccessibleParent *> >+29 [c:\builds\moz2_slave\m-cen-w32-ntly-000000000000000\build\src\obj-firefox\dist\include\nstarray.h @ 1276]
	5987e676 3901            cmp     dword ptr [ecx],eax

	EXCEPTION_RECORD:  ffffffff -- (.exr 0xffffffffffffffff)
	ExceptionAddress: 5987e676 (xul!nsDefaultComparator<mozilla::a11y::ProxyAccessible *,mozilla::a11y::DocAccessibleParent *>::Equals+0x00000012)
	   ExceptionCode: c0000005 (Access violation)
	  ExceptionFlags: 00000000
	NumberParameters: 2
	   Parameter[0]: 00000000
	   Parameter[1]: 17a0f000
	Attempt to read from address 17a0f000

	CONTEXT:  00000000 -- (.cxr 0x0;r)
	eax=1fa0a648 ebx=00000008 ecx=17a0f000 edx=179f61c4 esi=1fa0a630 edi=7f7198ec
	eip=5987e676 esp=00afe268 ebp=00afe274 iopl=0         nv up ei pl nz na pe nc
	cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010206
	xul!nsDefaultComparator<mozilla::a11y::ProxyAccessible *,mozilla::a11y::DocAccessibleParent *>::Equals+0x12 [inlined in xul!nsTArray_Impl<mozilla::a11y::ProxyAccessible *,nsTArrayInfallibleAllocator>::IndexOf<mozilla::a11y::DocAccessibleParent *,nsDefaultComparator<mozilla::a11y::ProxyAccessible *,mozilla::a11y::DocAccessibleParent *> >+0x29]:
	5987e676 3901            cmp     dword ptr [ecx],eax  ds:002b:17a0f000=????????

	FAULTING_THREAD:  0000374c

	DEFAULT_BUCKET_ID:  INVALID_POINTER_READ

	PROCESS_NAME:  firefox.exe

	OVERLAPPED_MODULE: Address regions for 'softokn3' and 'kbdus.dll' overlap

	ERROR_CODE: (NTSTATUS) 0xc0000005 - The instruction at 0x%p referenced memory at 0x%p. The memory could not be %s.

	EXCEPTION_CODE: (NTSTATUS) 0xc0000005 - The instruction at 0x%p referenced memory at 0x%p. The memory could not be %s.

	EXCEPTION_PARAMETER1:  00000000

	EXCEPTION_PARAMETER2:  17a0f000

	READ_ADDRESS:  17a0f000 

	FOLLOWUP_IP: 
	xul!nsTArray_Impl<mozilla::a11y::ProxyAccessible *,nsTArrayInfallibleAllocator>::IndexOf<mozilla::a11y::DocAccessibleParent *,nsDefaultComparator<mozilla::a11y::ProxyAccessible *,mozilla::a11y::DocAccessibleParent *> >+29 [c:\builds\moz2_slave\m-cen-w32-ntly-000000000000000\build\src\obj-firefox\dist\include\nstarray.h @ 1276]
	5987e676 3901            cmp     dword ptr [ecx],eax

	NTGLOBALFLAG:  2000000

	APPLICATION_VERIFIER_FLAGS:  0

	APP:  firefox.exe

	ANALYSIS_VERSION: 6.3.9600.17029 (debuggers(dbg).140219-1702) x86fre

	PRIMARY_PROBLEM_CLASS:  INVALID_POINTER_READ

	BUGCHECK_STR:  APPLICATION_FAULT_INVALID_POINTER_READ

	LAST_CONTROL_TRANSFER:  from 5987f635 to 5987e676

	STACK_TEXT:  
	00afe274 5987f635 00000000 17a76740 17a76740 xul!nsTArray_Impl<mozilla::a11y::ProxyAccessible *,nsTArrayInfallibleAllocator>::IndexOf<mozilla::a11y::DocAccessibleParent *,nsDefaultComparator<mozilla::a11y::ProxyAccessible *,mozilla::a11y::DocAccessibleParent *> >+0x29
	00afe29c 5987ea0e 1fa0a630 00000000 17a76740 xul!mozilla::a11y::DocAccessibleParent::RemoveChildDoc+0x22
	00afe2d8 5987e9a0 1fbe6c28 17a76740 1fbe6c28 xul!mozilla::a11y::DocAccessibleParent::Destroy+0xa6
	00afe310 5987f39a 1fbe6c28 17a76740 17a76740 xul!mozilla::a11y::DocAccessibleParent::Destroy+0x38
	00afe324 58cc5a7c 00afe33e 1fbe6c28 1ba0a800 xul!mozilla::a11y::DocAccessibleParent::RecvShutdown+0xd
	00afe384 58c9c12a 1fbe6c28 1fbe6c28 1ba0a8a8 xul!mozilla::a11y::PDocAccessibleParent::OnMessageReceived+0x57
	00aff0c0 582b8e5f 1fbe6c28 1ba0a8a8 200cff10 xul!mozilla::dom::PContentParent::OnMessageReceived+0x4b
	00aff0dc 582b8f91 00be6c28 1fbe6c28 1ba0a8a8 xul!mozilla::ipc::MessageChannel::DispatchAsyncMessage+0x4c
	00aff144 582b9296 1fbe6c28 1fbe6c00 200cff10 xul!mozilla::ipc::MessageChannel::DispatchMessageW+0xc2
	00aff15c 582b9231 1fbe6c00 0850b160 08511524 xul!mozilla::ipc::MessageChannel::RunMessage+0x53
	00aff174 5818391c 1fbe6c00 085221a0 08522190 xul!mozilla::ipc::MessageChannel::MessageTask::Run+0x38
	00aff1e4 58182119 0850b160 00000000 00aff217 xul!nsThread::ProcessNextEvent+0x14d
	00aff218 58281d48 0855f0c0 37a307f6 08511520 xul!mozilla::ipc::MessagePump::Run+0x70
	00aff250 58281d17 0850b160 00000001 08511500 xul!MessageLoop::RunHandler+0x20
	00aff270 584c2c61 14ad95c0 00000000 00aff290 xul!MessageLoop::Run+0x19
	00aff280 584c29f0 08511520 14ad95c0 00aff2a4 xul!nsBaseAppShell::Run+0x34
	00aff290 584c29a5 08511520 00aff61d 18215de0 xul!nsAppShell::Run+0x26
	00aff2a4 584cb85c 14ad95c0 62d92c50 00aff520 xul!nsAppStartup::Run+0x22
	00aff498 586501bc 085100b0 00aff65c 00c130c0 xul!XREMain::XRE_mainRun+0x65c
	00aff4c4 5864f6d9 00000000 00000000 00aff65c xul!XREMain::XRE_main+0x157
	00aff62c 00bf17fc 00000001 08501050 00aff65c xul!XRE_main+0x39
	00aff8c4 00bf3385 004eff18 085111c0 00000001 firefox!do_main+0x7cc
	00affc7c 00bf5e07 00000001 ffe03f50 0830cf18 firefox!wmain+0x405
	00affcc4 74228694 00910000 74228670 1e55464b firefox!__scrt_common_main_seh+0xf9
	00affcd8 778004ba 00910000 1df5b00d 00000000 KERNEL32!BaseThreadInitThunk+0x24
	00affd20 7780048a ffffffff 77822f86 00000000 ntdll!__RtlUserThreadStart+0x2f
	00affd30 00000000 00bf5e7d 00910000 00000000 ntdll!_RtlUserThreadStart+0x1b


	STACK_COMMAND:  .cxr 0x0 ; kb

	FAULTING_SOURCE_LINE:  c:\builds\moz2_slave\m-cen-w32-ntly-000000000000000\build\src\obj-firefox\dist\include\nstarray.h

	FAULTING_SOURCE_FILE:  c:\builds\moz2_slave\m-cen-w32-ntly-000000000000000\build\src\obj-firefox\dist\include\nstarray.h

	FAULTING_SOURCE_LINE_NUMBER:  1276

	FAULTING_SOURCE_CODE:  
	No source found for 'c:\builds\moz2_slave\m-cen-w32-ntly-000000000000000\build\src\obj-firefox\dist\include\nstarray.h'


	SYMBOL_STACK_INDEX:  0

	SYMBOL_NAME:  xul!nsTArray_Impl<mozilla::a11y::ProxyAccessible *,nsTArrayInfallibleAllocator>::IndexOf<mozilla::a11y::DocAccessibleParent *,nsDefaultComparator<mozilla::a11y::ProxyAccessible *,mozilla::a11y::DocAccessibleParent *> >+29

	FOLLOWUP_NAME:  MachineOwner

	MODULE_NAME: xul

	IMAGE_NAME:  xul.dll

	DEBUG_FLR_IMAGE_TIMESTAMP:  5863c2af

	FAILURE_BUCKET_ID:  INVALID_POINTER_READ_c0000005_xul.dll!nsTArray_Impl_mozilla::a11y::ProxyAccessible_*,nsTArrayInfallibleAllocator_::IndexOf_mozilla::a11y::DocAccessibleParent_*,nsDefaultComparator_mozilla::a11y::ProxyAccessible_*,mozilla::a11y::DocAccessibleParent_*___

	BUCKET_ID:  APPLICATION_FAULT_INVALID_POINTER_READ_xul!nsTArray_Impl_mozilla::a11y::ProxyAccessible_*,nsTArrayInfallibleAllocator_::IndexOf_mozilla::a11y::DocAccessibleParent_*,nsDefaultComparator_mozilla::a11y::ProxyAccessible_*,mozilla::a11y::DocAccessibleParent_*___+2

	ANALYSIS_SOURCE:  UM

	FAILURE_ID_HASH_STRING:  um:invalid_pointer_read_c0000005_xul.dll!nstarray_impl_mozilla::a11y::proxyaccessible_*,nstarrayinfallibleallocator_::indexof_mozilla::a11y::docaccessibleparent_*,nsdefaultcomparator_mozilla::a11y::proxyaccessible_*,mozilla::a11y::docaccessibleparent_*___

	FAILURE_ID_HASH:  {6ee3eec6-b59d-bfd8-7523-dec28070ab0a}

	Followup: MachineOwner
	---------



Sometimes it crashes at e5e5e5e5. Variables shows that the aChildDoc->mParent pointed to a freed object:


	(5a0.3570): Unknown exception - code c000041d (!!! second chance !!!)
	eax=0093d1f8 ebx=00000008 ecx=187ca884 edx=e5e5e5e5 esi=187ca880 edi=239eba50
	eip=5f12e655 esp=0093d1c8 ebp=0093d1d4 iopl=0         nv up ei pl nz na pe nc
	cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00210206
	xul!nsTArray_Impl<mozilla::a11y::ProxyAccessible *,nsTArrayInfallibleAllocator>::IndexOf<mozilla::a11y::DocAccessibleParent *,nsDefaultComparator<mozilla::a11y::ProxyAccessible *,mozilla::a11y::DocAccessibleParent *> >+0x8:
	5f12e655 8b3a            mov     edi,dword ptr [edx]  ds:002b:e5e5e5e5=????????


	aChildDoc	0x239eba50 class mozilla::a11y::DocAccessibleParent *
	 mozilla::a11y::ProxyAccessible	class mozilla::a11y::ProxyAccessible
	  mozilla::a11y::ProxyAccessibleBase<mozilla::a11y::ProxyAccessible>	class mozilla::a11y::ProxyAccessibleBase<mozilla::a11y::ProxyAccessible>
	   mParent	0x187ca880 class mozilla::a11y::ProxyAccessible *
		mozilla::a11y::ProxyAccessibleBase<mozilla::a11y::ProxyAccessible>	class mozilla::a11y::ProxyAccessibleBase<mozilla::a11y::ProxyAccessible>
		 mParent	0xe5e5e5e5 class mozilla::a11y::ProxyAccessible *
		 mChildren	class nsTArray<mozilla::a11y::ProxyAccessible *>
		 mDoc	0xe5e5e5e5 class mozilla::a11y::DocAccessibleParent *
		 mWrapper	0xe5e5e5e5
		 mID	0xe5e5e5e5`e5e5e5e5
		 mRole	0n98952677 (No matching enumerant)
		 mOuterDoc	true
		 mIsDoc	false
		 mHasValue	true
		 mIsHyperLink	false
		 mIsHyperText	false
		mCOMProxy	class RefPtr<IAccessible>
	   mChildren	class nsTArray<mozilla::a11y::ProxyAccessible *>
	   mDoc	0x239eba50 class mozilla::a11y::DocAccessibleParent *
	   mWrapper	0
	   mID	0
	   mRole	DOCUMENT (0n15)
	   mOuterDoc	false
	   mIsDoc	true
	   mHasValue	false
	   mIsHyperLink	false
	   mIsHyperText	false
	  mCOMProxy	class RefPtr<IAccessible>
	 mozilla::a11y::PDocAccessibleParent	class mozilla::a11y::PDocAccessibleParent
	 mChildDocs	class nsTArray<mozilla::a11y::DocAccessibleParent *>
	 mParentDoc	0x255e7a90 class mozilla::a11y::DocAccessibleParent *
	 mAccessibles	class nsTHashtable<mozilla::a11y::DocAccessibleParent::ProxyEntry>
	 mTopLevel	false
	 mShutdown	true
	this	0x255e7a90 class mozilla::a11y::DocAccessibleParent *
	 mozilla::a11y::ProxyAccessible	class mozilla::a11y::ProxyAccessible
	 mozilla::a11y::PDocAccessibleParent	class mozilla::a11y::PDocAccessibleParent
	 mChildDocs	class nsTArray<mozilla::a11y::DocAccessibleParent *>
	 mParentDoc	0x00000000 class mozilla::a11y::DocAccessibleParent *
	 mAccessibles	class nsTHashtable<mozilla::a11y::DocAccessibleParent::ProxyEntry>
	 mTopLevel	true
	 mShutdown	true
Group: core-security → dom-core-security
Component: IPC → Disability Access APIs
Keywords: csectype-uaf, sec-high
The test case involves animation, so I'm not sure if that or a11y IPC stuff is to blame here.
We've been having problems with how a11y+e10s handles iframes, so that's the real repro here. I'll try this repro locally and see if it works for me.
Attached file UAF_RemoveChildDoc_Repro.html — 
Flags: sec-bounty?

    
    

    
  
The attached repro works pretty quickly with a debug build on my machine.

I think what I'm seeing here is that hide events are being dispatched in the parent in an unexpected order with respect to shutdown messages.

We still need to be able to work backwards do determine how we got into this state.
Whiteboard: aes+

    
    

    
  
Trevor is back agreed to take a look at this. (Note public bug is likely: bug 1324863)
Assignee: nobody → tbsaunde+mozbugs

    
    

    
  
Anyone working on it?(In reply to Aaron Klotz [:aklotz] from comment #4)
> The attached repro works pretty quickly with a debug build on my machine.
> 
> I think what I'm seeing here is that hide events are being dispatched in the
> parent in an unexpected order with respect to shutdown messages.
> 
> We still need to be able to work backwards do determine how we got into this
> state.

Thanks for confirming it.

Are you or someone else working on it?

    
    

    
  
Trevor is currently assigned to this.

    
    

    
  
Hi Trevor, are you able to reproduce it?
If you need more information, please let me know.
Flags: needinfo?(tbsaunde+mozbugs)

    
    

    
  
I've never been able to reproduce this on linux, I guess I can try on windows, but I suspect debugging there will be much harder.  Can you only reproduce on windows?
Flags: needinfo?(tbsaunde+mozbugs)

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> I've never been able to reproduce this on linux, I guess I can try on
> windows, but I suspect debugging there will be much harder.  Can you only
> reproduce on windows?

Yes I only reproduce it on Windows, I don't have a Linux machine.

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> I've never been able to reproduce this on linux, I guess I can try on
> windows, but I suspect debugging there will be much harder.  Can you only
> reproduce on windows?

If it's not convenient for you to debug it in Windows. Do you need it to be re-assigned to someone else?
Flags: needinfo?(tbsaunde+mozbugs)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix a11y document teardown issues (obsolete)
        — 
          — Splinter Review
      

    


  
Hey Trevor, I think I've got a bead on this so I guess I'll take it.  Its not hard to see whats going on in the patch but there are a few other issues to cover:

---

This patch seems to stop the crashes for me.  I'm starting the Narrarator screen reader built into Windows (on Win10) to
turn on Accessibility in Firefox.  I don't have a touch device yet but I assume its the same failure.  That's the only
step I added to the STR.

---

The loop fix in the patch is obvious.

The code in DocAccessibleParent::RemoveChildDoc [1] seems pretty clear -- it is trying to free aChildDoc from its parent.
Note, however, that the Parent [2] and the ParentDoc [3] are not the same thing, at least in the case of this crash.
It seems from the rest of RemoveChildDoc that this was supposed to be the ParentDoc (which is what mParentDoc refers to),
but I don't know this code enough to say for sure.

---

So, initially, the code crashed almost immediately after turning on a11y due to the RemoveChildDoc bug.  With the patch,
the crashes seemed to stop but, after turning a11y on/off a few times and watching it for a while, then leaving the browser to write this
comment, then returning to the browser (with the narrarator off), I got this ASSERT [4]:

 	xul.dll!mozilla::a11y::ProxyDestroyed(mozilla::a11y::ProxyAccessible * aProxy=0x12f30958) Line 84	C++
 	xul.dll!mozilla::a11y::DocAccessibleParent::Destroy() Line 469	C++
>	xul.dll!mozilla::a11y::DocAccessibleParent::ActorDestroy(mozilla::ipc::IProtocol::ActorDestroyReason aWhy=AbnormalShutdown) Line 93	C++
 	xul.dll!mozilla::a11y::PDocAccessibleParent::DestroySubtree(mozilla::ipc::IProtocol::ActorDestroyReason why=AbnormalShutdown) Line 610	C++
 	xul.dll!mozilla::dom::PBrowserParent::DestroySubtree(mozilla::ipc::IProtocol::ActorDestroyReason why=AbnormalShutdown) Line 5030	C++
 	xul.dll!mozilla::dom::PContentParent::DestroySubtree(mozilla::ipc::IProtocol::ActorDestroyReason why=AbnormalShutdown) Line 8620	C++
 	xul.dll!mozilla::dom::PContentParent::OnChannelError() Line 8584	C++
 	xul.dll!mozilla::dom::ContentParent::OnChannelError() Line 1315	C++
 	xul.dll!mozilla::ipc::MessageChannel::NotifyMaybeChannelError() Line 2217	C++
 	xul.dll!mozilla::ipc::MessageChannel::OnNotifyMaybeChannelError() Line 2246	C++
 	xul.dll!mozilla::detail::RunnableMethodArguments<>::applyImpl<mozilla::ipc::MessageChannel,void (__thiscall mozilla::ipc::MessageChannel::*)(void)>(mozilla::ipc::MessageChannel * o=0x123c90b8, void(mozilla::ipc::MessageChannel::*)() m=0x01def5f2, mozilla::Tuple<> & args={...}, mozilla::IndexSequence<> __formal={...}) Line 826	C++
 	xul.dll!mozilla::detail::RunnableMethodArguments<>::apply<mozilla::ipc::MessageChannel,void (__thiscall mozilla::ipc::MessageChannel::*)(void)>(mozilla::ipc::MessageChannel * o=0x123c90b8, void(mozilla::ipc::MessageChannel::*)() m=0x01def5f2) Line 831	C++
 	xul.dll!mozilla::detail::RunnableMethodImpl<mozilla::ipc::MessageChannel * const,void (__thiscall mozilla::ipc::MessageChannel::*)(void),0,1>::Run() Line 862	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait=false, bool * aResult=0x00e5f015) Line 1240	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread=0x01c62280, bool aMayWait=false) Line 390	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x01c621b0) Line 96	C++
 	xul.dll!MessageLoop::RunInternal() Line 239	C++
 	xul.dll!MessageLoop::RunHandler() Line 232	C++
 	xul.dll!MessageLoop::Run() Line 212	C++
 	xul.dll!nsBaseAppShell::Run() Line 158	C++
 	xul.dll!nsAppShell::Run() Line 262	C++
 	xul.dll!nsAppStartup::Run() Line 283	C++
 	xul.dll!XREMain::XRE_mainRun() Line 4461	C++
 	xul.dll!XREMain::XRE_main(int argc=3, char * * argv=0x01c01030, const mozilla::BootstrapConfig & aConfig={...}) Line 4638	C++
 	xul.dll!XRE_main(int argc=3, char * * argv=0x01c01030, const mozilla::BootstrapConfig & aConfig={...}) Line 4729	C++
 	xul.dll!mozilla::BootstrapImpl::XRE_main(int argc=3, char * * argv=0x01c01030, const mozilla::BootstrapConfig & aConfig={...}) Line 45	C++
 	firefox.exe!do_main(int argc=3, char * * argv=0x01c01030, char * * envp=0x015f30e8) Line 235	C++
 	firefox.exe!NS_internal_main(int argc=3, char * * argv=0x01c01030, char * * envp=0x015f30e8) Line 305	C++
 	firefox.exe!wmain(int argc=3, wchar_t * * argv=0x015eb518) Line 115	C++
 	firefox.exe!__scrt_common_main_seh() Line 253	C++
 	kernel32.dll!@BaseThreadInitThunk@12()	Unknown
 	ntdll.dll!__RtlUserThreadStart()	Unknown
 	ntdll.dll!__RtlUserThreadStart@8()	Unknown

It doesn't look like it would crash a release but I couldnt continue past the ASSERT in the debug build to test that.  The signature is
clearly different than this bug so I'm assuming this is a separate issue.  OTOH, it may be due to the mParent vs mParentDoc issue.
Its worth pointing out that the call stack indicates some exceptional behavior.  But I only have a cursory understanding of this code
at this point.

---

I'm also getting spammed by this message while the page runs with a11y on [5]:

[Parent 1000] ###!!! ASSERTION: why weren't the child docs destroyed already?: 'mChildDocs.IsEmpty()', file c:/mozilla-src/mozilla-unified/accessible/ipc/DocAccessibleParent.cpp, line 450
[Parent 1000] ###!!! ASSERTION: Why do we still have a child doc?: '!mOuterDoc', file c:/mozilla-src/mozilla-unified/accessible/ipc/ProxyAccessibleBase.cpp, line 28

It seems to print it about a half-dozen times on each page reload.
However, the only place other than the location in [5] that would destroy the child doc is the Unbind method [6].  I don't know why it hasn't been called.  Again, this seems to be a different bug.  Previously, it just crashed before it had a chance to spam.

---


[1] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/accessible/ipc/DocAccessibleParent.h#112
[2] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/accessible/ipc/ProxyAccessibleBase.h#93
[3] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/accessible/ipc/DocAccessibleParent.h#99
[4] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/accessible/windows/msaa/Platform.cpp#84
[5] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/accessible/ipc/DocAccessibleParent.cpp#449
[6] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/accessible/ipc/DocAccessibleParent.h#77
Assignee: tbsaunde+mozbugs → davidp99

        Attachment #8830045 -
        Flags: review?(tbsaunde+mozbugs)

    
    

    
  
(In reply to David Parks (dparks) [:handyman] from comment #12)
> Created attachment 8830045 [details] [diff] [review]
> Fix a11y document teardown issues
> 
> Hey Trevor, I think I've got a bead on this so I guess I'll take it.  Its
> not hard to see whats going on in the patch but there are a few other issues
> to cover:
> 
> ---
> 
> This patch seems to stop the crashes for me.  I'm starting the Narrarator
> screen reader built into Windows (on Win10) to
> turn on Accessibility in Firefox.  I don't have a touch device yet but I
> assume its the same failure.  That's the only
> step I added to the STR.

I would expect so, I was seeing similar crashes with nvda, with this test case.


> The loop fix in the patch is obvious.

I'm not actually convinced it is, though I admit relying on unsigned underflow being defined is sneaky.

> The code in DocAccessibleParent::RemoveChildDoc [1] seems pretty clear -- it
> is trying to free aChildDoc from its parent.
> Note, however, that the Parent [2] and the ParentDoc [3] are not the same
> thing, at least in the case of this crash.

I believe they should always be different, I believe we have asserts that check this.

> It seems from the rest of RemoveChildDoc that this was supposed to be the
> ParentDoc (which is what mParentDoc refers to),
> but I don't know this code enough to say for sure.

no, it actually was trying to remove the document from the parent, not the parent document.

> ---
> 
> So, initially, the code crashed almost immediately after turning on a11y due
> to the RemoveChildDoc bug.  With the patch,
> the crashes seemed to stop but, after turning a11y on/off a few times and
> watching it for a while, then leaving the browser to write this
> comment, then returning to the browser (with the narrarator off), I got this

so, given the above I'm not really sure why this patch fixes the crashes.

> ASSERT [4]:
> 
>  	xul.dll!mozilla::a11y::ProxyDestroyed(mozilla::a11y::ProxyAccessible *
> aProxy=0x12f30958) Line 84	C++
>  	xul.dll!mozilla::a11y::DocAccessibleParent::Destroy() Line 469	C++
> >	xul.dll!mozilla::a11y::DocAccessibleParent::ActorDestroy(mozilla::ipc::IProtocol::ActorDestroyReason aWhy=AbnormalShutdown) Line 93	C++

this does seem different, I'm not totally sure if that assert is valid, but for the moment seems safer to leave it in.

> It doesn't look like it would crash a release but I couldnt continue past
> the ASSERT in the debug build to test that.  The signature is
> clearly different than this bug so I'm assuming this is a separate issue. 
> OTOH, it may be due to the mParent vs mParentDoc issue.
> Its worth pointing out that the call stack indicates some exceptional
> behavior.  But I only have a cursory understanding of this code
> at this point.

the only thing I find odd in a quick look is that the shutdown reason for the actor is abnormal shutdown.

> 
> I'm also getting spammed by this message while the page runs with a11y on
> [5]:
> 
> [Parent 1000] ###!!! ASSERTION: why weren't the child docs destroyed
> already?: 'mChildDocs.IsEmpty()', file
> c:/mozilla-src/mozilla-unified/accessible/ipc/DocAccessibleParent.cpp, line
> 450

I haven't seen that one, but I'm not really sure its valid, though it would be nice if it was.

> [Parent 1000] ###!!! ASSERTION: Why do we still have a child doc?:
> '!mOuterDoc', file
> c:/mozilla-src/mozilla-unified/accessible/ipc/ProxyAccessibleBase.cpp, line
> 28

yeah, that's known, I believe things work fine after that happens, but it would be nice to see if we can fix it some day.
Flags: needinfo?(tbsaunde+mozbugs)

    
    

    
  
OK, it was worth a shot.  I'm returning the bug to you.
Assignee: davidp99 → tbsaunde+mozbugs

    
    

    
  
Copy info from bug duped to this.  Affects 52, so this should be a high priority.
Crash Signature: https://crash-stats.mozilla.com/signature/?signature=nsTArray_Impl%3CT%3E%3A%3AIndexOf%3CT%3E%20%7C%20mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%3A%3ARemoveChildDoc
https://crash- stats.mozilla.com/signature/?signature=mozilla%3A%3Aa11y%3A%3ADocAccessibl…

          status-firefox52:
          --- → affected
Keywords: topcrash-win
Crash Signature: https://crash-stats.mozilla.com/signature/?signature=nsTArray_Impl%3CT%3E%3A%3AIndexOf%3CT%3E%20%7C%20mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%3A%3ARemoveChildDoc
https://crash- stats.mozilla.com/signature/?signature=mozilla%3A%3Aa11y%3A%3ADocAccessibl… → [@ mozilla::a11y::DocAccessibleParent::RemoveChildDoc]
[@nsTArray_Impl<T>::IndexOf<T> | mozilla::a11y::DocAccessibleParent::RemoveChildDoc]
Crash Signature: [@ mozilla::a11y::DocAccessibleParent::RemoveChildDoc]
[@nsTArray_Impl<T>::IndexOf<T> | mozilla::a11y::DocAccessibleParent::RemoveChildDoc] → [@ mozilla::a11y::DocAccessibleParent::RemoveChildDoc]
[@nsTArray_Impl<T>::IndexOf<T> | mozilla::a11y::DocAccessibleParent::RemoveChildDoc]
[@ mozilla::a11y::ProxyAccessibleBase<T>::ClearChildDoc]
[@ nsTArray_Impl<T>::IndexOf<T> | mozilla::a11y::ProxyA…

    
    

    
  
(In reply to David Parks (dparks) [:handyman] from comment #12)

> [Parent 1000] ###!!! ASSERTION: why weren't the child docs destroyed
> already?: 'mChildDocs.IsEmpty()', file
> c:/mozilla-src/mozilla-unified/accessible/ipc/DocAccessibleParent.cpp, line
> 450
> [Parent 1000] ###!!! ASSERTION: Why do we still have a child doc?:
> '!mOuterDoc', file
> c:/mozilla-src/mozilla-unified/accessible/ipc/ProxyAccessibleBase.cpp, line
> 28

Aaron, Trevor do we know why this happens?
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(aklotz)

    
    

    
  
In the last 7 days, these signatures have combined for over 3,353 crashes on nightly.

    
    

    
  


  
Since it can be called in strange orderings wrt SetChildDoc() it seems
reasonable to believe we can get an ordering where the proxy has a document as
a child but is not marked as having document children.

I'm not completely sure, but I think this fixes the test case locally.

        Attachment #8832315 -
        Flags: review?(aklotz)

        Attachment #8832315 -
        Flags: review?(aklotz) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c2aee4595f1a
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox54:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Group: dom-core-security → core-security-release

    
    

    
  
This high-severity security bug was checked in without sec-approval
https://wiki.mozilla.org/Security/Bug_Approval_Process

Please retroactively request approval and answer the questions that are added to the comment so we can evaluate what other branches will need this fix and how much risk is associated with that.

          status-firefox-esr45:
          --- → unaffected

          status-firefox-esr52:
          --- → affected
Flags: needinfo?(dbolter)

    
    

    
  
(In reply to Daniel Veditz [:dveditz] from comment #22)
> This high-severity security bug was checked in without sec-approval
> https://wiki.mozilla.org/Security/Bug_Approval_Process

shit sorry.  Though in my defense I don't know if this actually fixes it, and if it does I don't think the patch leaks much information.  I certainly have no idea how to trigger it.
Flags: needinfo?(tbsaunde+mozbugs)

    
    

    
  
Comment on attachment 8832315 [details] [diff] [review]
ClearChildDoc() should leave the proxy marked as being an outerdoc

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
supposing that the patch does actually fix the bug which is unclear I don't think the patch tells you much about how to trigger it.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
no
Which older supported branches are affected by this flaw?
52 and 53
If not all supported branches, which bug introduced the flaw?
unclear but this is only an issue when a11y is in use with e10s enabled which is 52 and later.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
should be pretty easy.
How likely is this patch to cause regressions; how much testing does it need?

we need some testing on nightly to see if it does infact fix the issue.  I don't think it can regress much, but I wouldn't backport it if it doesn't fix the problem.

        Attachment #8832315 -
        Flags: sec-approval?

    
    

    
  
We're currently targeting FF 54 for e10s a11y support so this configuration (and bug) should be disabled in <= 53. I do see crashes in recent 53 builds though -- Jimm is this expected?
Flags: needinfo?(dbolter) → needinfo?(jmathies)

    
    

    
  
78% of those crashes have "addons should have disabled e10s" set to true. They must be force-enabled, so I'd say it's expected.
Flags: needinfo?(jmathies)
Flags: needinfo?(aklotz)

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

    
  
Depends on: 1336093

    
    

    
  
backout out because of start up crashes

    
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
  
Blocks: 1336770

    
    

    
  
For the last few days I seem to hit this nearly once an hour on my Surface Book Pro using Microsoft Japanese IME with e10s turned on.

    
    

    
  
Some crash comments:
"closed a tab"
"every time if I close a Facebook tab, Nightly make a crash"
"basically every time i use ctrl+w"
"crash when closing a tab"
"closed google docs tab"

    
    

    
  


  
updated version of last patch

        Attachment #8835060 -
        Flags: review?(yzenevich)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1337370 patch with test (obsolete)
        — 
          — Splinter Review
      

    


  
Trevor, note that the latest patch still seems to produce the startup crash

        Attachment #8835714 -
        Flags: review?(tbsaunde+mozbugs)

    
    

    
  
(In reply to Yura Zenevich [:yzen] from comment #33)
> Created attachment 8835714 [details] [diff] [review]
> 1337370 patch with test
> 
> Trevor, note that the latest patch still seems to produce the startup crash

also fyi: the test passes as is on trunk

    
    

    
  
Comment on attachment 8835714 [details] [diff] [review]
1337370 patch with test

I'm not going to pretend I understand this, but its a test so if it works fine.

        Attachment #8835714 -
        Flags: review?(tbsaunde+mozbugs) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1337370 patch with test v2
        — 
          — Splinter Review
      

    


  
Things changed:
* instead of wait of executeSoon, after content a11y doc is loaded, waiting for 3000 as I found that sometimes executeSoon is not enough to see the crash.
* RyanVM: as per our conversation, updated the browser.ini to reflect what's going to happen in bug 1336712

        Attachment #8835714 -
        Attachment is obsolete: true

        Attachment #8835999 -
        Flags: review+

        Attachment #8835999 -
        Flags: feedback?(ryanvm)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/11b11c798427 landed and has this bug number in it - is this the right patch for this bug or maybe a wrong bug number
Flags: needinfo?(tbsaunde+mozbugs)

        Attachment #8835999 -
        Flags: feedback?(ryanvm) → feedback+

    
    

    
  
(In reply to Carsten Book [:Tomcat] from comment #37)
> https://hg.mozilla.org/mozilla-central/rev/11b11c798427 landed and has this
> bug number in it - is this the right patch for this bug or maybe a wrong bug
> number

its the right bug.
Flags: needinfo?(tbsaunde+mozbugs)

    
    

    
  
Comment on attachment 8835060 [details] [diff] [review]
rework ProxyAccessible handling of outer docs

I thought this was reviewed, but whatever I'll just carry forward review from the first patch.

        Attachment #8835060 -
        Flags: review?(yzenevich)

    
    

    
  
Are there any specific requirements regarding landing, or pushing to try the test for this patch?
Flags: needinfo?(abillings)

    
    

    
  
Isn't this checked in now, per comment 28?

We don't land tests for security bugs until after the release with the fix for the issue has gone public.
Flags: needinfo?(abillings)

    
    

    
  
(In reply to Al Billings [:abillings] from comment #41)
> Isn't this checked in now, per comment 28?

I relanded what bounced the first time, but I don't think we know for certain if that fixes the issue yet.
I haven't done anything with the test.

    
    

    
  
The signature this was originally filed against appears fixed, others were not. I'll close this out and make sure we have bugs filed on the other signatures.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED

    
    

    
  
The signature "nsTArray_Impl<T>::IndexOf<T> | mozilla::a11y::DocAccessibleParent::RemoveChildDoc" just reappeared on nightly 54 with buildid 20170216030210: there are 12 crashes.
Status: RESOLVED → REOPENED

          status-firefox54:
          fixed → affected
Resolution: FIXED → ---

    
    

    
  
Trevor would have best answers here. I do recall him saying he's not sure his fix would fix all STR cases.
Flags: needinfo?(dbolter) → needinfo?(tbsaunde+mozbugs)

    
  
Depends on: 1340579

    
    

    
  
I'm not able to crash (on windows touchscreen) via the test case. (I saved it as html and loaded locally)

    
    

    
  
Comment on attachment 8830045 [details] [diff] [review]
Fix a11y document teardown issues

Removing old review request per Trevor.

        Attachment #8830045 -
        Attachment is obsolete: true

        Attachment #8830045 -
        Flags: review?(tbsaunde+mozbugs)

    
  
Assignee: tbsaunde+mozbugs → davidp99
Flags: needinfo?(tbsaunde+mozbugs)

    
    

    
  
David Parks is working on a public bug 1332690 we hope will fix this.

    
    

    
  
firefox53 is marked as disabled but the [@ mozilla::a11y::DocAccessibleParent::RemoveChildDoc ] signature is the #6 top brower crash on 53.0b6 accounting for 1% of all crashes.

    
    

    
  
dbolter - can you make sure we get that other fix into 53, or actually disable?  bug 1332690 is marked as 53-disabled as well, but clearly that's not the case.
Flags: needinfo?(dbolter)
Flags: needinfo?(davidp99)

    
    

    
  
Suspected fixes are bug 1332690 and bug 1340579 - I've requested authors to request uplifts.
Flags: needinfo?(dbolter)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Maintain mChildDocs when swapping child docs (obsolete)
        — 
          — Splinter Review
      

    


  
This one is pretty straight-forward.  When we 'swap' child docs in the tree (instead of the 'usual' creating them), we were properly updating mChildren, mParent and mParentDoc but not mChildDocs.
Flags: needinfo?(davidp99)

        Attachment #8853166 -
        Flags: review?(tbsaunde+mozbugs)

    
    

    
  
Re-instituting NI to self to remember uplift
Flags: needinfo?(davidp99)

    
    

    
  
(In reply to David Parks (dparks) [:handyman] from comment #54)
> Created attachment 8853166 [details] [diff] [review]
> Maintain mChildDocs when swapping child docs
> 
> This one is pretty straight-forward.  When we 'swap' child docs in the tree
> (instead of the 'usual' creating them), we were properly updating mChildren,
> mParent and mParentDoc but not mChildDocs.

yeah... that seems reasonable, and I really should have seen it :(

On the subject of the patch I think the best thing to do is call outerDoc->ChildAt(0)->AsDoc()->Unbind() which should also fix this issue and allow us to remove the stuff in ProxyAccessiblebase::SetChildDoc() dealing with replacing one document with another.

That is what happens in this case  either way is we start with a new doc N and an existing bound doc B, and end with B being out of the tree and N bound where B used to be.  The simplest way to implement that is unbind both B and N, and then bind N where it belongs.

On the other hand maybe we should take the patch more or less as is for branches? it might be slightly less risk? but maybe that doesn't realy matter.

    
    

    
  
Comment on attachment 8853166 [details] [diff] [review]
Maintain mChildDocs when swapping child docs

dparks your going to post an updated patch right?

        Attachment #8853166 -
        Flags: review?(tbsaunde+mozbugs)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Maintain mChildDocs when swapping child docs (obsolete)
        — 
          — Splinter Review
      

    


  
Includes notes from comment 56.

        Attachment #8853166 -
        Attachment is obsolete: true
Flags: needinfo?(davidp99)

        Attachment #8856596 -
        Flags: review?(tbsaunde+mozbugs)

        Attachment #8856596 -
        Flags: review?(tbsaunde+mozbugs) → review+

    
    

    
  
Comment on attachment 8856596 [details] [diff] [review]
Maintain mChildDocs when swapping child docs

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

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

Which older supported branches are affected by this flaw?
55 and 54 are affected.  Requires e10s + a11y.

If not all supported branches, which bug introduced the flaw?
Unknown but a11y is disabled when e10s is on in prior releases.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Any backporting should be very simple.

How likely is this patch to cause regressions; how much testing does it need?
Regressions are unlikely.  At worst they would manifest simply as a crash in a different spot.

        Attachment #8856596 -
        Flags: sec-approval?

    
    

    
  
(In reply to David Parks (dparks) [:handyman] from comment #60)
> Which older supported branches are affected by this flaw?
> 55 and 54 are affected.  Requires e10s + a11y.

What about comment 51? That makes it sound like 53 is affected too. Or is it two bugs with the same signature?
Flags: needinfo?(davidp99)

    
    

    
  
it seems the [@ mozilla::a11y::DocAccessibleParent::RemoveChildDoc ] crashes on 53.0b have significantly declined starting with beta 9 at least: https://crash-stats.mozilla.com/signature/?release_channel=beta&signature=mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%3A%3ARemoveChildDoc&date=%3E%3D2017-03-11T22%3A13%3A00.000Z#graphs

    
    

    
  
The top crash signature in this bug picks up crashes we are handling as a different bug:

https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%3A%3ARemoveChildDoc

is bug 1339168.  The difference is the second entry in the call stack -- the nsTArray (or ClearChildDoc) crash is handled by this bug and DocAccessibleParent::Destroy is the other.

More to the point, I believe 53 is supposed to have a11y disabled when e10s is on so it would be irrelevant.  But I'm not sure because, while _most_ of the crashes show AddonsShouldHaveBlockedE10S to be true, there is one that doesnt...

https://crash-stats.mozilla.com/report/index/f41d1acd-2c79-4ae8-8183-3f6022170408#tab-metadata

So I'm not sure what's up with 53.  If a11y+e10s is turned on there then maybe 53 needs this too.  NI to jimm for clarification.
Flags: needinfo?(davidp99) → needinfo?(jmathies)

    
    

    
  
(In reply to David Parks (dparks) [:handyman] from comment #60)
> Comment on attachment 8856596 [details] [diff] [review]
> Maintain mChildDocs when swapping child docs
> 
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Not easily.

Is that actually true?  Once you know that what's happening is BindChildDoc() is leaving mChildDocs containing pointers to objects that can die you just need to create a page where you start out with a document as a child of an iframe and then move it to be the child of something else before getting rid of it.  Then later get rid of the iframe and from there its a pretty standard vtable ptr overwriting exploit.

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

above?

> Which older supported branches are affected by this flaw?
> 55 and 54 are affected.  Requires e10s + a11y.

we may not be seeing it, but I'm pretty sure 53 is effected.

> If not all supported branches, which bug introduced the flaw?
> Unknown but a11y is disabled when e10s is on in prior releases.

pretty sure its always been there.  I think we have e10s on in linux and maybe mac? those are effected we just haven't seen it there for some reason.

    
    

    
  
(In reply to David Parks (dparks) [:handyman] from comment #63)
> The top crash signature in this bug picks up crashes we are handling as a
> different bug:
> 
> https://crash-stats.mozilla.com/signature/
> ?signature=mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%3A%3ARemoveChildDoc
> 
> is bug 1339168.  The difference is the second entry in the call stack -- the
> nsTArray (or ClearChildDoc) crash is handled by this bug and
> DocAccessibleParent::Destroy is the other.
> 
> More to the point, I believe 53 is supposed to have a11y disabled when e10s
> is on so it would be irrelevant.  But I'm not sure because, while _most_ of
> the crashes show AddonsShouldHaveBlockedE10S to be true, there is one that
> doesnt...
> 
> https://crash-stats.mozilla.com/report/index/f41d1acd-2c79-4ae8-8183-
> 3f6022170408#tab-metadata
> 
> So I'm not sure what's up with 53.  If a11y+e10s is turned on there then
> maybe 53 needs this too.  NI to jimm for clarification.

Hey David, which signature set are you targeting with this fix, the ClearChildDoc signatures or the RemoveChildDoc/Destroy signatures?
Flags: needinfo?(davidp99)

    
    

    
  
Daniel, my socorro access is current hosed so I can't see sec info in crash reports. I'm having a hard time understanding which crashes that fall under the list of four signatures tracked here represent the use after free security issue this bug is tracking. We have this bug and we have bug 1339168 which tracks one of the signatures tracked here - 

[@ mozilla::a11y::DocAccessibleParent::RemoveChildDoc ]

I'm wondering if that bug is a pure dupe of this sec bug and should be duped over (or ignored).
Flags: needinfo?(dveditz)

    
    

    
  
History

Original stack reported here:
-------------------------------------------------------------
xul!mozilla::a11y::DocAccessibleParent::RemoveChildDoc+0x22
xul!mozilla::a11y::DocAccessibleParent::Destroy+0xa6
xul!mozilla::a11y::DocAccessibleParent::Destroy+0x38
xul!mozilla::a11y::DocAccessibleParent::RecvShutdown+0xd
xul!mozilla::a11y::PDocAccessibleParent::OnMessageReceived+0x57
xul!mozilla::dom::PContentParent::OnMessageReceived+0x4b

Landings in sec bug 1326084 (this bug)
-------------------------------------------------------------
2017-02-01 c2aee4595f1a (in Fx54)
2017-02-02 a100bc4eab44
2017-02-03 backout of a100bc4eab44 due to startup crashes
2017-02-10 11b11c798427 (in Fx54)
--
2017-03-30 new patch from dparks posted, hasn't landed yet, needs sec approval

Landings in other bugs that may impact this:
--------------------------------------------------------------
2017-03-01 5c809b2f8930 (bug 1340579) (in Fx54)
2017-03-29 3c911506e8c7 (bug 1332690) (in Fx55, Fx54, Fx53)
 uplifts: 2017-04-02 47ae4627fe0d (Fx 54), 00212876d4c4, ee66bb1f3ff0 (Fx 53)

    
    

    
  
Signature summary across builds:


Fx52 release, Fx 52esr, Fx53 betas, Fx54 Nightly:
[@ mozilla::a11y::DocAccessibleParent::RemoveChildDoc ]

Fx52 release, Fx53 betas, Fx54 Nightly:
[@ nsTArray_Impl<T>::IndexOf<T> | mozilla::a11y::DocAccessibleParent::RemoveChildDoc ]

Fx54 Nightly:
[@ mozilla::a11y::ProxyAccessibleBase<T>::ClearChildDoc ]

No signatures:
[@ nsTArray_Impl<T>::IndexOf<T> | mozilla::a11y::ProxyAccessibleBase<T>::ClearChildDoc ]

    
    

    
  
(In reply to Jim Mathies [:jimm] from comment #65)
> Hey David, which signature set are you targeting with this fix, the
> ClearChildDoc signatures or the RemoveChildDoc/Destroy signatures?

Its the ClearChildDoc signatures.  Note that the ones with IndexOf<T> are also this bug -- looking at source, I suspect the IndexOf call is inlined from ClearChildDoc.  So only the first signature ([@ mozilla::a11y::DocAccessibleParent::RemoveChildDoc ]) included any signatures that were not dealt with in this bug.
Flags: needinfo?(davidp99)

    
  
Flags: needinfo?(jmathies)

    
    

    
  
(In reply to Jim Mathies [:jimm] from comment #66)
> Daniel, my socorro access is current hosed so I can't see sec info in crash reports.
> I'm having a hard time understanding which crashes that fall under the list of four
> signatures tracked here represent the use after free security issue this bug is tracking.

You don't need extended socorro access to see the signs of use-after-free. Won't spot _all_ UAF, and certainly not a successfully exploited one, but the ones flagged by crash-stats mining have been done from the public information. The easiest to spot is to show the "address" column in crash reports:

https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%3A%3ARemoveChildDoc&date=%3E%3D2017-03-31T01%3A38%3A24.000Z&date=%3C2017-04-14T01%3A38%3A24.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1

An address of 0xffffffffe5e5e5xx is a reliable indicator since we use 0xe5 as a freed memory marker. Sometimes the crashing address will be something else but if you look at the "raw dump" tab of a crash you'll see 0xe5e5e5e5 values in the registers. That's a clue but you have to look at individual crashes to see that. I don't think you can search on it.

If Firefox crashes on a random valid-looking memory address (like bp-624a521e-3371-4095-ad94-49b012170401) that's some kind of memory corruption. Might be a use-after-free where the memory got re-used before we stumbled over the poisoned value, could be some other kind of memory corruption like a buffer overflow. Hard to say whether it's related or not.


> We have this bug and we have bug 1339168 which tracks one of the signatures tracked here - 
> I'm wondering if that bug is a pure dupe of this sec bug and should be duped
> over (or ignored).

And also originally bug 1324863 was duped here.

We've checked in some stuff here and seem to have reduced the number of crashes. Even if bug 1339168 is part of the same set of issues it's probably better to continue on over there since this bug is getting long and confusing.
Flags: needinfo?(dveditz)

    
    

    
  
dparks, lets post your 'Maintain mChildDocs when swapping child docs' w/trevors r+ patch over to bug 1339168 and land it. We'll keep this bug open so we can dig into these signatures looking for the telltale signs of a sec issue per Daniel's comments.
Flags: needinfo?(davidp99)

    
  

        Attachment #8856596 -
        Flags: sec-approval?

    
    

    
  
Comment on attachment 8856596 [details] [diff] [review]
Maintain mChildDocs when swapping child docs

Obsoleting this patch because we are landing it on bug 1339168.

        Attachment #8856596 -
        Attachment is obsolete: true
Flags: needinfo?(davidp99)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0f60f50ae0f4
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago

          status-firefox55:
          --- → fixed
Resolution: --- → FIXED

    
    

    
      
  
Flags: sec-bounty? → sec-bounty+

    
    

    
      
  
Flags: qe-verify+
Whiteboard: aes+ → [post-critsmash-triage] aes+

    
    

    
  
I have reproduced this issue using Firefox debug build (32 bit) from 2017.01.03, on Win 10 x32, I'm starting the Narrarator screen reader built into Windows, Firefox crashed, but the crash report was not generated, I just saw that the tab title was flickering,loading non-stop.
I can confirm this issue is fixed, I verified using Firefox 55.0 debug build, on Windows 10 x32, seems that Firefox crashes stoped, and the tab title no longer flickering.
Status: RESOLVED → VERIFIED

          status-firefox55:
          fixed → verified
Flags: qe-verify+
Group: core-security-release

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: