1027906 - Delayed token level for GMP processes should be more restrictive

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

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

We haven't done this yet because setting the delayed token to anything lower than `USER_RESTRICTED_SAME_ACCESS` causes IPC channel creation to fail, which means that the processes can't communicate with each other and eventually time out.

Comment 1

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

Changing the dependency to bug 1027902; I'll attach a patch there that accomplishes both what that bug is trying to achieve and what this bug is trying to achieve (it's easier to land both changes simultaneously than to try to break them apart)

Comment 2

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

Attachment: Patch v1

Rather than copy+paste Bug 1027902 comment 12 I'll just put this link here.

Comment 3

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

Comment on attachment 8473203 [details] [diff] [review]
Patch v1

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

With this patch I see current https://github.com/cpearce/gmp-clearkey master in decrypt-only mode fail to load, even when running in decrypt-only mode. I'm not sure how to debug this further. The PGMPParent::SendPGMPDecryptorConstructor() call is failing.

Comment 4

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

(In reply to Chris Pearce (:cpearce) from comment #3)
> Comment on attachment 8473203 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8473203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> With this patch I see current https://github.com/cpearce/gmp-clearkey master
> in decrypt-only mode fail to load, even when running in decrypt-only mode.
> I'm not sure how to debug this further. The
> PGMPParent::SendPGMPDecryptorConstructor() call is failing.

I'll take a look today/tomorrow/Friday with current master.

Comment 5

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

Attachment: Patch v2

cpearce - I'm not able to reproduce the issue you described.

I did discover that the EME plugin failed to load with my patch applied. I tried a number of things to get it loading again and the only thing that worked was changing our delayed token from `USER_LOCKDOWN` to `USER_RESTRICTED`. After doing this and successfully loading the DLL even once, I could change the token back to `USER_LOCKDOWN` and the EME test page would load just fine, no matter how many times I re-tested. It seems that Windows caches some information about the DLL. If that information is available in the cache, then we can load the DLL with a token of `USER_LOCKDOWN`. If that information is not available, we need a token of `USER_RESTRICTED` or better. This theory is corroborated by the fact that building the DLL is enough to cause loading it to start failing again.

This patch changes our delayed token level from `USER_LOCKDOWN` to `USER_RESTRICTED`. We'd still like to reach `USER_LOCKDOWN` in the future, but it'll be beneficial to land this much now and investigate the DLL loading issues later.

cpearce - As I said, I wasn't able to repro the issue you mentioned. Please check out this patch and let me know if it causes further issues for you.

Try run:
  https://tbpl.mozilla.org/?tree=Try&rev=9f4d28dc8030

Comment 6

[Bob Owen (:bobowen)]

Comment on attachment 8474029 [details] [diff] [review]
Patch v2

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

r=bobowen for the sandboxing parts.

I'm particularly interested that you're using registry rules.
Is this working in opt and debug builds?

Some possible changes below.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +75,5 @@
> +  ret = ret && (sandbox::SBOX_ALL_OK == result);
> +  result =
> +    mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_LOW);
> +  ret = ret && (sandbox::SBOX_ALL_OK == result);
> +  result =  mPolicy->SetAlternateDesktop(true);

nit: extra space

@@ +77,5 @@
> +    mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_LOW);
> +  ret = ret && (sandbox::SBOX_ALL_OK == result);
> +  result =  mPolicy->SetAlternateDesktop(true);
> +  ret = ret && (sandbox::SBOX_ALL_OK == result);
> +  return ret;

I notice that the existing calls to these SetSecurityLevelFor*  functions aren't checking the return value and would start the process anyway.
As has been mentioned before, it seems unlikely that any of these calls would fail, but I think we should fix that (possibly in a follow-up), as I think some can fail if used incorrectly so this might pick up future errors.

I've been debating whether we could just do:

if (sandbox::SBOX_ALL_OK != mPolicy->Wibble()) {
  return false;
}

But I think it probably is best to try and set up the whole policy, in case (as now) the return value is ignored.

I'm not sure I'm keen on this separate function per process type approach.
I'd prefer something like a static map of process type to policy-settings (wrapped up in a class) that could be passed to SandboxBroker::LaunchApp.
Anyway, even if anyone agreed with me :-), I think it's too late to change now given the urgency of EME.

