<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Andrew McCreight [:mccr8]

Reporter

Comment 1

•

3 years ago

Attached image reproduce.gif — Details

Sergey Galich [:serg]

Updated

•

3 years ago

Group: firefox-core-security → core-security

Type: task → defect

Component: Security → Audio/Video

Product: Firefox → Core

:Gijs (he/him)

Updated

•

3 years ago

Group: core-security → media-core-security

Updated

•

3 years ago

Keywords: csectype-uaf, sec-high

Assignee

Comment 2

•

3 years ago

What is the build ID for the testing Firefox you were running on? I couldn't reprodue this error (but I encountered another SEGV) while testing that poc file on the latest central by using following commands.

python3 -m fuzzfetch --central --asan --fuzzing -n firefox
python3 -m grizzly.replay ./firefox/firefox ~/Downloads/poc.html -l ~/poc-testing

Thank you.

Assignee: nobody → alwu

Flags: needinfo?(bo13oy)

Reporter

Comment 3

•

3 years ago

Vendor=Mozilla
Name=Firefox
RemotingName=firefox
CodeName=Nightly
Version=95.0.1
BuildID=20211213184707
SourceStamp=e1e02ca86a8e08d28a750053f51cc30ed144fbb8
ID={ec8030f7-c20a-464f-9b0e-13a3a9e97384}

/usr/bin/python3 -m grizzly ../browsers/firefox/firefox sample -p ../browsers/prefs.js ->
/home/code/browsers/firefox/firefox -no-remote -profile /tmp/ffprof_d_obgvku http://127.0.0.1:32029/poc.html

I feel that this is a conditional competition vulnerability，The conditions for triggering this vulnerability are demanding, and as you can see from my reproduction video(reproduce.gif), it needs to be based on a very poorly performing environment in order to trigger the vulnerability.

Flags: needinfo?(bo13oy)

Reporter

Comment 4

•

3 years ago

Attached file prefs.js — Details

Comment hidden (offtopic)

Assignee

Comment 8

•

3 years ago

•

Edited

Set severity to P3 is because this UAF isn't easy to reproduce, per comment 3, it seems only being able to reproduce on poorly performing environment. I couldn't reproduce this UAF on both central and the build 20211213184707, which is the one used by reporter.

I suspect this issue is caused by here. The following is the possible situation.

(1) Shutdown the media sink #1
When we finish decoding the first frame, we would suspend the decoder which causes the media sink gets shutdown.

(2) Media starts playing, then creating another new media sink #2
In POC file, audio.play().then(function(){}); will trigger decoder to start playing, and then the play state of decoder will be mirrored to MDSM. As the play state is playing, it's possible to trigger starting media sink and then create a new media sink.

(3) Resume media sink, which causes incorrectly discard media sink #2 and create another sink #3
We would resume the decoder when media element starts playing. Notice that, although this happen before mDecoder->Play() (that is the step (2) to change play state), the mirror is done by a direct task, so even if it's called later, it might still be possible to be processed before the resume task (just a regular task).

Therefore, if (2) runs before (3), ResumeMediaSink() will just create another new sink, and the old one won't be stopped properly.

Severity: -- → S3

Priority: -- → P1

Assignee

Comment 9

•

3 years ago

Attached file Bug 1745874 - only recreate and start media sink when it's not running. — Details

Assignee

Comment 10

•

3 years ago

After an offline discussion with bryce, I will provide more details for how those tasks get dispatched.

Let's assuming that now the task queue (MDSM thread) is still running another task group runnable A, and the script is calling audio.play() on the main thread. Then in HTMLMediaElement::PlayInternal(), these two functions will be called, the first one will eventually queue a regular task on the task queue, and the second one will change the canonical play state which queues a state-change task on the task queue.

Because the task queue supports tail dispatching, all related tasks will be stored in its task group. Inside a task group, the task will be seperate into three different types of tasks: StateChangeTask (state-mirroring), DirectTask (state-watching) and RegularTask. Those tasks will be run in a different priorities.

Now back to the crash, after calling decoder.resume(), it would queue a regular task into the task group and the task queue (MDSM) will look like this

[task group A (running)] [task group B : resuming media sink (waiting)]

What I guess is that, on a super slow machine, task A takes longer time to run, and decoder.play() would be called before the task A finsihes, which would queue a state-change into task group B. Then as state-change task would be run before regular task, resuming media sink would run later and cause recreating the media sink.

Assignee

Comment 11

•

3 years ago

Comment on attachment 9255824 [details]
Bug 1745874 - only recreate and start media sink when it's not running.

Security Approval Request

How easily could an exploit be constructed based on the patch?: It seems difficult to use this patch to construct the exploit because it involves many layers of event dispatching.
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?: 96
If not all supported branches, which bug introduced the flaw?: Bug 1571513
Do you have backports for the affected branches?: Yes
If not, how different, hard to create, and risky will they be?:
How likely is this patch to cause regressions; how much testing does it need?: Low risk to cause a regression, because we already have a debug assertion to prevent this thing happen. Now we just change the assertion to real conditional check to further prevent this situation happens. This patch doesn't introduce any new functionality.

