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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bob Owen (:bobowen)]

See User Story.

Comment 1

[Bob Owen (:bobowen)]

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

Comment 2

[Bob Owen (:bobowen)]

Attachment: Use the USER_LOCKDOWN access token for GMP processes.

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?

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8518865 [details] [diff] [review]
Use the USER_LOCKDOWN access token for GMP processes.

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

Neat!

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 5

[Bob Owen (:bobowen)]

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

Comment 6

[Chris Peterson [:cpeterson]]

These bugs are necessary for vouching and sandboxing a third-party CDM.

Comment 7

[Bob Owen (:bobowen)]

Attachment: Use the USER_LOCKDOWN access token for GMP processes.

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.

Comment 8

[Bob Owen (:bobowen)]

Forgot the try push link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbc4ccf3282f

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 10

[Bob Owen (:bobowen)]

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.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 12

[Bob Owen (:bobowen)]

Attachment: Use the USER_LOCKDOWN access token for GMP processes. v2

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.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

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!

Comment 14

[Bob Owen (:bobowen)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/02cac0756bc1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/02cac0756bc1

Comment 16

[Bob Owen (:bobowen)]

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

Comment 17

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

> [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.

Comment 18

[Bob Owen (:bobowen)]

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

Comment 19

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

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 20

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 21

[Bob Owen (:bobowen)]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/941a8c8385eb