@@ +151,5 @@
> +
> +  // The plugin process can't create named events, but we'll
> +  // make an exception for the events used in logging. Removing
> +  // this will break EME in debug builds.
> +  result = mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_SYNC,

If this is only needed in debug builds should we wrap this with a #ifdef?

@@ +164,5 @@
> +  // of at least one EME plugin.
> +  result = mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_REGISTRY,
> +                            sandbox::TargetPolicy::REG_ALLOW_READONLY,
> +                            L"HKEY_CURRENT_USER");
> +  ret = ret && (sandbox::SBOX_ALL_OK == result);

So, I'm assuming that registry rules are now working.
Has something been done to fix the issue I was having?

I also wonder whether some sort of list with a loop would be better, given the repetitious nature of adding rules and checking the return value.

Comment 7

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

Comment on attachment 8474029 [details] [diff] [review]
Patch v2

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

Works for me. Not sure what happened the other day.

Comment 8

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

Attachment: Patch v3

> I'm particularly interested that you're using registry rules.
> Is this working in opt and debug builds?

I've tested in debug and opt builds and the plugin seemed to load correctly in both.
 
> ::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
> @@ +75,5 @@
> > +  ret = ret && (sandbox::SBOX_ALL_OK == result);
> > +  result =
> > +    mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_LOW);
> > +  ret = ret && (sandbox::SBOX_ALL_OK == result);
> > +  result =  mPolicy->SetAlternateDesktop(true);
> 
> nit: extra space

Fixed.

> @@ +77,5 @@
> > +    mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_LOW);
> > +  ret = ret && (sandbox::SBOX_ALL_OK == result);
> > +  result =  mPolicy->SetAlternateDesktop(true);
> > +  ret = ret && (sandbox::SBOX_ALL_OK == result);
> > +  return ret;
> 
> I notice that the existing calls to these SetSecurityLevelFor*  functions
> aren't checking the return value and would start the process anyway.
> As has been mentioned before, it seems unlikely that any of these calls
> would fail, but I think we should fix that (possibly in a follow-up), as I
> think some can fail if used incorrectly so this might pick up future errors.

Yup! I agree that this should be fixed in a follow-up.

> I've been debating whether we could just do:
> 
> if (sandbox::SBOX_ALL_OK != mPolicy->Wibble()) {
>   return false;
> }
> 
> But I think it probably is best to try and set up the whole policy, in case
> (as now) the return value is ignored.

I also went back and forth on this. I agree that attempting to set up the whole policy (instead of early-returning in case of failures) makes sense.

> I'm not sure I'm keen on this separate function per process type approach.
> I'd prefer something like a static map of process type to policy-settings
> (wrapped up in a class) that could be passed to SandboxBroker::LaunchApp.
> Anyway, even if anyone agreed with me :-), I think it's too late to change
> now given the urgency of EME.

We could certainly explore this in the future! Existing code exists to be refactored and rewritten :)
 
> @@ +151,5 @@
> > +
> > +  // The plugin process can't create named events, but we'll
> > +  // make an exception for the events used in logging. Removing
> > +  // this will break EME in debug builds.
> > +  result = mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_SYNC,
> 
> If this is only needed in debug builds should we wrap this with a #ifdef?

Good call. Added a "#ifdef DEBUG"

> @@ +164,5 @@
> > +  // of at least one EME plugin.
> > +  result = mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_REGISTRY,
> > +                            sandbox::TargetPolicy::REG_ALLOW_READONLY,
> > +                            L"HKEY_CURRENT_USER");
> > +  ret = ret && (sandbox::SBOX_ALL_OK == result);
> 
> So, I'm assuming that registry rules are now working.
> Has something been done to fix the issue I was having?

I don't think anything has been done explicitly to make registry rules work. Let's chat elsewhere to see if we can figure out why these registry rules seem to work while the ones you worked with seemed to fail.

> I also wonder whether some sort of list with a loop would be better, given
> the repetitious nature of adding rules and checking the return value.

Sure, we could do that! I can file a follow-up for this as well; it'll probably become more of an issue once we start ratcheting down the permissions of our other types of child process.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8475624 [details] [diff] [review]
Patch v3

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

::: content/media/gmp/GMPProcessParent.cpp
@@ +48,5 @@
>    vector<string> args;
>    args.push_back(mGMPPath);
> +
> +#ifdef XP_WIN
> +  std::wstring_convert<std::codecvt_utf8<wchar_t>> converter;