Attachment #9255824 - Flags: sec-approval?

Tom Ritter [:tjr]

Comment 12

•

3 years ago

Comment on attachment 9255824 [details]
Bug 1745874 - only recreate and start media sink when it's not running.

approved to land and request uplift

Attachment #9255824 - Flags: sec-approval? → sec-approval+

Assignee

Comment 13

•

3 years ago

•

Edited

Comment on attachment 9255824 [details]
Bug 1745874 - only recreate and start media sink when it's not running.

Per this process I would like to request a uplift and land them as close as possible. However, this patch won't work on Release, I would need to update another verison of patch in order to avoid the merging conflict.

Beta/Release Uplift Approval Request

User impact if declined: Firefox would crash and the exploit might be used for attacking
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: No
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): This change is to add a new conditional check to avoid triggering the UAF, it doesn't introduce any new behavior.
String changes made/needed: no

Attachment #9255824 - Flags: approval-mozilla-beta?

Assignee

Comment 14

•

3 years ago

Attached file Fix-for-release-version — Details

Approval Request Comment
[Feature/Bug causing the regression]: bug 1571513
[User impact if declined]: Firefox would crash and the exploit might be used for attacking
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: This change is to add a new conditional check to avoid triggering the UAF, it doesn't introduce any new behavior.
[String changes made/needed]: no

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Firefox would crash and the exploit might be used for attacking
Fix Landed on Version: 97
Risk to taking this patch (and alternatives if risky): low risk, this change is to add a new conditional check to avoid triggering the UAF, it doesn't introduce any new behavior.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Attachment #9257377 - Flags: approval-mozilla-release?

Attachment #9257377 - Flags: approval-mozilla-esr91?

Updated

•

3 years ago

status-firefox95: --- → wontfix

status-firefox96: --- → affected

status-firefox97: --- → affected

status-firefox-esr91: --- → affected

tracking-firefox96: --- → +

tracking-firefox97: --- → +

tracking-firefox-esr91: --- → 96+

Comment 15

•

3 years ago

Comment on attachment 9255824 [details]
Bug 1745874 - only recreate and start media sink when it's not running.

96 was already merged to release today as we're now in RC week.

Attachment #9255824 - Flags: approval-mozilla-beta? → approval-mozilla-release?

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

Updated

•

3 years ago

Attachment #9257377 - Flags: approval-mozilla-release?

Comment 16

•

3 years ago

only recreate and start media sink when it's not running. r=bryce
https://hg.mozilla.org/integration/autoland/rev/20b9b3d5a59533c894f8350f2f608d07137bb312
https://hg.mozilla.org/mozilla-central/rev/20b9b3d5a595

Group: media-core-security → core-security-release

Status: NEW → RESOLVED

Closed: 3 years ago

status-firefox97: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → 97 Branch

Dianna Smith [:diannaS]

Comment 17

•

3 years ago

Comment on attachment 9255824 [details]
Bug 1745874 - only recreate and start media sink when it's not running.

Approved for 96.0rc2

Attachment #9255824 - Flags: approval-mozilla-release? → approval-mozilla-release+

Dianna Smith [:diannaS]

Comment 18

•

3 years ago

uplift

https://hg.mozilla.org/releases/mozilla-release/rev/7269567942cd

status-firefox96: affected → fixed

Updated

•

3 years ago

Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form][adv-main96+][adv-ESR91.5+]

Comment 19

•

3 years ago

Attached file advisory.txt (obsolete) — Details

Reporter

Comment 20

•

3 years ago

Comment on attachment 9257644 [details] advisory.txt >Race condition when playing audio files >bo13oy of Cyber Kunlun Lab > >Constructing audio sinks could lead to a race condition when playing audio files and closing windows. This can lead to a use-after-free causing a potentially exploitable crash.

Comment 21

•

3 years ago

Attached file advisory.txt — Details

Thanks.

Attachment #9257644 - Attachment is obsolete: true

Comment 22

•

3 years ago

Comment on attachment 9257377 [details]
Fix-for-release-version

Approved for 91.5esr.

Attachment #9257377 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+

https://hg.mozilla.org/releases/mozilla-esr91/rev/c50ba4894d46

Comment 23

•

3 years ago

uplift

status-firefox-esr91: affected → fixed

BugBot [:suhaib / :marco/ :calixte]

Comment 24

•

3 years ago

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(alwu)

Whiteboard: [reporter-external] [client-bounty-form][adv-main96+][adv-ESR91.5+] → [reporter-external] [client-bounty-form][adv-main96+][adv-ESR91.5+][sec-survey]

Assignee

Updated

•

3 years ago

Flags: needinfo?(alwu)