1846688 - (CVE-2023-4574) use-after-free in ColorPickerShownCallback

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P2)

Description

[sonakkbi]

Attachment: ASAN.txt

|ColorPickerShownCallback| is requested when a Color Picker window open1. It isn’t considered several |ColorPickerShownCallbacks| are requested at a time. And |Done()| will destroy |mColorPickerParent|2. This leads to use-after-free when another |Done()| is called after a |Done()|

REPRODUCE

1. apply *patch.diff
2. visit index.html

*: Patch to emulate a compromised content process.

Crash State: see asan file

Comment 1

[sonakkbi]

Attachment: index.html

Comment 2

[sonakkbi]

Attachment: patch.diff

Comment 3

[sonakkbi]

Attachment: patch.diff

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 9346875 [details] [diff] [review]
patch.diff

hiding to avoid confusion... duplicate attachment

Comment 5

[Andrew McCreight [:mccr8]]

As this code is basically identical to the code in bug 1846689, we want to make sure these are fixed in the same release.

Comment 6

[Vincent Hilla [:vhilla]]

Attachment: Bug 1846688 - Make ColorPickerShownCallback::mCallback into a RefPtr. r=mccr8

Depends on D185820

Comment 7

[Vincent Hilla [:vhilla]]

Comment on attachment 9348238 [details]
Bug 1846688 - Make ColorPickerShownCallback::mCallback into a RefPtr. r=mccr8

Security Approval Request

• How easily could an exploit be constructed based on the patch?: To exploit this bug, there first has to be a content-RCE security bug compromising the child process.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: Bug 910384
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The same patch should apply to older versions too.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. The main concern are leaks, though the newly introduced strong reference is always cleared in FilePickerParent::ActorDestroy.
• Is Android affected?: Unknown

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 9348238 [details]
Bug 1846688 - Make ColorPickerShownCallback::mCallback into a RefPtr. r=mccr8

Approved to land and uplift

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e69719641442
Make ColorPickerShownCallback::mCallback into a RefPtr. r=mccr8

Comment 10

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

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

Comment 11

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

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

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

For more information, please visit BugBot documentation.

Comment 12

[Vincent Hilla [:vhilla]]

Comment on attachment 9348238 [details]
Bug 1846688 - Make ColorPickerShownCallback::mCallback into a RefPtr. r=mccr8

Beta/Release Uplift Approval Request

• User impact if declined: Security risk. If someone can compromise a child process, this UAF could enable a sandbox escape.
• 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: Bug 1846689
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is small with the only risk being a leak. Though the newly introduced strong reference is always cleared in ColorPickerParent::ActorDestroy, which should prevent such.
• String changes made/needed:
• Is Android affected?: Unknown

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Security risk. If someone can compromise a child process, this UAF could enable a sandbox escape.
• Fix Landed on Version: 118
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is small with the only risk being a leak. Though the newly introduced strong reference is always cleared in ColorPickerParent::ActorDestroy, which should prevent such.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9348238 [details]
Bug 1846688 - Make ColorPickerShownCallback::mCallback into a RefPtr. r=mccr8

Approved for 117.0b9, 115.2esr, and 102.15esr.

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/93fe46d88a5e

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/5c1290876623

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr102/rev/21a369743b3a

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1846688 - Backport ManualDealloc removal from ColorPicker and FilePicker.

This is a rebased version of bug 1816740 #6:
https://hg.mozilla.org/mozilla-central/rev/dab075577d74

There should be no functional changes from this change, but it resolves
the build failures caused by the patches in bug 1846688 and bug 1846689.

Comment 18

[Phabricator Automation]

Uplift Approval Request

• Is Android affected?: no
• Fix verified in Nightly: no
• Code covered by automated testing: yes
• Risk associated with taking this patch: low
• String changes made/needed: --
• Needs manual QE test: yes
• Explanation of risk level: no functional changes; straightforward backport of (verified-in-Nightly) no-functional-changes patch on mainline
• Steps to reproduce for manual QE testing: confirm no crash when using data:text/html,<input type=file multiple>
• User impact if declined: can't easily uplift other dependent patches

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr102/rev/6f58eb1a03ef

Comment 20

[June Wilde (she/her) [:jewilde]]

Attachment: advisory.txt