Have you verified that this is guaranteed to be utf8? I believe it is actually the system encoding and you will need to do something more clever here.

Comment 10

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

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #9)
> Comment on attachment 8475624 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8475624 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/gmp/GMPProcessParent.cpp
> @@ +48,5 @@
> >    vector<string> args;
> >    args.push_back(mGMPPath);
> > +
> > +#ifdef XP_WIN
> > +  std::wstring_convert<std::codecvt_utf8<wchar_t>> converter;
> 
> Have you verified that this is guaranteed to be utf8? I believe it is
> actually the system encoding and you will need to do something more clever
> here.

The path passed to the `GMPProcessParent` constructor is utf8 [1] which means that `GMPProcessParent::mGMPPath` is utf8 [2].

[1] https://mxr.mozilla.org/mozilla-central/source/content/media/gmp/GMPParent.cpp?rev=8058658b0a28#136
[2] https://mxr.mozilla.org/mozilla-central/source/content/media/gmp/GMPProcessParent.cpp?rev=51419e405e61#29

Comment 11

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

Link to try run:
  https://tbpl.mozilla.org/?tree=Try&rev=247cd3dacc71

Comment 12

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

Comment on attachment 8475624 [details] [diff] [review]
Patch v3

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

r+ with #ifdefs around the unused vectors, or remove.  #ifdef RESERVED_FOR_FUTURE_SANDBOXING_NEEDS or whatever.
Also, verify answer to bent's question about utf8

Comment 13

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

Attachment: Patch v4

Updated patch to apply to current m-c and addressed review comment.

Comment 14

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

Comment on attachment 8481582 [details] [diff] [review]
Patch v4

OK so we've got review on the content/media/gmp/* and the security/sandbox/win/* pieces.

