Closed Bug 1094370 Opened 5 years ago Closed 5 years ago

Move to using the USER_LOCKDOWN token for the EME/GMP sandbox.

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

User Story

Now that we are pre-loading white-listed DLLs that the GMPs say they require, we believe that we can move to using the most restrictive sandboxing access token, USER_LOCKDOWN.

Attachments

(1 file, 2 obsolete files)

See User Story.
Tim, looks like you were right over being able to get to USER_LOCKDOWN.

All the GeckoMediaPlugins gtests pass locally and the clearkey plugin runs as well.

The try push in comment 1 looks OK.

Chris,
Could you make sure this doesn't cause problems for any versions you have locally.
Are there any other tests I need to run on try?
Attachment #8518865 - Flags: review?(tabraldes)
Attachment #8518865 - Flags: review?(cpearce)
Comment on attachment 8518865 [details] [diff] [review]
Use the USER_LOCKDOWN access token for GMP processes.

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

Neat!
Attachment #8518865 - Flags: review?(tabraldes) → review+
Comment on attachment 8518865 [details] [diff] [review]
Use the USER_LOCKDOWN access token for GMP processes.

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

WMF usage seems to be broken by this patch.

There's something odd happening when I apply this patch. Using my gmp-clearkey project, if I have built gmp-clearkey with all options in stdafx.h turned off, it works fine. But if I #define on TEST_DECODING, the child process fails to start, and the handler GMPChid::RecvStartPlugin subsequently fails, as we fail to load the GMP DLL. Even if I turn off WMF usage in the gmp-clearkey stdafx.h and rebuild the DLL, subsequent attempts to load the GMP still fails to load the plugin library. Maybe it's getting blacklisted?
Attachment #8518865 - Flags: review?(cpearce) → review-
(In reply to Chris Pearce (:cpearce) from comment #4)
> Comment on attachment 8518865 [details] [diff] [review]
> Use the USER_LOCKDOWN access token for GMP processes.
> 
> Review of attachment 8518865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> WMF usage seems to be broken by this patch.
> 
> There's something odd happening when I apply this patch. Using my
> gmp-clearkey project, if I have built gmp-clearkey with all options in
> stdafx.h turned off, it works fine. But if I #define on TEST_DECODING, the
> child process fails to start, and the handler GMPChid::RecvStartPlugin
> subsequently fails, as we fail to load the GMP DLL. Even if I turn off WMF
> usage in the gmp-clearkey stdafx.h and rebuild the DLL, subsequent attempts
> to load the GMP still fails to load the plugin library. Maybe it's getting
> blacklisted?

Right, I'm getting the same thing, didn't realise there were various #defines you had to uncomment.
I was a bit confused at first, as I was getting a different failure, but it was because e10s had been enabled.

I don't see anything extra getting blocked with the logging that will be available after bug 928044 lands.
Although once it fails it does then block the loading of dbghelp.dll, so I'll see if I can fix that.

I'm going to look at the real deal now though.
These bugs are necessary for vouching and sandboxing a third-party CDM.
Blocks: eme-m2
No longer blocks: eme-m2
After a fair bit of assembler debugging, I think I've finally got to the bottom of this.

As it tries to load the dll, because it is a side-by-side assembly it tries to create an activation context for the associated manifest, which fails.
If the dll has already been loaded then it works even with USER_LOCKDOWN, so it appears to cache this somehow.

If I create an activation context before loading the dll it works.

So, it wasn't to do with the TEST_DECODING as such, it is just that that caused the 
"full" loading behaviour.
Removing TEST_DECODING and attempting to load it caused the same problem and explains the behaviour you were seeing.

Hopefully this patch works for you as well, I'm not sure how much of a hack it will be considered.
Attachment #8518865 - Attachment is obsolete: true
Attachment #8551397 - Flags: feedback?(cpearce)
Comment on attachment 8551397 [details] [diff] [review]
Use the USER_LOCKDOWN access token for GMP processes.

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

My decoding GMP still seems to work.
Attachment #8551397 - Flags: feedback?(cpearce) → feedback+
Comment on attachment 8551397 [details] [diff] [review]
Use the USER_LOCKDOWN access token for GMP processes.

This solution seems to work, but I'm not sure whether it will be seen as an acceptable solution.

Also, I don't know whether there is an easier way to do the path widening.
I tried using NS string stuff, but as this is in the exe I had to use the external API and ran into conflicts.
Attachment #8551397 - Flags: review?(aklotz)
Comment on attachment 8551397 [details] [diff] [review]
Use the USER_LOCKDOWN access token for GMP processes.

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

Looks good but I'd like to see a couple of things tidied up.

::: dom/media/gmp/GMPLoader.cpp
@@ +176,5 @@
> +  ACTCTX actctx = {sizeof(actctx)};
> +  actctx.dwFlags = ACTCTX_FLAG_RESOURCE_NAME_VALID;
> +  int wideLen = MultiByteToWideChar(CP_ACP, 0, aLibPath, -1, nullptr, 0);
> +  wchar_t* widePath = new wchar_t[wideLen];
> +  MultiByteToWideChar(CP_ACP, 0, aLibPath, -1, widePath, wideLen);

Can you use NS_CopyNativeToUnicode instead of calling MultiByteToWideChar directly, please? See xpcom/io/nsNativeCharsetUtils.h

@@ +179,5 @@
> +  wchar_t* widePath = new wchar_t[wideLen];
> +  MultiByteToWideChar(CP_ACP, 0, aLibPath, -1, widePath, wideLen);
> +  actctx.lpSource = widePath;
> +  actctx.lpResourceName = ISOLATIONAWARE_MANIFEST_RESOURCE_ID;
> +  HANDLE hActCtx = CreateActCtx(&actctx);

I'd prefer that we use RAII for hActCtx (there's already a handle leak in this code if PR_LoadLibraryWithFlags fails). Probably your best option here is to use mfbt/Scoped.h and create traits for an activation context handle. See an example in xpcom/io/FileUtilsWin.cpp.
Attachment #8551397 - Flags: review?(aklotz) → review-
I decided to return false if MultiByteToWideChar fails, as the library loading is going to fail anyway if we can't convert the path.

