1768014 - Crash in [@ InsertCFontCache]

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[Bob Owen (:bobowen)]

This looks like IsWin32kLockedDown must be returning false even though it is locked down and then it tries to access GDI font functions and crashes.

It's low volume (9 clients in crash pings) , only seems to be on win11 and it's possible that it doesn't affect all content processes, so the browser remains usable.

This can only happen (AFAICT) if the function retrieval fails or the call fails.
We might be able to change the default in that case to what the sandbox policy says, so the code still behaves correctly.

Crash report: https://crash-stats.mozilla.org/report/index/091fa8e0-233b-4162-b151-4bf9c0220426

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 gdi32full.dll InsertCFontCache 
1 gdi32full.dll plfCreateLOCALFONT 
2 gdi32full.dll CreateFontIndirectExW 
3 gdi32full.dll CreateFontIndirectWImpl 
4 xul.dll gfxDWriteFontEntry::CopyFontTable gfx/thebes/gfxDWriteFontList.cpp:412
5 xul.dll gfxFontEntry::GetFontTable gfx/thebes/gfxFontEntry.cpp:579
6 xul.dll gfxDWriteFontEntry::ReadCMAP gfx/thebes/gfxDWriteFontList.cpp:523
7 xul.dll gfxFontEntry::TestCharacterMap gfx/thebes/gfxFontEntry.cpp:183
8 xul.dll gfxFontGroup::FindFontForChar gfx/thebes/gfxTextRun.cpp:3238
9 xul.dll gfxFontGroup::InitScriptRun<char16_t> gfx/thebes/gfxTextRun.cpp:2697

Comment 1

[Bob Owen (:bobowen)]

This is the second largest crash from the crash pings related to solely win32k lockdown.
Batching up the analysis makes the client count a bit tricky, but about 20-40 clients, so far in Fx100.
Crashes seem to be a small multiple of these, so might not affect all content processes.

Interesting thing is that nearly all of these seem to be Windows 11 with a locale of zh-Hans-CN.
I've installed a zh-Hans-CN version of Windows 11 on a VM (which was fun) and I get a very similar set of metadata from a crash using about:crashcontent.
However, I haven't managed to reproduce any problems with win32k lockdown enabled.

The only thing from the code that might go wrong is finding the function (to determine win32k lockdown status) or the function call failing.
I have a patch that passes over the sandbox policy settings and uses that as the default, if the above happens, instead of assuming it is not enabled.
So, we could try that, but has at least a small risk for uplift.

If it is something else causing the issue, I'm not sure we can do much about this.

Comment 2

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/jobs?repo=try&revision=f07fc38ea1c30026ac43fb383caface26a998b06

Comment 3

[Bob Owen (:bobowen)]

Messed up the selection regex last time:
https://treeherder.mozilla.org/jobs?repo=try&revision=fc924b6317a6e858f4d9f931c463c10aa105ba4a

Comment 4

[Bob Owen (:bobowen)]

Attachment: Bug 1768014 p1: Transfer mitigations to sandboxed child process. r=gcp!

Comment 5

[Bob Owen (:bobowen)]

Attachment: Bug 1768014 p2: Default to policy win32k lockdown status if in process check fails. r=gcp!

Depends on D145872

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6afde8456771
p1: Transfer mitigations to sandboxed child process. r=gcp
https://hg.mozilla.org/integration/autoland/rev/53032d712512
p2: Default to policy win32k lockdown status if in process check fails. r=gcp,cmartin

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6afde8456771
https://hg.mozilla.org/mozilla-central/rev/53032d712512

Comment 8

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

Set release status flags based on info from the regressing bug 1759168

Comment 9

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

The patch landed in nightly and beta is affected.
:bobowen, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Bob Owen (:bobowen)]

Comment on attachment 9275695 [details]
Bug 1768014 p1: Transfer mitigations to sandboxed child process. r=gcp!

Beta/Release Uplift Approval Request

• User impact if declined: Affected users will continue to experience tab crashes.
  We have only very rarely seen the issue in Nightly, so we are unlikely to know whether the fix works before it hits Beta either way.
• 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: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Putting medium because even though the change is fairly simple, it does involve writing variables cross process.
  This is done in the exact same way as an existing variable however.
• String changes made/needed: None
• Is Android affected?: No

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7cd856939016
https://hg.mozilla.org/releases/mozilla-beta/rev/5caa0f5a4ecc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9275695 [details]
Bug 1768014 p1: Transfer mitigations to sandboxed child process. r=gcp!

Approved for 101.0b6.