1821448 - Crash in [@ nsTArray_base<T>::Length | nsTArray_Impl<T>::ElementAt | VisitDocAccessibleParentDescendantsAtTopLevelInContentProcess<T> ]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/f98b8116-94a4-4e2c-8fe7-065180230308

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::dom::PBrowserBridgeParent*, nsTArrayInfallibleAllocator>::ElementAt const  xpcom/ds/nsTArray.h:1204
0  xul.dll  mozilla::ArrayIterator<mozilla::dom::PBrowserBridgeParent*const&, nsTArray_Impl<mozilla::dom::PBrowserBridgeParent*, nsTArrayInfallibleAllocator> >::operator* const  xpcom/ds/ArrayIterator.h:107
0  xul.dll  std::all_of  /builds/worker/fetches/vs/VC/Tools/MSVC/14.16.27023/include/algorithm:500
0  xul.dll  VisitDocAccessibleParentDescendantsAtTopLevelInContentProcess<`lambda at /builds/worker/checkouts/gecko/accessible/windows/msaa/MsaaAccessible.cpp:725:21'>  accessible/windows/msaa/MsaaAccessible.cpp:678
1  xul.dll  mozilla::a11y::MsaaAccessible::GetRemoteIAccessibleFor  accessible/windows/msaa/MsaaAccessible.cpp:743
2  xul.dll  mozilla::a11y::MsaaAccessible::GetIAccessibleFor  accessible/windows/msaa/MsaaAccessible.cpp:550
3  xul.dll  mozilla::a11y::MsaaAccessible::get_accChild  accessible/windows/msaa/MsaaAccessible.cpp:970
4  xul.dll  mozilla::a11y::LazyInstantiator::get_accChild  accessible/windows/msaa/LazyInstantiator.cpp:563
5  rpcrt4.dll  Invoke  

There are a few crashes that look like this, on jemalloc poison values, indicating a use after free. I don't know if it is a duplicate of an existing issue. My guess is that this is caused by ManagedPBrowserBridgeParent() being modified during the iteration.

Bug 1739372 is a prior issue with a UAF involving VisitDocAccessibleParentDescendantsAtTopLevelInContentProcess, so maybe this is similar.

Here are a few more crashes like this:
bp-a70187ed-9884-4f5c-aa13-4a80d0230308
bp-d62e5269-cf74-4f06-a03e-98c9a0230306

Comment 1

[Andrew McCreight [:mccr8]]

This looks similar, except we're hitting a release assert for some reason: bp-b14a57ea-5cc6-40ef-bb4a-035d50230307

I'll file a bug about adding ArrayIterator to the prefix list so these signatures are less generic.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:Jamie, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 3

[James Teh [:Jamie]]

My bad, I missed the fact that this was a sec-high.

Comment 4

[James Teh [:Jamie]]

Attachment: Bug 1821448: Improve handling of reentry in MsaaAccessible::VisitDocAccessibleParentDescendantsAtTopLevelInContentProcess.

Comment 5

[Andrew McCreight [:mccr8]]

This signature looks similar, and there are also UAFs under it.

Comment 6

[Andrew McCreight [:mccr8]]

Updating with a new signature after bug 1821453.

Comment 7

[Andrew McCreight [:mccr8]]

I'll make that the signature in the summary because it is more useful.

Comment 8

[James Teh [:Jamie]]

Comment on attachment 9325096 [details]
Bug 1821448: Improve handling of reentry in MsaaAccessible::VisitDocAccessibleParentDescendantsAtTopLevelInContentProcess.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unfortunately, the addition of RefPtr in a few places strongly hints at us fixing a UAF. That said, this is likely difficult to exploit due to its rarity and coordinating a race between two or three 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?: ESR 102, beta
• 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 very easy to create.
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely to cause regressions; just an array copy and holding temporary references.
• Is Android affected?: No

Comment 9

[Tom Ritter [:tjr] (OOTO until 7/21)]

Comment on attachment 9325096 [details]
Bug 1821448: Improve handling of reentry in MsaaAccessible::VisitDocAccessibleParentDescendantsAtTopLevelInContentProcess.

Approved to request uplift and land

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f9785025e232

Go ahead and request Beta & ESR approval on this whenever you're comfortable doing so.

Comment 11

[James Teh [:Jamie]]

Comment on attachment 9325096 [details]
Bug 1821448: Improve handling of reentry in MsaaAccessible::VisitDocAccessibleParentDescendantsAtTopLevelInContentProcess.

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 an array copy and holding temporary references.
• 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 an array copy and holding temporary references.

Comment 12

[Dianna Smith [:diannaS]]

Comment on attachment 9325096 [details]
Bug 1821448: Improve handling of reentry in MsaaAccessible::VisitDocAccessibleParentDescendantsAtTopLevelInContentProcess.

Approved for 112.0b9

Comment 13

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c6e79e82e3bb

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9325096 [details]
Bug 1821448: Improve handling of reentry in MsaaAccessible::VisitDocAccessibleParentDescendantsAtTopLevelInContentProcess.

Approved for 102.10esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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