I spoke with :cjones who is the module owner for ipc/glue/* and he suggested poking :bent again.

Poke!

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8481582 [details] [diff] [review]
Patch v4

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

Oops, sorry. Thanks for verifying.

::: content/media/gmp/GMPProcessParent.cpp
@@ +50,5 @@
>    vector<string> args;
>    args.push_back(mGMPPath);
> +
> +#ifdef XP_WIN
> +  std::wstring_convert<std::codecvt_utf8<wchar_t> > converter;

Nit: s/> >/>>/

Comment 16

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

Attachment: Patch v5

Thanks everyone!

Here's a link to another try run I had submitted (not quite the latest patch):
  https://tbpl.mozilla.org/?tree=Try&rev=317ff9f18660

https://hg.mozilla.org/integration/mozilla-inbound/rev/97229b790012

Comment 17

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

Updating the title since we didn't actually quite make it to USER_LOCKDOWN :)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97229b790012

Comment 19

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

Comment on attachment 8481730 [details] [diff] [review]
Patch v5

Approval Request Comment

[Feature/regressing bug #]: GMP and EME plugin process sandboxing support was added in bug 985252. The sandbox enabled in that bug is not very restrictive. This patch tightens the security of our sandbox for GMP and EME plugin processes. This includes OpenH264.

[User impact if declined]: Our OpenH264 plugin processes will not be meaningfully sandboxed.

[Describe test coverage new/current, TBPL]: This landed on central Fri Aug 29 and was uplifted to Aurora on Tue Sep 02. It has as much test coverage as our GMP and EME plugins have (I'm not sure how much coverage that is).

[Risks and why]: Risks are that the OpenH264 plugin could fail to load in some circumstances due to unforeseen interactions with the new sandbox policies or bugs. The reason to uplift this patch is to provide meaningful protection against malicious or buggy H264 streams. The sandbox as it exists before this patch is not very restrictive; it is possible to perform all sorts of file and probably registry accesses from within the sandbox.

[String/UUID change made/needed]: None

Comment 20

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

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #19)
> [Feature/regressing bug #]: GMP and EME plugin process sandboxing support
> was added in bug 985252. The sandbox enabled in that bug is not very
> restrictive. This patch tightens the security of our sandbox for GMP and EME
> plugin processes. This includes OpenH264.

Is the security of the sandbox without this fix worse than it was in Firefox 32 before bug 985252 landed?

> [User impact if declined]: Our OpenH264 plugin processes will not be
> meaningfully sandboxed.
> 
> [Describe test coverage new/current, TBPL]: This landed on central Fri Aug
> 29 and was uplifted to Aurora on Tue Sep 02. It has as much test coverage as
> our GMP and EME plugins have (I'm not sure how much coverage that is).

Do you have ideas on how we can effectively test the sandbox? Can you work with QE to create a test plan?

> [Risks and why]: Risks are that the OpenH264 plugin could fail to load in
> some circumstances due to unforeseen interactions with the new sandbox
> policies or bugs. The reason to uplift this patch is to provide meaningful
> protection against malicious or buggy H264 streams. The sandbox as it exists
> before this patch is not very restrictive; it is possible to perform all
> sorts of file and probably registry accesses from within the sandbox.

I think this means that with OpenH264 being new in 33 this will potentially be a new attack vector. Can you confirm?

Comment 21

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

(In reply to Lawrence Mandel [:lmandel] from comment #20)
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #19)
> > [Feature/regressing bug #]: GMP and EME plugin process sandboxing support
> > was added in bug 985252. The sandbox enabled in that bug is not very
> > restrictive. This patch tightens the security of our sandbox for GMP and EME
> > plugin processes. This includes OpenH264.
> 
> Is the security of the sandbox without this fix worse than it was in Firefox
> 32 before bug 985252 landed?

Before bug 985252 landed, there was no sandbox for GMP/EME plugins. After bug 985252 landed, there was a sandbox but it didn't restrict very much. The patch in this bug tightens the sandbox policies.

So the short answer is "we're better off with bug 985252 having landed than not having landed, but it didn't give us as much security as we would have liked."

> > [User impact if declined]: Our OpenH264 plugin processes will not be
> > meaningfully sandboxed.
> > 
> > [Describe test coverage new/current, TBPL]: This landed on central Fri Aug
> > 29 and was uplifted to Aurora on Tue Sep 02. It has as much test coverage as
> > our GMP and EME plugins have (I'm not sure how much coverage that is).
> 
> Do you have ideas on how we can effectively test the sandbox? Can you work
> with QE to create a test plan?

Luckily our GMP plugins don't have a huge API that we have to test. They really just decode video, so we could just do a manual test of one of the OpenH264 consumers under as many conditions as we can:
  Each version of Windows we can get our hands on (XP through 8.1)
  "Run as Administrator" and as regular user
Also we should do one test where the users's profile contains a character that is not in the system codepage.

> > [Risks and why]: Risks are that the OpenH264 plugin could fail to load in
> > some circumstances due to unforeseen interactions with the new sandbox
> > policies or bugs. The reason to uplift this patch is to provide meaningful
> > protection against malicious or buggy H264 streams. The sandbox as it exists
> > before this patch is not very restrictive; it is possible to perform all
> > sorts of file and probably registry accesses from within the sandbox.
> 
> I think this means that with OpenH264 being new in 33 this will potentially
> be a new attack vector. Can you confirm?

OpenH264 definitely provides a new attack vector. My understanding is that its use will initially be pretty limited, so maybe we're OK with that exposure. Uplifting this fix will definitely decrease its appeal as a target for attacks. If it causes unforeseen issues for OpenH264 then that plugin will be less usable for whoever is affected (the plugin might fail to load, for example).

Comment 22

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

Comment on attachment 8481730 [details] [diff] [review]
Patch v5

I'm going to approve this on beta as this improves the security for our users for this new feature. We should also have enough time to test and flush out any big issues.

ni on Tracy and Anthony as this change will be in 33 and 34. Do we have a test plan for the GMP sandbox or OpenH264 that we can use to test these changes? Assuming we have a test plan, it may also need to be modified to incorporate these changes.

Comment 23

[Tracy Walker [:tracy]]

Nils is on PTO, but he is owner of H264. 

Leaving myself and Anthony ni's for Nils to clear when he is able to answer.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0af2575571f3

Comment 25

[Nils Ohlmeier [:drno]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #21)
> (In reply to Lawrence Mandel [:lmandel] from comment #20)
> > Do you have ideas on how we can effectively test the sandbox? Can you work
> > with QE to create a test plan?
> 
> Luckily our GMP plugins don't have a huge API that we have to test. 

Not sure if I would agree with that. A quick a dirty counting of the functions in the gmp-api header files tells me the API is around 170 functions. I would call that pretty big from a testing perspective.

> They
> really just decode video, so we could just do a manual test of one of the
> OpenH264 consumers under as many conditions as we can:
>   Each version of Windows we can get our hands on (XP through 8.1)
>   "Run as Administrator" and as regular user

Sure, I can do that.

> Also we should do one test where the users's profile contains a character
> that is not in the system codepage.

I'm assuming you mean a user setting which is actually read by GMP, and not just any setting right? Can you suggest a good candidate for such a test?

I think we have to distinguish here a couple of things:

A) Testing of the the OpenH264 plugin: we are manually testing H264 calls between FF versions and across OS's. But that is mostly a functional test. This is also limited to manual testing, because of the licensing restrictions around the OpenH264 plugin.
  We don't have any other device then a FF browser to generate a H264 stream.
  We don't have any means to inject a H264 stream with malicious content into the GMP decoding plugin (which would be one possible attack scenario).
  I don't know how I would verify that the OpenH264 plugin is not reading files from my computer or writing data to the hard disk.

B) Testing of the GMP sandbox: this has not been on the QE road map at all so far as far as I know. For my perspective testing the API would involve writing our own test plugins which try to break out of the sandbox. As this goes into more of a security testing, maybe Matt knows anything about this?

Comment 26

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Nils Ohlmeier [:drno] from comment #25)
> B) Testing of the GMP sandbox: this has not been on the QE road map at all
> so far as far as I know. For my perspective testing the API would involve
> writing our own test plugins which try to break out of the sandbox. As this
> goes into more of a security testing, maybe Matt knows anything about this?

I filed bug 1054621 for this, but I haven't looked into it in detail yet.

Comment 27

[Nils Ohlmeier [:drno]]

(In reply to Jed Davis [:jld] from comment #26)
> I filed bug 1054621 for this, but I haven't looked into it in detail yet.

Great. Then we will track that effort separately in that ticket, and verify here "only" that this change does not have negative impact on the existing functionality for the OpenH264 plugin.

Comment 28

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

> > Luckily our GMP plugins don't have a huge API that we have to test. 
> 
> Not sure if I would agree with that. A quick a dirty counting of the
> functions in the gmp-api header files tells me the API is around 170
> functions. I would call that pretty big from a testing perspective.

I meant that the plugins don't expose many functions. I might be wrong, but I think that they only expose `GMPInit`, `GMPGetAPI`, and `GMPShutdown`, which would mean that successfully decoding video with a plugin effectively tests the whole API for that plugin.

> > They
> > really just decode video, so we could just do a manual test of one of the
> > OpenH264 consumers under as many conditions as we can:
> >   Each version of Windows we can get our hands on (XP through 8.1)
> >   "Run as Administrator" and as regular user
> 
> Sure, I can do that.
> 
> > Also we should do one test where the users's profile contains a character
> > that is not in the system codepage.
> 
> I'm assuming you mean a user setting which is actually read by GMP, and not
> just any setting right? Can you suggest a good candidate for such a test?

There was some question during development of this patch about whether we are correctly dealing with unicode paths. The OpenH264 plugin will be located at %APPDATA%\Mozilla\Firefox\Profiles\[Profile name here]\gmp-gmpopenh264 so I was suggesting a test where either the Windows profile or the Firefox profile has a character in it that is not in the system codepage (e.g. a Chinese character when the system codepage is CP1252). That way we can test that we're handling unicode paths properly.


> I think we have to distinguish here a couple of things:
> 
> A) Testing of the the OpenH264 plugin: we are manually testing H264 calls
> between FF versions and across OS's. But that is mostly a functional test.
> This is also limited to manual testing, because of the licensing
> restrictions around the OpenH264 plugin.
>   We don't have any other device then a FF browser to generate a H264 stream.
>   We don't have any means to inject a H264 stream with malicious content
> into the GMP decoding plugin (which would be one possible attack scenario).
>   I don't know how I would verify that the OpenH264 plugin is not reading
> files from my computer or writing data to the hard disk.

Functional testing is all I would expect for this bug. I don't think we could have accidentally made the sandbox less restrictive in this patch, and we'll follow up in bug 1054621 about verifying that we actually restrict everything we think we restrict.