1643874 - (CVE-2020-12419) Crash in [@ mozilla::dom::Promise::MaybeSomething<T>]

Status: RESOLVED FIXED

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

Description

[Worcester12345]

This bug is for crash report bp-1c57af62-86cf-4afd-9dfb-acbb30200605.

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::Promise::MaybeSomething<mozilla::ErrorResult> dom/promise/Promise.h:327
1 xul.dll PromiseDocumentFlushedResolver::Call dom/base/nsGlobalWindowInner.cpp:816
2 xul.dll nsGlobalWindowInner::CallOrCancelDocumentFlushedResolvers<1> dom/base/nsGlobalWindowInner.cpp:6691
3 xul.dll nsGlobalWindowInner::DidRefresh dom/base/nsGlobalWindowInner.cpp:6762
4 xul.dll nsRefreshDriver::Tick layout/base/nsRefreshDriver.cpp:2263
5 xul.dll mozilla::RefreshDriverTimer::TickRefreshDrivers layout/base/nsRefreshDriver.cpp:350
6 xul.dll mozilla::RefreshDriverTimer::Tick layout/base/nsRefreshDriver.cpp:367
7 xul.dll mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver layout/base/nsRefreshDriver.cpp:745
8 xul.dll mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync layout/base/nsRefreshDriver.cpp:644
9 xul.dll mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyParentProcessVsync layout/base/nsRefreshDriver.cpp:565

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0 ID:20200604213430

Adblock Plus - free ad blocker 3.8.4
Amazon Assistant for Firefox 10.2005.6.12051
Amazon.com 1.1
Auto Tab Discard 0.3.5.2
Avast Online Security 20.1.480 [DISABLED]
Bing 1.1
DuckDuckGo 1.0
Duplicate Tabs Closer 3.5.1 [DISABLED]
Google 1.0
Nightly Tester Tools 4.0
NoScript 11.0.30
Print Selection to PDF 0.1.0 [DISABLED]
Screengrab! 2.18 [DISABLED]
SortTabs 1.1.0 [DISABLED]
Tab Counter 0.4.1
Tabliss 2.0.3 [DISABLED]
Tabs manager 1.8
Wikipedia (en) 1.0
eBay 1.0

Firefox 78.0b2 Crash Report [@ mozilla::dom::Promise::MaybeSomething<T> ]
You are seeing public data only. See protected data access documentation for more information.
Search Mozilla Support for this signature How to read this crash report
ID: 1c57af62-86cf-4afd-9dfb-acbb30200605
Signature: mozilla::dom::Promise::MaybeSomething<T>

Comment 1

[John Dai[:jdai]]

Hi mconley,
According to crash frame 1, it seems from PromiseDocumentFlushedResolver::Call. Could you please help to take a look? Thank you.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I'm not 100% certain how this stack could occur. Is the Promise's mGlobal nullptr or something?

Hey jya, you've been doing work with platform Promise-y things lately... do you have any idea what might be triggering a segfault here?

Comment 3

[Rachel Tublitz [:rachel]]

S1 or S2 bugs needs an assignee - could you find someone for this bug?

Comment 4

[Jean-Yves Avenard [:jya]]

(In reply to Mike Conley (:mconley) (:⚙️) (Extremely busy) from comment #2)

I'm not 100% certain how this stack could occur. Is the Promise's mGlobal nullptr or something?

Hey jya, you've been doing work with platform Promise-y things lately... do you have any idea what might be triggering a segfault here?

Sorry , but wrong promises.

It's MozPromise I've been playing with, a pure C++ object, not dom::Promise

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Hm, okay. I'm hesitant to bug smaug, since he's so busy lately... maybe asuth knows?

Hey asuth, any idea how a DOM Promise could result in this kind of segfault? The only way I can imagine this segfaulting on the line that the crash report suggests is if mGlobal is null... but I can't see how that can happen. Anything obvious that I'm missing?

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

This is probably a use-after-free.

get-minidump-instructions from minidump-tools can be directly run against the downloaded .dmp file from crash-stats (which magically is able to actually find the source and interleave it), and it gives us the following line as crashing:

   7fffd8d7d001:	44 8a 88 2c 00 00 00 	mov r9b, byte ptr [rax + 0x2c]
   7fffd8d7d008:	48 8b 56 18          	mov rdx, qword ptr [rsi + 0x18]
^^^^^^^^^^^^^^^^	^^^^^^^^^^^^^^^^^^^^^	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   7fffd8d7d00c:	4c 8d 05 de a5 10 04 	lea r8, qword ptr [rip + 0x410a5de]

The raw data JSON tells us "rsi": "0xe5e5e5e5e5e5e5e5", which is our free pattern.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Why is https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/dom/base/nsGlobalWindowInner.cpp#6698 safe?
Can't something something modify the array while it is being iterated because https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/dom/base/nsGlobalWindowInner.cpp#806 calls random JS ?
Or perhaps the whole nsGlobalWindowInner dies. Nothing seems to guarantee it stays alive if somehow unlink happens while JS is called.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Olli Pettay [:smaug] from comment #7)

Why is https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/dom/base/nsGlobalWindowInner.cpp#6698 safe?
Can't something something modify the array while it is being iterated because https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/dom/base/nsGlobalWindowInner.cpp#806 calls random JS ?

It looks like we have a mIteratingDocumentFlushedResolvers guard to prevent appending more PromiseDocumentFlushed resolvers while iterating the list, so I suspect no.

Or perhaps the whole nsGlobalWindowInner dies. Nothing seems to guarantee it stays alive if somehow unlink happens while JS is called.

This might be it. Is it sufficient to add a RefPtr to the inner window on each PromiseDocumentFlushedResolver, or is there a better way to keep the window alive?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Mike Conley (:mconley) (:⚙️) (Extremely busy) from comment #8)

(In reply to Olli Pettay [:smaug] from comment #7)

Why is https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/dom/base/nsGlobalWindowInner.cpp#6698 safe?
Can't something something modify the array while it is being iterated because https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/dom/base/nsGlobalWindowInner.cpp#806 calls random JS ?

It looks like we have a mIteratingDocumentFlushedResolvers guard to prevent appending more PromiseDocumentFlushed resolvers while iterating the list, so I suspect no.

Only if Unlink doesn't happen somehow during iteration. Unlikely though.

I would use effectively a kungfuDeathGrip on stack when dealing with RefreshDriver callbacks (which are weak).

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1643874. r?smaug!

Comment 11

[Mike Conley (:mconley) (:⚙️)]

While evaluating this from a security point of view, note that promiseDocumentFlushed is a chrome-privileged method on Window that can only be called on chrome windows.

Comment 12

[Daniel Veditz [:dveditz]]

When is it called from privileged code? Any states web content can cause us to get into by opening and closing tabs/windows/frames?

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9156282 [details]
Bug 1643874. r?smaug!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I would imagine pretty difficult. An attacker would already need to have gotten access to the parent process to use the promiseDocumentFlushed method.
• 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?: 78, 77 and ESR 68
• 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?: Not hard, not different, not risky.
• How likely is this patch to cause regressions; how much testing does it need?: Extremely unlikely to cause regressions, will not require additional testing.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 9156282 [details]
Bug 1643874. r?smaug!

sec-approval+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/650eb25c46f144684c2b5f498a96d4cda904c8bc

Please nominate this for Beta and ESR68 approval when you get a chance. It grafts cleanly to both as-landed.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9156282 [details]
Bug 1643874. r?smaug!

Beta/Release Uplift Approval Request

• User impact if declined: (Low) risk of a UAF vulnerability being exploited.
• 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): Just adds a kungFuDeathGrip. This is a common pattern to hold a reference to something that might die later on in the function when JS runs.
• String changes made/needed: None.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: (Low) risk of a UAF vulnerability being exploited.
• Fix Landed on Version: 79.0a1
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just adds a kungFuDeathGrip. This is a common pattern to hold a reference to something that might die later on in the function when JS runs.
• String or UUID changes made by this patch: None.

Comment 17

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

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 9156282 [details]
Bug 1643874. r?smaug!

approved for 78.0b9 and 68.10esr

Comment 19

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

https://hg.mozilla.org/integration/autoland/rev/650eb25c46f144684c2b5f498a96d4cda904c8bc
https://hg.mozilla.org/mozilla-central/rev/650eb25c46f1

Comment 20

[Julien Cristau [:jcristau]]

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

Comment 21

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/f60d8342433b

Comment 22

[Tom Ritter [:tjr]]

Attachment: advisory.txt

mconley - Does this advisory look okay to you?
worcester12345 - how would you like to be credited?

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Hey tjr, I'd suggest altering this slightly from:

When processing callbacks that occured during window flushing

to

When processing callbacks that occurred during window flushing in the parent process

since it's not really possible to trigger this from content processes. (Also, note typo: "occured" -> "occurred").

Thanks!

Comment 24

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Thanks!

Comment 25

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

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.

Comment 26

[Worcester12345]

(In reply to Tom Ritter [:tjr] (ni for response to sec-[approval|rating|advisories|cve]) from comment #22)

Created attachment 9158639 [details]
advisory.txt

worcester12345 - how would you like to be credited?

Not sure what you mean.

Comment 27

[Tom Ritter [:tjr]]

(In reply to Worcester12345 from comment #26)

(In reply to Tom Ritter [:tjr] (ni for response to sec-[approval|rating|advisories|cve]) from comment #22)

Created attachment 9158639 [details]
advisory.txt

worcester12345 - how would you like to be credited?

Not sure what you mean.

Ah - we'll be issuing an advisory for this issue, here is a page of the advisories for the past Firefox release: https://www.mozilla.org/en-US/security/advisories/mfsa2020-20/

In it we credit the reporter, presently all I have for you is 'worcester12345', if you'd like something different, please let me know.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

I filled in the Google form.