Use MITIGATION_WIN32K_DISABLE flag for GMP process.

REOPENED
Unassigned

Status

()

enhancement
P2
normal
REOPENED
Last year
9 months ago

People

(Reporter: bobowen, Unassigned)

Tracking

(Depends on 1 bug)

unspecified
mozilla61
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

No description provided.
Summary: Use MITIGATION_WIN32K_DISABLE flags for GMP process. → Use MITIGATION_WIN32K_DISABLE flag for GMP process.
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
Flags: needinfo?(aklotz)
See Also: → 1433065
Should be okay, but ensure that the REQUIRE_DEFERRED_MESSAGE_PROTECTION flag is not set on the message channel.
Flags: needinfo?(aklotz)
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)
(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.
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.
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.
Attachment #8962406 - Flags: review?(jmathies)
Attachment #8962404 - Flags: review?(jmathies) → review+
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
Attachment #8962406 - Flags: review?(jmathies) → review+
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'?
Attachment #8962407 - Flags: review?(jmathies) → review+
(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

Last year
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
Depends on: 1451270

Updated

9 months ago
Assignee: bobowencode → nobody
Priority: P1 → P2

Updated

9 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.