1977166 - Potential double free through [@ DocAccessibleParent::RecvMutationEvents]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Christian Holler (:decoder)]

The IPC function DocAccessibleParent::RecvMutationEvents when triggering a ShowEvent receives a tree of AccessibleData that's used to create/arrange a tree of RemoteAccessible objects in DocAccessibleParent::ProcessShowEvent.

The RemoteAccessible parent <-> child relationship is double-linked and this code path does not validate that each child only has one parent (there is a MOZ_ASSERT(CheckDocTree()); in place but that's not sufficient as a security check).

When the content process now sends such a broken tree where one child has multiple parents and then sends a HideEvent, eventually RemoteAccessible::Shutdown() will be called twice (once for each parent) on the same RemoteAccessible causing a double free.

Comment 1

[James Teh [:Jamie]]

We have an assertion here. We should be able to fix this by adding a null return for an existing parent below that:

if (newProxy->RemoteParent()) {
  return nullptr;
}

This will cause us to fail the operation and return an IPC failure here.

Comment 3

[James Teh [:Jamie]]

Attachment: (secure)

Comment 4

[James Teh [:Jamie]]

Comment on attachment 9500470 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch does hint that there is something amiss when reusing a RemoteAccessible that still has a parent. I'm hoping this looks more like something that could result in broken behaviour, though, rather than a potential security concern.
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all, yes
• 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 as is to all branches, but I haven't verified that.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely; minimal.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 5

[Tom Ritter [:tjr]]

Comment on attachment 9500470 [details]
(secure)

Approved to land

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/79144658dff0
https://hg.mozilla.org/integration/autoland/rev/a60223cb1ca3
When moving a RemoteAccessible, ensure it doesn't still have a parent. r=eeejay,decoder

Comment 7

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

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

Comment 8

[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.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox142 to wontfix.

For more information, please visit BugBot documentation.

Comment 9

[James Teh [:Jamie]]

Attachment: (secure)

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

Comment 10

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Potential security exploit.
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: not applicable
• Risk associated with taking this patch: low
• Explanation of risk level: Straightforward early return in a place where we already had an assertion.
• String changes made/needed: none
• Is Android affected?: yes

Comment 11

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/c131d6eaa323
https://hg.mozilla.org/releases/mozilla-beta/rev/714dcb2fb0a9

Comment 12

[James Teh [:Jamie]]

Attachment: (secure)

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

Comment 13

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Potential security exploit.
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: not applicable
• Risk associated with taking this patch: low
• Explanation of risk level: Straightforward early return in a place where we already had an assertion.
• String changes made/needed: none
• Is Android affected?: yes

Comment 14

[James Teh [:Jamie]]

Attachment: (secure)

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

Comment 15

[Phabricator Automation]

firefox-esr128 Uplift Approval Request

• User impact if declined: Potential security exploit.
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: not applicable
• Risk associated with taking this patch: low
• Explanation of risk level: Straightforward early return in a place where we already had an assertion.
• String changes made/needed: none
• Is Android affected?: yes

Comment 16

[James Teh [:Jamie]]

Attachment: (secure)

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

Comment 17

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined: Potential security exploit.
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: not applicable
• Risk associated with taking this patch: low
• Explanation of risk level: Straightforward early return in a place where we already had an assertion.
• String changes made/needed: none
• Is Android affected?: yes

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/5c56075bff0d

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/797307240afa

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/64d5ff6ffa5f