1824828 - Use-after-free crash in [@ nsTArray_base<T>::Length | nsTArray_Impl<T>::IndexOf | nsTArray_Impl<T>::RemoveElement<T> | nsTArray_Impl<T>::RemoveElement | mozilla::a11y::RemoteAccessibleBase<T>::RemoveChild] from RecvHideEvent

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/908d4b9a-889b-40c9-adef-ea3540230316

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_RelocateUsingMemutils>::Length const  xpcom/ds/nsTArray.h:410
0  xul.dll  nsTArray_Impl<mozilla::net::Http2PushedStream*, nsTArrayInfallibleAllocator>::IndexOf const  xpcom/ds/nsTArray.h:1343
0  xul.dll  nsTArray_Impl<mozilla::net::Http2PushedStream*, nsTArrayInfallibleAllocator>::RemoveElement<mozilla::net::Http2StreamBase*, nsDefaultComparator<mozilla::net::Http2PushedStream*, mozilla::net::Http2StreamBase*> >  xpcom/ds/nsTArray.h:1953
1  xul.dll  nsTArray_Impl<mozilla::a11y::RemoteAccessible*, nsTArrayInfallibleAllocator>::RemoveElement  xpcom/ds/nsTArray.h:1966
1  xul.dll  mozilla::a11y::RemoteAccessibleBase<mozilla::a11y::RemoteAccessible>::RemoveChild  accessible/ipc/RemoteAccessibleBase.h:125
2  xul.dll  mozilla::a11y::DocAccessibleParent::RecvHideEvent  accessible/ipc/DocAccessibleParent.cpp:315
3  xul.dll  mozilla::a11y::PDocAccessibleParent::OnMessageReceived  ipc/ipdl/PDocAccessibleParent.cpp:726
4  xul.dll  mozilla::dom::PContentParent::OnMessageReceived  ipc/ipdl/PContentParent.cpp:6725
5  xul.dll  mozilla::ipc::MessageChannel::DispatchAsyncMessage  ipc/glue/MessageChannel.cpp:1800
5  xul.dll  mozilla::ipc::MessageChannel::DispatchMessage  ipc/glue/MessageChannel.cpp:1725

There are a decent number of these crashes on poison values. It looks like they all have DocAccessibleParent::RecvHideEvent in the stack. It doesn't look like the other recent UAFs on file that I saw, but maybe I missed one.

Comment 1

[James Teh [:Jamie]]

RecvHideEvent doesn't make any COM calls itself and nor does anything it calls, at least not directly. However, in-process clients might make COM calls synchronously inside NotifyWinEvent when handling the event. Perhaps a client tried to retrieve an IAccessible from Firefox, which would result in outgoing calls and potential IPDL reentry.

Comment 2

[James Teh [:Jamie]]

Attachment: Bug 1824828: Handle tree mutation during RecvHideEvent.

Comment 3

[James Teh [:Jamie]]

Comment on attachment 9325603 [details]
Bug 1824828: Handle tree mutation during RecvHideEvent.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The addition of a WeakPtr strongly suggests UAF mitigation. However, I think it would be difficult to exploit this given its rarity and the difficulty of coordinating this race between processes.
• 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?: beta, ESR 102
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should apply cleanly or be easy to create.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely; just checking that an object is still alive.
• Is Android affected?: No

Comment 4

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9325603 [details]
Bug 1824828: Handle tree mutation during RecvHideEvent.

Today is the last day of uplifts, but I am okay with this landing if relman can uplift it. If they can't uplift it this cycle, let's land it next cycle.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f9e26dfd470

Please nominate this for Beta and ESR approval.

Comment 6

[James Teh [:Jamie]]

Comment on attachment 9325603 [details]
Bug 1824828: Handle tree mutation during RecvHideEvent.

Beta/Release Uplift Approval Request

• User impact if declined: Crash, UAF.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just adds a check and early return to make sure an object is alive.
• String changes made/needed:
• Is Android affected?: No

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Crash, UAF.
• Fix Landed on Version: 113
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just adds a check and early return to make sure an object is alive.

Comment 7

[Dianna Smith [:diannaS]]

Comment on attachment 9325603 [details]
Bug 1824828: Handle tree mutation during RecvHideEvent.

Approved for 112.0rc1 and 102.10esr

Comment 8

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3aea9ee1100b

Comment 9

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/a2f6815be2be