1851118 - `gfxFontGroup::GetFirstValidFont()` may run script via loading web fonts, but it does not look like so

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

gfxFontGroup::GetFirstValidFont() may request to load a web font. Then, it reaches here and it will run observers. Therefore, we cannot detect security bugs with static analysis. The bad thing here is, the API is used for most of nsFontMetrics methods. Therefore, when DOM or layout code tries to retrieve a size of characters, etc, content script may potentially run and some frames could be destroyed. However, most of the nsFontMetrics users do not take care of the lifetime of frames. Therefore, UAF could happen. E.g., after here, the this may have already gone.

I have no idea how to fix this issue with minimizing the risk. If the GFX API could stop loading web fonts synchronously, we could fix this with minimum changes, but I guess it's risky?

Comment 1

[Emilio Cobos Álvarez [:emilio]]

I don't think it's expected that AsyncOpen runs script. Triggering the font load during layout is kind of how web fonts are supposed to work. We could defer them and have code to do so in some cases, but...

Comment 2

[Andrew McCreight [:mccr8]]

It looks like devtools and maybe some WebExtension code observes "http-on-opening-request" and can end up running JS.

Comment 3

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

It is expected that AsyncOpen runs scripts, at least traditionally it has been.
That is why DOM code for example has all these script runners https://searchfox.org/mozilla-central/rev/a7e33b7f61e7729e2b1051d2a7a27799f11a5de6/dom/html/HTMLImageElement.cpp#585-587

Comment 4

[Emilio Cobos Álvarez [:emilio]]

So... one potential solution is just making the load async using a script runner, but... WDYT about just switching necko to use NotifyWhenScriptSafe? Seems we could also clean up some DOM code if observers don't rely on being synchronous?

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

So... one potential solution is just making the load async using a script runner, but... WDYT about just switching necko to use NotifyWhenScriptSafe? Seems we could also clean up some DOM code if observers don't rely on being synchronous?

