1864015 - Poison crash in [@ mozilla::a11y::Accessible::AsRemote] with FSDomNodeFirefoxMP.DLL

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/ff2449f5-9543-4abe-9bc2-302360231104

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  mozilla::a11y::Accessible::AsRemote  accessible/ipc/RemoteAccessible.h:505
0  xul.dll  mozilla::a11y::MsaaAccessible::GetFrom  accessible/windows/msaa/MsaaAccessible.cpp:479
0  xul.dll  mozilla::a11y::MsaaAccessible::NativeAccessible  accessible/windows/msaa/MsaaAccessible.cpp:452
1  xul.dll  mozilla::a11y::ia2AccessibleRelation::ia2AccessibleRelation  accessible/windows/ia2/ia2AccessibleRelation.cpp:21
2  xul.dll  mozilla::a11y::ia2Accessible::get_relations  accessible/windows/ia2/ia2Accessible.cpp:140
3  FSDomNodeFirefoxMP.DLL  FSDomNodeFirefoxMP.DLL@0x25da3  
4  FSDomNodeFirefoxMP.DLL  FSDomNodeFirefoxMP.DLL@0x25dfc  
5  FSDomNodeFirefoxMP.DLL  FSDomNodeFirefoxMP.DLL@0x21bdb  
6  FSDomNodeFirefoxMP.DLL  FSDomNodeFirefoxMP.DLL@0x21b97  
7  FSDomNodeFirefoxMP.DLL  FSDomNodeFirefoxMP.DLL@0x1ab96  

This signature is a bit generic, but the crashes I looked at mostly had FSDomNodeFirefoxMP.DLL in the stack. Maybe there's some kind of callback involving a stale pointer?

I noticed a lot of DropBox URLs with these crashes, if that means anything.

Comment 1

[Maire Reavy [:mreavy]]

Jamie, Frank - For some reason none of us had permission to view this bug when it was reported. I just got access, so I'm adding you both.

Comment 2

[James Teh [:Jamie]]

Thanks Maire. Weird that we didn't have access. The triage owner on our team now gets rotated weekly, so that might explain why I didn't have access, but it doesn't explain why you or Frank didn't.

Investigation note: I'm suspicious of AccGroupInfo::ConceptualParent. That gets invalidated by reorder events; i.e. when any siblings change. However, I'm wondering whether a client call could be received between the IPDL call where the conceptual parent (really a sibling) is removed and the IPDL call where the group info is invalidated. In that case, we'd be holding onto a dead RemoteAccessible.

Comment 3

[James Teh [:Jamie]]

Attachment: Bug 1864015: Correctly handle removal of the conceptual parent.

Comment 4

[Andrew McCreight [:mccr8]]

Sorry, I thought a11y was under the DOM security group, but looking now it is actually layout. I'll try to remember that.

Comment 5

[Andrew McCreight [:mccr8]]

I did a search just now and it looks like all security bugs in Disability Access APIs are now either release-track or in layout-core-security.

Comment 6

[James Teh [:Jamie]]

Comment on attachment 9364879 [details]
Bug 1864015: Correctly handle removal of the conceptual parent.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch switches from a raw pointer to an id, which could hint at a possible UAF. However, i think it would be difficult to get the impacted memory address from within the content process.
• 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?: all
• 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?: This patch should apply cleanly or with minimal modification on all supported older branches.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. The underlying functionality is covered by automated tests.
• Is Android affected?: No

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 9364879 [details]
Bug 1864015: Correctly handle removal of the conceptual parent.

Approved to land and uplift

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02c2a340c117
Correctly handle removal of the conceptual parent. r=eeejay

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/02c2a340c117

Comment 10

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

The patch landed in nightly and beta is affected.
:Jamie, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox121 to wontfix.

For more information, please visit BugBot documentation.

Comment 11

[James Teh [:Jamie]]

Attachment: Bug 1864015: Correctly handle removal of the conceptual parent.

Original Revision: https://phabricator.services.mozilla.com/D194322

Comment 12

[Phabricator Automation]

Uplift Approval Request

• User impact if declined: Crash. UAF.
• Steps to reproduce for manual QE testing: NA
• Needs manual QE test: no
• Explanation of risk level: Isolated to accessibility group position calculations. Just changes a pointer to use an existing id. Underlying code covered by automated tests.
• Is Android affected?: no
• Code covered by automated testing: yes
• String changes made/needed: none
• Fix verified in Nightly: no
• Risk associated with taking this patch: low

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9366762 [details]
Bug 1864015: Correctly handle removal of the conceptual parent.

Approved for 121.0b8 and 115.6esr.

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/79cedc5de788

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/868794b9f63e

Comment 17

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter