1923344 - (CVE-2024-9680) UAF in mozilla::dom::DocumentTimeline::RemoveAnimation(mozilla::dom::Animation*) / mozilla::LinkedListElement<mozilla::dom::Animation>::remove()

Status: VERIFIED FIXED

(Core :: DOM: Animation, defect, P1)

Description

[:Gijs (he/him)]

See bug 1923324 for background.

Emilio has said he can investigate.

Crash report: https://crash-stats.mozilla.org/report/index/f54bb41f-7d69-4439-a036-1884c0241008

Comment 1

[Andrew McCreight [:mccr8]]

Decoder, could you attach the test case you used? Thanks.

Comment 2

[Andrew McCreight [:mccr8]]

It sounds like there's no real stand alone test case yet.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Relevant stack

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1923344. r=smaug!

Comment 5

[Emilio Cobos Álvarez (:emilio)]

So the main issue is that MaybeResolve(this) can end up calling Animation.then if you override its prototype (and thus arbitrary JS at a bad time).

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9429648 [details]
Bug 1923344. r=smaug!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not super easily? Once you see the stack in the comments it becomes a lot easier :)
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Relatively easy I believe? ESR115 may be tricky.
• How likely is this patch to cause regressions; how much testing does it need?: I tried to keep it as dumb as possible.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 7

[Tom Ritter [:tjr] (OOTO until 7/21)]

Comment on attachment 9429648 [details]
Bug 1923344. r=smaug!

Approved to shotgun all over the trees

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1923344.

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

Comment 9

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: See bug
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: .
• Risk associated with taking this patch: low
• Explanation of risk level: Relatively simple patch, intended to be low risk.
• String changes made/needed: none
• Is Android affected?: yes

Comment 10

[Andrew McCreight [:mccr8]]

In a debug build, this hits an assertion: Assertion failure: !aAnimation->GetTimeline() || aAnimation->GetTimeline() == this, at /Users/andrewmccreight/mc/dom/animation/AnimationTimeline.cpp:93

The stack is otherwise basically the same as what Emilio posted.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: simplifed test case

Here's a reduced test case I created from the files in bug 1923324. I won't claim that it is minimal, but I removed all or most of the post-exploitation stuff.

Steps to reproduce:

1. Download the file and unzip it somewhere.
2. From a command line, go to the directory where the files are and run python3 httpserver.py.
3. Go to the browser you want to test and visit this page http://localhost:4433/download.html
4. Wait about 5 seconds and the tab should crash.

I haven't checked what happens with the fix yet but I'll do that nexxt.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9429659 [details]
Bug 1923344.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively minimal patch.

Beta/Release Uplift Approval Request

• User impact if declined: Potentially actively exploited vulnerability fix.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: .
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively simple change.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #11)

I haven't checked what happens with the fix yet but I'll do that nexxt.

I patched a MacOS debug version locally, and with the fix, the test case loads as before, but then nothing happens, so you'll probably want to wait for maybe 10 seconds to confirm nothing crashes.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/0ee07613d050
r=smaug

Comment 15

[Dianna Smith [:diannaS]]

Comment on attachment 9429648 [details]
Bug 1923344. r=smaug!

Approved for 131.0.2 dot release
Approved for 132.0b5
Approved for 128.3.1esr

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/584e678ed560

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/d2a21d941ed5

Comment 18

[Dianna Smith [:diannaS]]

Comment on attachment 9429659 [details]
Bug 1923344.

Approved for 115.16.1esr

Comment 19

[Pulsebot]

default (128.4esr) - https://hg.mozilla.org/releases/mozilla-esr128/rev/65655e6d520a

Comment 20

[Pulsebot]

default (115.17esr) - https://hg.mozilla.org/releases/mozilla-esr115/rev/32f2d2979a32

Comment 21

[Andrew McCreight [:mccr8]]

The test case I posted doesn't reproduce 100% of the time. In Nightly Firefox, it reproduced a crash 8 out of 10 times, so you'll probably want to try it a few times to confirm the fix.

I don't know if this necessarily means anything, but I opened the test case 10 times each in Chrome and Safari and there were no crashes.

Comment 22

[Pulsebot]

FIREFOX_ESR_128_3_X_RELBRANCH (128.3.1esr) - https://hg.mozilla.org/releases/mozilla-esr128/rev/e0c969a3bfc0

Comment 23

[Pulsebot]

FIREFOX_ESR_115_16_X_RELBRANCH (115.16.1esr) - https://hg.mozilla.org/releases/mozilla-esr115/rev/9454a2277178

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1923344 - Remove useless DocumentTimeline::RemoveAnimation overload. r=#firefox-animation-reviewers

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1923344 - Simplify LinkedList -> nsTArray conversion. r=#xpcom-reviewers,#firefox-animation-reviewers

Add a ToTArray version that works with LinkedList.

This is much like what we do for other containers, but without walking the list
twice.

Comment 26

[Phabricator Automation]

Comment on attachment 9429750 [details]
Bug 1923344 - Remove useless DocumentTimeline::RemoveAnimation overload. r=#firefox-animation-reviewers

Revision D225000 was moved to bug 1923505. Setting attachment 9429750 [details] to obsolete.

Comment 27

[Phabricator Automation]

Comment on attachment 9429751 [details]
Bug 1923344 - Simplify LinkedList -> nsTArray conversion. r=#xpcom-reviewers,#firefox-animation-reviewers

Revision D225001 was moved to bug 1923505. Setting attachment 9429751 [details] to obsolete.

Comment 28

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt

Comment 29

[Brindusa Tot, Desktop QA]

On desktop side, we reproduced the bug on RC131.0 based on steps from Comment 11 and we can confirm the fix on all firefox versions: RC 131.0.2, ESR 128.3.1, ESR 115.16.1, Beta 132.0b5 and latest Nightly 133.
Verified on macOS 12 ARM, Windows 11 and Ubuntu 22.

Comment 30

[Oana Horvath [:ohorvath]]

On Android, we have verified the provided STR and can confirm the fix on all Fenix & Focus versions RC 131.0.2, 132 Beta 5.
Verified on the following devices: Google Pixel 8 (Android 14), Xiaomi 12T (Android 12) and Huawei P40 Lite (Android 10).

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Similar safari change was here fwiw.

Comment 32

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Hello,

On Thunderbird side, I have managed to reproduce this issue using the STR from comment 11 with 132.0b2(20241004165235) on macOS 14.

Confirming this issue as verified fixed using Thunderbird: 132.0b3-build2(20241009162740), 131.0.1(20241009163808), 128.3.1esr(20241009142959), 115.16.0esr-build2(20241009170210) with macOS 14, Windows 11 and Ubuntu 24. It is also worth mentioning that in order to reproduce/verify this issue on Thunderbird, the usage of Link in Tab extension is imperative.

Comment 34

[Yannis Juglaret [:yannis]]

Bug 1904442 is what forced the attackers to have a different version of their exploit code pre and post 129. Bug 1904442 altered the layout of mozilla::dom::Animation objects, such that on Windows (mNext,mPrev) would now be at offsets (0x70,0x78) rather than (0x78,0x80) previously. The code from main-128.js was reliant on mNext being located at an offset of 8 modulo 16 within the layout of a mozilla::dom::Animation. More details in the incident doc.