I believe that these notifications (I presume you're talking about the HTTP ones like https://searchfox.org/mozilla-central/rev/a7e33b7f61e7729e2b1051d2a7a27799f11a5de6/netwerk/protocol/http/nsHttpHandler.h#355-358) are intended to allow listeners to synchronously mutate the channel before proceeding to the next stage. We probably can't run these asynchronously, as that could change the expected behaviour.

The sequencing around these is a bit tricky. If we needed to make https://searchfox.org/mozilla-central/rev/b6cd901cf074c5e75e3902d660bafa3cf3967c40/netwerk/protocol/http/HttpChannelChild.cpp#2071 async for example, we may also need to delay the rest of the AsyncOpen call to happen after the observer notification has happened, which may or may not be OK. It's a similar situation in the parent process (https://searchfox.org/mozilla-central/rev/b6cd901cf074c5e75e3902d660bafa3cf3967c40/netwerk/protocol/http/nsHttpChannel.cpp#6016).

It might be safer to instead have the specific AsyncOpen calls which are happening at a bad time be moved behind a ScriptRunner. For example, for the case in bug 1850938, it might be possible to move the AsyncOpen call (https://searchfox.org/mozilla-central/rev/b6cd901cf074c5e75e3902d660bafa3cf3967c40/layout/style/FontFaceSetDocumentImpl.cpp#307) into a script runner. This reduces the risk of a large-scale behaviour change, in exchange for not making AsyncOpen generally avoid running script in general.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It might be safer to instead have the specific AsyncOpen calls which are happening at a bad time be moved behind a ScriptRunner. For example, for the case in bug 1850938, it might be possible to move the AsyncOpen call (https://searchfox.org/mozilla-central/rev/b6cd901cf074c5e75e3902d660bafa3cf3967c40/layout/style/FontFaceSetDocumentImpl.cpp#307) into a script runner.

Script runner requires script blocker to make it run asynchronously from callers point of view. So, the all callers require to change and the methods need to be MOZ_CAN_RUN_SCRIPT. So I think that NS_DispatchToCurrentThread or something is better.

Comment 7

[Daniel Veditz [:dveditz]]

Not sure this is "sec-high" or if it's "it would be sec-high if people abuse it, but we don't know if they are". The latter might be sec-moderate, at least until we find a place that uses this bad pattern. Or if the only places are DevTools and Web Extensions we could call it sec-moderate because not everyone will be affected unless they've opted-in to using those features.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Nika is correct in comment 5. Most of the observer notifications sent from AsyncOpen are meant to be sync so that devtools & webext can change the channel if needed.

It might be safer to instead have the specific AsyncOpen calls which are happening at a bad time be moved behind a ScriptRunner.

Not sure exactly how the script runner works, or if there's something we should do in Necko about it. I'd appreciate some info if possible.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #8)

It might be safer to instead have the specific AsyncOpen calls which are happening at a bad time be moved behind a ScriptRunner.

Not sure exactly how the script runner works, or if there's something we should do in Necko about it. I'd appreciate some info if possible.

[ni=nika since I think this was in response to her]

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #6)

It might be safer to instead have the specific AsyncOpen calls which are happening at a bad time be moved behind a ScriptRunner. For example, for the case in bug 1850938, it might be possible to move the AsyncOpen call (https://searchfox.org/mozilla-central/rev/b6cd901cf074c5e75e3902d660bafa3cf3967c40/layout/style/FontFaceSetDocumentImpl.cpp#307) into a script runner.

Script runner requires script blocker to make it run asynchronously from callers point of view. So, the all callers require to change and the methods need to be MOZ_CAN_RUN_SCRIPT. So I think that NS_DispatchToCurrentThread or something is better.

I was assuming based on :emilio's suggestion of using NotifyWhenScriptSafe that there is a script blocker on the stack in the problematic cases, hence why I suggested using a script runner for the async open calls. It would also be fine to use NS_DispatchToCurrentThread if we're OK with the latency in cases where we could theoretically synchronously run script.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1851118 - Minor cleanups in gfxUserFontSet. r=jfkthame,dholbert

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1851118 - Defer url() font load start until safe. r=jfkthame,dholbert

Use the same mechanism we use for style worker threads (LOAD_PENDING /
ContinueLoad()).

Depends on D189266

Comment 13

[Phabricator Automation]

Comment on attachment 9355148 [details]
Bug 1851118 - Minor cleanups in gfxUserFontSet. r=jfkthame,dholbert

Revision D189266 was moved to bug 1855504. Setting attachment 9355148 [details] to obsolete.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9355149 [details]
Bug 1851118 - Defer url() font load start until safe. r=jfkthame,dholbert

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. Afaict we don't allow untrusted script to run but...
• 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?: 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?: Should apply cleanly
• How likely is this patch to cause regressions; how much testing does it need?: Not very likely, we have good test coverage for this.
• Is Android affected?: Yes

Comment 15

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 9355149 [details]
Bug 1851118 - Defer url() font load start until safe. r=jfkthame,dholbert

Approved to land, and uplift when ready. (Not sure if you want to bake this for a week first.)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdb157285d27
Defer url() font load start until safe. r=jfkthame,masayuki

Comment 17

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

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

Comment 18

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

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

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

For more information, please visit BugBot documentation.

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Masayuki do you think this is worth uplifting?

Comment 20

[Alex Finder]

(In reply to Pulsebot from comment #16)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdb157285d27
Defer url() font load start until safe. r=jfkthame,masayuki
== Change summary for alert #39738 (as of Sun, 01 Oct 2023 21:42:07 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
17%	reddit-billgates-post-2.billg ContentfulSpeedIndex	linux1804-64-shippable-qr	cold fission webrender	965.68 -> 803.69
11%	reddit-billgates-post-2.billg SpeedIndex	linux1804-64-shippable-qr	cold fission webrender	616.71 -> 548.18
7%	reddit-billgates-post-2.billg PerceptualSpeedIndex	linux1804-64-shippable-qr	cold fission webrender	646.43 -> 600.04

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=39738

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #19)

Masayuki do you think this is worth uplifting?

I don't think so. It's a traditional bug.

Comment 22

[Andrew McCreight [:mccr8]]

Is there no actual danger of a UAF that this is fixing? Currently it is marked sec-high because it seemed like running script at an odd time was sec high. Maybe it is more like sec-moderate because we're only running a small set of chrome JS then?

Comment 23

[Donal Meehan [:dmeehan]]

:emilio based on Comment 21 and Comment 22, what are the current expectations around beta/esr uplifts?

Comment 24

[Emilio Cobos Álvarez [:emilio]]

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

Is there no actual danger of a UAF that this is fixing? Currently it is marked sec-high because it seemed like running script at an odd time was sec high. Maybe it is more like sec-moderate because we're only running a small set of chrome JS then?

Yeah my understanding is that no content JS should be able to run.

Comment 25

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter