1447019 - Use MITIGATION_WIN32K_DISABLE flag for GMP process.

Status: REOPENED

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

Comment 1

[Bob Owen (:bobowen)]

After dropping in the necessary policy this failed straight away as they saw for the pdfium process.

Some quick debugging and it was failing in this InitUIThread [1].

With that skipped for GMP, I have Shaka Player Demo and Amazon Prime working.

aklotz - does it make sense to skip this call for GMP?
(I guess we'd want to skip for pdfium as well.)

We also have a dependency on MS getting rid of those mitigations that they were turning on in some downloadable security settings.
(See bug 1433065 comment 52 et al., although I think I saw some possible changes from chromium, so I think they are looking into fixing this issue with Application Verifier.)

I haven't done any testing for win32 calls being blocked, but given that InitUIThread was failing, that's probably a good indication. :-)

[1] https://hg.mozilla.org/mozilla-central/file/4f1014eb5039/toolkit/xre/nsEmbedFunctions.cpp#l654

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Should be okay, but ensure that the REQUIRE_DEFERRED_MESSAGE_PROTECTION flag is not set on the message channel.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

See also: https://bugs.chromium.org/p/chromium/issues/detail?id=365160

Please also review why this was added, in bug 1009590, and document any resulting restrictions on the GMP thread.  Or can this be deferred and only called when we realize we'll need it, perhaps?  (Haven't looked that closely)

Comment 4

[Bob Owen (:bobowen)]

(In reply to Randell Jesup [:jesup] from comment #3)
> See also: https://bugs.chromium.org/p/chromium/issues/detail?id=365160
> 
> Please also review why this was added, in bug 1009590, and document any
> resulting restrictions on the GMP thread.  Or can this be deferred and only
> called when we realize we'll need it, perhaps?  (Haven't looked that closely)

Yes, I'll dig into that a little deeper.
I don't think we can defer it as the functions will still be blocked, but maybe there are parts of that we still need that are not blocked.

Comment 5

[Bob Owen (:bobowen)]

Looks like we need the first thing in InitUIThread where it sets gUIThreadId.
At least we assert against it if we make intr calls from the child, which we still do for NeedShmem messages.
GetCurrentThreadId isn't win32k, so we're OK to call that anyway.

Only ContentParent, PluginModuleParent and PluginModuleChild set REQUIRE_DEFERRED_MESSAGE_PROTECTION, so we should be fine on that count for not having the SetWinEventHook etc. for GMP.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 1: Don't SetWinEventHook in InitUIThread for GMP processes

Comment 7

[Bob Owen (:bobowen)]

Attachment: Part 2: Move running from a network drive check into WinUtils

I did this clean up when I thought I was going to add the pref caching from the next patch into SandboxBroker::Initialize, but it turned out Preferences aren't initialised by then.
However, I think it's a good change, so I just put it in a separate patch.

Comment 8

[Bob Owen (:bobowen)]

Attachment: Part 3: Use MITIGATION_WIN32K_DISABLE for GMP processes based on a pref

Comment 9

[Bob Owen (:bobowen)]

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

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8962406 [details] [diff] [review]
Part 2: Move running from a network drive check into WinUtils

Review of attachment 8962406 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/WinUtils.h
@@ +482,5 @@
>    static bool ResolveJunctionPointsAndSymLinks(nsIFile* aPath);
>  
> +
> +  /**
> +   * Returns true it executable's path is on a network drive.

nit - s/it/if

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8962407 [details] [diff] [review]
Part 3: Use MITIGATION_WIN32K_DISABLE for GMP processes based on a pref

Review of attachment 8962407 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/profile/firefox.js
@@ +1070,5 @@
>  // security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
>  pref("security.sandbox.gpu.level", 0);
> +
> +// Controls whether we disable win32k for the GMP processes.
> +pref("security.sandbox.gmp.win32k-disable", true);

Hmm, should this be 'security.sandbox.gmp.win32k-disable.enabled'?

Comment 12

[Bob Owen (:bobowen)]

(In reply to Jim Mathies [:jimm] from comment #10)
...
> > +   * Returns true it executable's path is on a network drive.
> 
> nit - s/it/if

Well spotted, fixed thanks. :-)

(In reply to Jim Mathies [:jimm] from comment #11)
...
> > +// Controls whether we disable win32k for the GMP processes.
> > +pref("security.sandbox.gmp.win32k-disable", true);
> 
> Hmm, should this be 'security.sandbox.gmp.win32k-disable.enabled'?

I think that is possibly more confusing. I've updated the comment to make it clear what true means.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fbb8da315c8
Part 1: Don't SetWinEventHook in InitUIThread for GMP processes. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f61e46990eb
Part 2: Move running from a network drive check into WinUtils. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5767bc129f8
Part 3: Use MITIGATION_WIN32K_DISABLE for GMP processes based on a pref. r=jimm

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/8fbb8da315c8
https://hg.mozilla.org/mozilla-central/rev/1f61e46990eb
https://hg.mozilla.org/mozilla-central/rev/a5767bc129f8

Comment 15

[Tom25519]

Can we enable MITIGATION_WIN32K_DISABLE for GMP process now? I enable security.sandbox.gmp.win32k-disable on Firefox 115, run demo from https://github.com/cpearce/mse-eme, it seems works well.

Comment 16

[Bob Owen (:bobowen)]

(In reply to Tom25519 from comment #15)

Can we enable MITIGATION_WIN32K_DISABLE for GMP process now? I enable security.sandbox.gmp.win32k-disable on Firefox 115, run demo from https://github.com/cpearce/mse-eme, it seems works well.

I too had no problems when I enabled it, but we had intermittent issues on try that still need investigating.
Hopefully we'll find some time in the coming year to investigate.

Comment 17

[Tom25519]

(In reply to Bob Owen (:bobowen) PTO until 3rd Aug from comment #16)

(In reply to Tom25519 from comment #15)

Can we enable MITIGATION_WIN32K_DISABLE for GMP process now? I enable security.sandbox.gmp.win32k-disable on Firefox 115, run demo from https://github.com/cpearce/mse-eme, it seems works well.

I too had no problems when I enabled it, but we had intermittent issues on try that still need investigating.
Hopefully we'll find some time in the coming year to investigate.

Yeah, you're right, GMP process really crash intermittently. Today I visit https://www.amazon.com/Jewels-La%C3%ABtitia-Pujol/dp/B083LC4Q83, it says WidevineCdm crash:

https://crash-stats.mozilla.org/report/index/bp-f1f05367-5aaf-4ced-9e40-238630230801
https://crash-stats.mozilla.org/report/index/bp-9cd0c547-8d52-4c35-8242-caf260230801
https://crash-stats.mozilla.org/report/index/bp-5dad3071-d546-479a-87aa-cbe470230801

Comment 18

[Bob Owen (:bobowen)]

(In reply to Tom25519 from comment #17)

(In reply to Bob Owen (:bobowen) PTO until 3rd Aug from comment #16)
...
Yeah, you're right, GMP process really crash intermittently. Today I visit https://www.amazon.com/Jewels-La%C3%ABtitia-Pujol/dp/B083LC4Q83, it says WidevineCdm crash:

This crashes for me as well thanks for the link.