1696387 - DWrite caching can break and not be able to reconnect with a strong sandbox.

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

This is a problem that chromium has a fix for and is probably caused by using USER_RESTRICTED (or better), untrusted integrity and possibly win32k lockdown.

Their fix hook and effectively disables certain service manager calls from dwrite.dll that causes DWrite to use a fallback path that does work within the sandbox.

We want to reimplement the same fix uses our own IAT patching.

Comment 1

[Bob Owen (:bobowen)]

Attachment: Bug 1696387 p1: Add delay-loaded patching to WindowsIATPatcher. r=aklotz!

Comment 2

[Bob Owen (:bobowen)]

Attachment: Bug 1696387 p2: Patch DWrite service manager APIs to force fallback to sandbox compatible code. r=jfkthame!,aklotz!

This includes a reimplementation of PatchServiceManagerCalls from chromium code:
content/child/font_warmup_win.cc

Depends on D107482

Comment 3

[Bob Owen (:bobowen)]

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

Comment 4

[Bob Owen (:bobowen)]

Finally got round to pushing this to try to test performance impact.
Unfortunately it does appear to have some serious regressions, as well as big reductions in file IO.
I'm not actually sure we need this for win32k lockdown, I think it might only be needed for USER_LOCKDOWN and/or untrusted integrity.
Either way we need to investigate the regressions.

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=497f608088fcdb60c90066e04005090f7a685598&newProject=try&newRevision=6f3e54bdefeb58b7f64d1dc339815938aeb06833&framework=1

Comment 5

[Bob Owen (:bobowen)]

Attachment: Bug 1696387: Don't add user's SID as deny only for restricted tokens. r=handyman!

The DirectWrite cache opens our process under impersonation.
While the access check for this passes for the restricted SIDs list, it fails
for the non-resticted SIDs.
Not adding the user's SID as deny only means it passes for both and the overall
access check works.
This shouldn't weaken the sandbox, because the user's SID is not in the
restricted list and both checks have to pass.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9fd577310e02
Don't add user's SID as deny only for restricted tokens. r=handyman

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/9fd577310e02