Use MITIGATION_WIN32K_DISABLE flag for GMP process.
Categories
(Core :: Security: Process Sandboxing, enhancement, P2)
Tracking
()
People
(Reporter: bobowen, Unassigned)
References
(Depends on 1 open bug)
Details
(Keywords: sec-want)
Attachments
(3 files)
1.82 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
3.42 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
4.67 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 1•6 years ago
|
||
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•6 years ago
|
||
Should be okay, but ensure that the REQUIRE_DEFERRED_MESSAGE_PROTECTION flag is not set on the message channel.
Comment 3•6 years ago
|
||
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)
Reporter | ||
Comment 4•6 years ago
|
||
(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.
Reporter | ||
Comment 5•6 years ago
|
||
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.
Reporter | ||
Comment 6•6 years ago
|
||
Reporter | ||
Comment 7•6 years ago
|
||
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.
Reporter | ||
Comment 8•6 years ago
|
||
Reporter | ||
Comment 9•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b82a94d7aab4f595c9fa9be4d79d6ff5d6714fd1
Updated•6 years ago
|
Comment 10•6 years ago
|
||
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•6 years ago
|
||
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'?
Reporter | ||
Comment 12•6 years ago
|
||
(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•6 years ago
|
||
Pushed by bobowencode@gmail.com: 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8fbb8da315c8 https://hg.mozilla.org/mozilla-central/rev/1f61e46990eb https://hg.mozilla.org/mozilla-central/rev/a5767bc129f8
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•2 years ago
|
Comment 15•10 months ago
|
||
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.
Reporter | ||
Comment 16•9 months ago
|
||
(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•9 months ago
|
||
(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
Reporter | ||
Comment 18•8 months ago
|
||
(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.
Description
•