Thanks for all your help on IRC.
Attachment #8551397 - Attachment is obsolete: true
Attachment #8553883 - Flags: review?(aklotz)
Comment on attachment 8553883 [details] [diff] [review]
Use the USER_LOCKDOWN access token for GMP processes. v2

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

Thanks, Bob!
Attachment #8553883 - Flags: review?(aklotz) → review+
https://hg.mozilla.org/mozilla-central/rev/02cac0756bc1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8553883 [details] [diff] [review]
Use the USER_LOCKDOWN access token for GMP processes. v2

Given cpeterson's point in Bug 1121479 Comment 9, I think it makes sense for this to be uplifted as well.

Approval Request Comment
[Feature/regressing bug #]: This is an improvement to the GMP process sandbox on Windows, which is already in Live.

[User impact if declined]: If a vulnerability is found in a GMP, we will not have the better protection that the USER_LOCKDOWN access token level provides.
This is probably more important for Windows XP that doesn't have the protection of the Windows Integrity Mechanism, which was introduced with Vista.

[Describe test coverage new/current, TreeHerder]: The sandbox is not tested directly, but there are GMP tests for webrtc and eme in tree, which helps ensure that this sandbox feature doesn't cause a regression.

[Risks and why]: Low to medium: the change itself is small, but it is possible that it might cause a regression for GMPs.

[String/UUID change made/needed]: None
Attachment #8553883 - Flags: approval-mozilla-aurora?
> [Describe test coverage new/current, TreeHerder]: The sandbox is not tested
> directly, but there are GMP tests for webrtc and eme in tree, which helps
> ensure that this sandbox feature doesn't cause a regression.

Please hand-test with OpenH264: add an integer pref named media.navigator.video.preferred_codec with a value of 126 to two browsers using this code, then make a call between them (Hello, talky.io, apprtc).  I don't expect any problems, but the fake GMP plugin in the tree does not exercise everything a real OpenH264 plugin does.
Flags: needinfo?(bobowen.code)
(In reply to Randell Jesup [:jesup] from comment #17)
> > [Describe test coverage new/current, TreeHerder]: The sandbox is not tested
> > directly, but there are GMP tests for webrtc and eme in tree, which helps
> > ensure that this sandbox feature doesn't cause a regression.
> 
> Please hand-test with OpenH264: add an integer pref named
> media.navigator.video.preferred_codec with a value of 126 to two browsers
> using this code, then make a call between them (Hello, talky.io, apprtc).  I
> don't expect any problems, but the fake GMP plugin in the tree does not
> exercise everything a real OpenH264 plugin does.

I don't seem to be able to get Hello or talky.io to work between my Linux laptop and my Windows desktop.

However, apprtc worked fine and I could see the gmpopenh264.dll loaded in the GMP process, which was using the USER_LOCKDOWN access token level.
The video looked fine to me.
Flags: needinfo?(bobowen.code)
Likely the problems you saw were bug 1126036 (which is now fixed on inbound).  You can also test using webrtc-landing/pc_test.html

Sounds good though.
Comment on attachment 8553883 [details] [diff] [review]
Use the USER_LOCKDOWN access token for GMP processes. v2

Approving for Aurora as we're pushing on our sandbox. My understanding is that this change is not EME specific. If it is, we should defer to 38.
Attachment #8553883 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #20)
> Comment on attachment 8553883 [details] [diff] [review]
> Use the USER_LOCKDOWN access token for GMP processes. v2
> 
> Approving for Aurora as we're pushing on our sandbox. My understanding is
> that this change is not EME specific. If it is, we should defer to 38.

Thanks Lawrence.
That is correct, this is not EME specific.
You need to log in before you can comment on or make changes to this bug.