1846689 - (CVE-2023-4575) use-after-free in FilePickerShownCallback

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[sonakkbi]

Attachment: ASAN.txt

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

REPRODUCE

1. apply *patch.diff
2. visit index.html
3. close serveral File Picker windows

*: 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

[Jens Stutte [:jstutte]]

From the ASAN trace it seems to me that we have a raw pointer as member of FilePickerShownCallback which in turn is hold with ref counting inside AsyncShowFilePicker which lives as a runnable in the event queue until it will be executed. So if we have more than one callback in the queue, we can fail.

It might be enough to make that raw pointer become a RefPtr<FilePickerParent>, at least to avoid this specific UAF. If the inner state of FilePickerParent is or should be able to otherwise deal with several callbacks, is a different question.

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

We should indeed probably use RefPtr here. But I wonder there's a real world way to actually call SendOpen multiple times without modifying code as comment #2 does, though. I see guards to prevent that:

• https://searchfox.org/mozilla-central/rev/85269d4444c2553e7f4c669fe4de72d64f4fe438/dom/html/HTMLInputElement.cpp#771-774
• https://searchfox.org/mozilla-central/rev/85269d4444c2553e7f4c669fe4de72d64f4fe438/dom/html/HTMLInputElement.cpp#724-727

And also the transient user activation consumption that the reporter disabled:

• https://searchfox.org/mozilla-central/rev/85269d4444c2553e7f4c669fe4de72d64f4fe438/dom/html/HTMLInputElement.cpp#679

Combined, I doubt this is actually exploitable.

Comment 5

[Daniel Veditz [:dveditz]]

The thesis of this bug is "assume a content-RCE security bug exists, it's possible to defeat the sandbox and trigger a vulnerability in non-sandboxed parent process", which is in fact something our Bug Bounty encourages people to look for. Patching our code to do extra stuff is a convenience for the demonstration (code reuse). In a real exploit the first stage would be to compromise the child and then execute your own code to send these messages. Those would clearly not be constrained by checks we put in other parts of the code that we're not even running. Any constraints in the child are entirely fictional in this scenario.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Assign this to Vincent.

Comment 7

[Jens Stutte [:jstutte]]

:vhilla, given bug 1846688, I was wondering if you plan to make a test for this. I assume it would look very similar to what is needed over there.

Comment 8

[Vincent Hilla [:vhilla]]

Apart from the raw pointer in FilePickerShownCallback, also the one here in IORunnable should be turned into a RefPtr. Otherwise, when having two file picker return a file, the second call to SendFilesOrDirectories will be a UAF.

:jstutte I'm unsure of how to test for this given the bug requires modifying code in the client process.

Comment 9

[Vincent Hilla [:vhilla]]

Attachment: Bug 1846689 - Make IORunnable::mFilePickerParent into a RefPtr. r=mccr8

Comment 10

[Andrew McCreight [:mccr8]]

Yeah, unfortunately we don't have a good framework for testing these kinds of sandbox escape issues that require arbitrary execution in the child process.

Nika looked at this history of this a bit, and it seems like FilePickerParent wasn't always refcounted (it used to be a big pain to make an IPDL actor refcounted). She also said that the color picker code was basically a copy and paste of this one (or vice versa, I forget). As such, we should ensure that both are fixed in the same release.

Nice catch on the FilePickerParent::IORunnable. It looks like you'd need FilePickerParent::Done() to get called twice before the runnable resolves in order to get two runnables created, which I don't think can be triggered from the content process, but it is still a good idea to fix this, as there's no reason now for this to be a raw pointer.

Comment 11

[Vincent Hilla [:vhilla]]

Right, IORunnable is only of concern when turning FilePickerParent* in FilePickerShownCallback into a RefPtr but not in IORunnable. Then, I can produce a UAF with the same test case as in this bug by opening a file in several file pickers.

Comment 12

[Christian Holler (:decoder)]

Is this triggerable on Linux? I know that bug 1846688 is Windows only, so it would be good to know if this can be hit by IPC fuzzing on Linux.

Comment 13

[Vincent Hilla [:vhilla]]

:decoder why do you think that about bug 1846688? I can reproduce both on Linux.

Comment 14

[Vincent Hilla [:vhilla]]

Comment on attachment 9348193 [details]
Bug 1846689 - Make IORunnable::mFilePickerParent 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 15

[Christian Holler (:decoder)]

(In reply to Vincent Hilla [:vhilla] from comment #13)

:decoder why do you think that about bug 1846688? I can reproduce both on Linux.

Does the FilePicker one require user interaction? e.g. do you need to close a dialog before the uaf happens?

Comment 16

[Tom Ritter [:tjr]]

Comment on attachment 9348193 [details]
Bug 1846689 - Make IORunnable::mFilePickerParent into a RefPtr. r=mccr8

Approved to land and uplift. Sorry this was in the queue so long, it says it still 'needs review' but still has been approved so i was confused...

Comment 17

[Pulsebot]

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

Comment 18

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

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

Comment 19

[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 20

[Vincent Hilla [:vhilla]]

(In reply to Christian Holler (:decoder) from comment #15)

Does the FilePicker one require user interaction? e.g. do you need to close a dialog before the uaf happens?

Yes, multiple dialogs need to be closed to trigger the UAF.

Comment 21

[Vincent Hilla [:vhilla]]

Comment on attachment 9348193 [details]
Bug 1846689 - Make IORunnable::mFilePickerParent 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 1846688
• 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 FilePickerParent::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 FilePickerParent::ActorDestroy, which should prevent such.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9348193 [details]
Bug 1846689 - Make IORunnable::mFilePickerParent into a RefPtr. r=mccr8

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

Comment 23

[Pulsebot]

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

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/8e661993124f

Comment 25

[Pulsebot]

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

Comment 26

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

Attachment: advisory.txt