Tighten GPU Sandbox Further
Categories
(Core :: Security: Process Sandboxing, enhancement, P3)
Tracking
()
People
(Reporter: cmartin, Assigned: cmartin)
References
()
Details
(Keywords: parity-chrome)
Attachments
(1 file, 1 obsolete file)
The GPU Sandbox can likely be tightened a little further, especially the access token. Let's crank up the sandbox a little more!
Assignee | ||
Comment 1•8 months ago
|
||
Assignee | ||
Comment 2•8 months ago
|
||
Depends on D191493
Comment 3•7 months ago
|
||
There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:cmartin, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.
Assignee | ||
Comment 4•7 months ago
|
||
Was on vacation for 2 weeks, waiting for next fork because I want this to ride the trains as long as possible.
Assignee | ||
Comment 5•6 months ago
•
|
||
I'm about to land this change, but before I do I just want to document how I arrived here, because I tried increasing the GPU sandbox past where it is in several ways, but all of them returned failures:
-
Job level: Currently at
JOB_LIMITED_USER
. Increasing one level to
JOB_RESTRICTED
prevented GPU process from accessing thehWnd
of the
compositor window. This was not fixed even if all UI restrictions were
lifted.Interestingly, even getting rid of the few remaining UI exceptions (like
the ability to shutdown windows) also broke things. It seems like Windows
must disable some entire subsystem in an all-or-nothing fashion when all
the required mitigations are enabled, and when we cross that line it
breaks our GPU process. -
Integrity Level: Currently at
INTEGRITY_LEVEL_LOW
. Increasing to
INTEGRITY_LEVEL_UNTRUSTED
causedVRShMem
to stop initializing properly. -
Delayed Integrity Level: Same as the initial Integrity Level.
-
Access Token: Currently at
USER_RESTRICTED_SAME_ACCESS
. Increasing to
USER_NON_ADMIN
prevented GPU process from accessing thehWnd
of the
compositor window. -
Delayed Access Token: Currently at
USER_RESTRICTED_NON_ADMIN
. Increasing
toUSER_LIMITED
was successful in my testing. -
I did not play around too much with the OS-level mitigations, as none of
them struck me as something that would help above-and-beyond what we
already had.
So, overall the only headway I was able to make for Sandbox Level 2 was an increase in the Delayed Access Token from USER_RESTRICTED_NON_ADMIN
to USER_LIMITED
.
This is actually the same conclusion David Parks had reached when he worked on this years ago, so I guess it's nice to see two separate people arrive at the same conclusion 😎.
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5bf048ef92b7 Minor Refactor of GPU Sandbox Initialize Code r=handyman https://hg.mozilla.org/integration/autoland/rev/4a6ed4317438 Tighten GPU Sandbox Further r=handyman
Comment 7•6 months ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5bf048ef92b7
https://hg.mozilla.org/mozilla-central/rev/4a6ed4317438
Comment 8•6 months ago
|
||
(In reply to Chris Martin [:cmartin] from comment #5)
I'm about to land this change, but before I do I just want to document how I arrived here, because I tried increasing the GPU sandbox past where it is in several ways, but all of them returned failures:
How do our values for these things compare to Chrome's?
- Integrity Level: Currently at
INTEGRITY_LEVEL_LOW
. Increasing to
INTEGRITY_LEVEL_UNTRUSTED
causedVRShMem
to stop initializing properly.
We can probably disable/remove VRShMem if that will help here.
Updated•6 months ago
|
Comment 9•6 months ago
|
||
Requested backout, see regressions list
Comment 10•6 months ago
|
||
Backed out from central: https://hg.mozilla.org/mozilla-central/rev/42742c4655f65e195789f7bbef595f5b7f3ed727
Updated•6 months ago
|
Assignee | ||
Comment 11•6 months ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
How do our values for these things compare to Chrome's?
This would have put us on par with Chrome.
We can probably disable/remove VRShMem if that will help here.
It might, but I'd have to test it. It's possible that if we remove VRShMem
, something else would blow up instead.
Is there an easy way for me to disable it for testing purposes?
Assignee | ||
Updated•6 months ago
|
Assignee | ||
Updated•6 months ago
|
Updated•6 months ago
|
Comment 12•6 months ago
|
||
(In reply to Chris Martin [:cmartin] from comment #11)
It might, but I'd have to test it. It's possible that if we remove
VRShMem
, something else would blow up instead.Is there an easy way for me to disable it for testing purposes?
Commenting out EnsureVRManager()
here https://searchfox.org/mozilla-central/rev/381c3f18a1896792e23a9503c1904aedac19aa06/gfx/ipc/GPUProcessManager.cpp#371 might do the trick
Assignee | ||
Updated•6 months ago
|
Comment 13•6 months ago
|
||
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/18b02585b071 Minor Refactor of GPU Sandbox Initialize Code r=handyman
Comment 14•6 months ago
|
||
bugherder |
Assignee | ||
Comment 15•5 months ago
|
||
Oops -- This wasn't meant to be closed when that change landed.
Updated•5 months ago
|
Assignee | ||
Comment 16•3 months ago
•
|
||
Just to follow up on the status of this for the time being -- We were hoping that we could simply increase the sandbox level to '2' and have it Just Work™. The regressions above show that is not the case.
The VRShMem
thing is fairly-easily worked around by disabling it (as Jeff Muizelaar suggested above), but the font stuff seems like it's pretty non-trivial to fix. We were able to reproduce some of the behavior locally, but we weren't able to reproduce the behavior seen in Bug 1865973, which was probably the worst of the bunch. It's possible that the effort to fix these regressions could be quite large.
For the time being, we're going to pause on this and open a JIRA ticket to start a discussion about the cost:benefit ratio of increasing the GPU Sandbox strength further.
Updated•26 days ago
|
Updated•23 days ago
|
Description
•