1860062 - Tighten GPU Sandbox Further

Status: REOPENED

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

Description

[Chris Martin [:cmartin]]

The GPU Sandbox can likely be tightened a little further, especially the access token. Let's crank up the sandbox a little more!

Comment 1

[Chris Martin [:cmartin]]

Attachment: Bug 1860062 - Minor Refactor of GPU Sandbox Initialize Code

Comment 2

[Chris Martin [:cmartin]]

Attachment: Bug 1860062 - Tighten GPU Sandbox Further

Depends on D191493

Comment 3

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

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.

Comment 4

[Chris Martin [:cmartin]]

Was on vacation for 2 weeks, waiting for next fork because I want this to ride the trains as long as possible.

Comment 5

[Chris Martin [:cmartin]]

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 the hWnd 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 caused VRShMem 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 the hWnd of the
  compositor window.

• Delayed Access Token: Currently at USER_RESTRICTED_NON_ADMIN. Increasing
  to USER_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 😎.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/5bf048ef92b7
https://hg.mozilla.org/mozilla-central/rev/4a6ed4317438

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(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 caused VRShMem to stop initializing properly.

We can probably disable/remove VRShMem if that will help here.

Comment 9

[Donal Meehan [:dmeehan]]

Requested backout, see regressions list

Comment 10

[Cosmin Sabou [:CosminS]]

Backed out from central: https://hg.mozilla.org/mozilla-central/rev/42742c4655f65e195789f7bbef595f5b7f3ed727

Comment 11

[Chris Martin [:cmartin]]

(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?

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18b02585b071
Minor Refactor of GPU Sandbox Initialize Code r=handyman

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/18b02585b071

Comment 15

[Chris Martin [:cmartin]]

Oops -- This wasn't meant to be closed when that change landed.

Comment 16

[Chris Martin [:cmartin]]

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.