1066326 - Ensure that Output Protection Manager APIs work from within the EME sandbox

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Attachment: Patch v1

Per email discussions

Comment 1

[Bob Owen (:bobowen)]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #0)
> Created attachment 8488255 [details] [diff] [review]
> Patch v1I think those load should be inside the 
> I think those load should be inside the 
> Per email discussions

I'm sure you meant to put those loads inside the XP_WIN #if. :)

Is there a way that we can lock down those dlls to just the functions that are required or are we not worried about access to other functions from them?

Comment 2

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

> I'm sure you meant to put those loads inside the XP_WIN #if. :)

Of course! :)
 
> Is there a way that we can lock down those dlls to just the functions that
> are required or are we not worried about access to other functions from them?

I've gone back and forth about this.

Even if we prevent specific functions from being executed from the DLL, we can't stop a malicious entity from injecting identical machine code (or building it into the plugin in the case of malicious plugins) and jumping to that.

If we're trying to make it harder for EME and GMP designers to casually collect data about our users' machines, then preventing access to specific functions might help (though, as mentioned above, we can't stop them from executing identical machine code).

ISTM that trying to restrict access to particular functions within a DLL is
  a) A lot of effort
  b) Not going to meaningfully restrict what a plugin process can do
As such it seems like it's not worth exploring.

If a) is untrue (maybe chromium has a mechanism for this), then this seems reasonable to investigate.

Comment 3

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

Attachment: d3d9.txt

This is the list of symbols exported by D3D9.dll, in case this helps us decide whether we want to try to limit access to some of its functions

Comment 4

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

Attachment: Symbols exported by dxva2.DLL

This is the list of symbols exported by dxva2.dll, in case this helps us decide whether we want to try to limit access to some of its functions

Comment 5

[Bob Owen (:bobowen)]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #2)

> Even if we prevent specific functions from being executed from the DLL, we
> can't stop a malicious entity from injecting identical machine code (or
> building it into the plugin in the case of malicious plugins) and jumping to
> that.
> 
> If we're trying to make it harder for EME and GMP designers to casually
> collect data about our users' machines, then preventing access to specific
> functions might help (though, as mentioned above, we can't stop them from
> executing identical machine code).
> 
> ISTM that trying to restrict access to particular functions within a DLL is
>   a) A lot of effort
>   b) Not going to meaningfully restrict what a plugin process can do
> As such it seems like it's not worth exploring.
> 
> If a) is untrue (maybe chromium has a mechanism for this), then this seems
> reasonable to investigate.

Right, it doesn't seem worth the effort if it can be relatively easily circumvented.
I imagine we'll need to get other people's views on whether any of this causes privacy issues.
Although, I assume these would exist even without this patch.

The only thing I've seen in the Chromium sandbox is a list of dlls to unload, because they are know to cause problems.
I haven't seen a per function mechanism.

Because of the job object settings we are setting JOB_OBJECT_UILIMIT_DISPLAYSETTINGS, which may well stop some of the Set functions from working.

Got to love that DestroyPhysicalMonitor function name. :)

Comment 6

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

:cpearce suggested via email that we allow a list of DLLs to be included in the .info file. The GMP plugin process would pre-load the DLLs from that list that also appear on an internal whitelist.

I'm thinking we can follow the existing syntax for the .info file, so something like this:

[ExamplePlugin.info]
Name: Example EME plugin
Description: This is quite a plugin.
Version: 0.1
APIs: decode-video[org.w3.example:h264], decode-audio[org.w3.example:aac], eme-decrypt[org.w3.example]
DLLs: dxva2.dll, d3d9.dll

Comment 7

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

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #6)
> :cpearce suggested via email that we allow a list of DLLs to be included in
> the .info file. The GMP plugin process would pre-load the DLLs from that
> list that also appear on an internal whitelist.

Shall we do this work in this bug, or in another?

Comment 8

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

(In reply to Chris Pearce (:cpearce) from comment #7)
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #6)
> > :cpearce suggested via email that we allow a list of DLLs to be included in
> > the .info file. The GMP plugin process would pre-load the DLLs from that
> > list that also appear on an internal whitelist.
> 
> Shall we do this work in this bug, or in another?

I'm happy to do the work in this bug! I'll post something here today or tomorrow

Comment 9

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

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

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

::: content/media/gmp/GMPChild.cpp
@@ +237,5 @@
>  
> +  // Pre-load DLLs that need to be used by the EME plugin but that can't be
> +  // loaded after the sandbox has started
> +  LoadLibraryW(L"dxva2.dll");
> +  LoadLibraryW(L"D3D9.dll");

We'll also need:

  LoadLibraryW(L"msmpeg2vdec.dll"); // H.264 decoder
  LoadLibraryW(L"msmpeg2adec.dll"); // AAC decoder (on Windows 7)
  LoadLibraryW(L"MSAudDecMFT.dll"); // AAC decoder (on Windows 8)

Comment 10

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

Attachment: Patch v2

This pre-loads DLLs specified in a "Libraries:" section of the plugin's info file.

Still to do:
  Cleanup (including making sure that all this code is correctly #ifdef'd into Windows-only sections)
  Only load DLLs that are in a pre-approved whitelist

On the second point, I'm not sure exactly where we want to keep the whitelist of DLLs or what our selection criteria should be.

Comment 11

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

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

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

With this patch and my GMP's .info file containing the libs I need, AAC and H.264 decoding using WMF work again.

::: content/media/gmp/GMPChild.cpp
@@ +304,5 @@
> +    if (0 == strncmp("Libraries:", buf, 10)) {
> +      char* start = buf+10;
> +      char* tok = strtok(start, ", ");
> +      while (tok) {
> +        LoadLibraryA(tok);

I'd say we can hard code the whitelist here, and check to see if tok is in the whitelist and only load the library if appears in the whitelist.

Comment 12

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

Attachment: Patch v3

:jesup - are you the person to ask for reviews in content/media/gmp?
:bobowen - requesting review since this is Windows sandbox related
:cpearce - I didn't copy down the pastebin you had sent with whitelist code so I rewrote it. Let me know if this looks OK

Try push:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=90acfe187afa

Comment 13

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

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

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

r- for strtok

::: content/media/gmp/GMPChild.cpp
@@ +290,5 @@
> +       "d3d9.dll",
> +       "dxva2.dll",
> +       "msauddecmft.dll"
> +       "msmpeg2adec.dll",
> +       "msmpeg2vdec.dll",

comment each as to why?

@@ +292,5 @@
> +       "msauddecmft.dll"
> +       "msmpeg2adec.dll",
> +       "msmpeg2vdec.dll",
> +    };
> +  static const int whitelistLen = sizeof(whitelist) / sizeof(char*);

prefer sizeof(list)/sizeof(list[0]). Avoids problems if the list type changes; or if the size of an array entry is padded up from the size of a raw struct.

@@ +303,5 @@
> +
> +  std::ifstream stream;
> +  stream.open(path.get());
> +  if (!stream.good()) {
> +    return false;

log? (if you want) or if (NS_WARN_IF(...)) {

@@ +306,5 @@
> +  if (!stream.good()) {
> +    return false;
> +  }
> +
> +  char buf[128];

128?  Is there a reason that's a good/safe size?

@@ +316,5 @@
> +    for (char* lower = buf; *lower != 0; lower++) {
> +      *lower = tolower(*lower);
> +    }
> +    if (0 == strncmp("libraries:", buf, 10)) {
> +      char* start = buf+10;

10 == sizeof("libraries:"), use that, not an uncommented constant

@@ +317,5 @@
> +      *lower = tolower(*lower);
> +    }
> +    if (0 == strncmp("libraries:", buf, 10)) {
> +      char* start = buf+10;
> +      char* tok = strtok(start, ", ");

Never, ever use strtok! it's not re-entrant or threadsafe.  strtok_r() or some other parsing functions in our tree.

@@ +325,5 @@
> +            LoadLibraryA(tok);
> +            break;
> +          }
> +        }
> +        tok = strtok(NULL, ", ");

nullptr

Comment 14

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

Attachment: Patch v4

This patch addresses the review comments from the previous patch. Also it hopefully fixes some build issues on platforms other than Windows.

I didn't add the comments for the DLLs because I'm not sure what would be appropriate to write there. Maybe :cpearce can suggest something?

New try push:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=780468d12634

Comment 15

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

> @@ +306,5 @@
> > +  if (!stream.good()) {
> > +    return false;
> > +  }
> > +
> > +  char buf[128];
> 
> 128?  Is there a reason that's a good/safe size?

Oops, I changed this to MAX_PATH but after re-reading what's going on here (we're getting a line that could contain an arbitrary number of DLLs), it's clear that that doesn't make sense.

We could do something like (MAX_PATH * number of items in whitelist) though in practice the line will be much shorter than that.

Comment 16

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

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

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

r+ with bugs/nit addressed...

::: content/media/gmp/GMPChild.cpp
@@ +288,5 @@
> +  static const char* whitelist[] =
> +    {
> +       "d3d9.dll",
> +       "dxva2.dll",
> +       "msauddecmft.dll"

"msauddecmft.dll",

Missing ',' at end of line, meaning this string is actually "msauddecmft.dllmsmpeg2adec.dll"...

@@ +318,5 @@
> +    for (char* lower = buf; *lower != 0; lower++) {
> +      *lower = tolower(*lower);
> +    }
> +    static const char* prefix = "libraries";
> +    static const int prefixLen = sizeof(prefix);

static const int prefixLen = strlen(prefix);

sizeof(prefix) == sizeof(int).

@@ +331,5 @@
> +        }
> +      }
> +      break;
> +    }
> +  } while(!stream.eof());

nit:

while (!stream.eof());

Comment 17

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

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

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

::: content/media/gmp/GMPChild.cpp
@@ +290,5 @@
> +       "d3d9.dll",
> +       "dxva2.dll",
> +       "msauddecmft.dll"
> +       "msmpeg2adec.dll",
> +       "msmpeg2vdec.dll",

I'd still like to see a description (short) about why each of these is needed.

Comment 18

[Bob Owen (:bobowen)]

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

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

r=bobowen with these as well.

I'm far less experienced in C++ than all of you, so I'm still surprised that the string handling is so painful.
Doesn't look like std::string helps much for this use-case either.

::: content/media/gmp/GMPChild.cpp
@@ +16,5 @@
>  #include "gmp-video-encode.h"
>  #include "GMPPlatform.h"
>  #include "mozilla/dom/CrashReporterChild.h"
> +#include <fstream>
> +#include "nsCRT.h"

I think these are only used for XP_WIN.

@@ +99,5 @@
> +#endif
> +              nsCOMPtr<nsIFile>& aLibFile)
> +{
> +  nsAutoString baseName;
> +  GetFileBase(aPluginPath, aLibFile, baseName);

This call will need aLibDirectory on Mac.

@@ +290,5 @@
> +       "d3d9.dll",
> +       "dxva2.dll",
> +       "msauddecmft.dll"
> +       "msmpeg2adec.dll",
> +       "msmpeg2vdec.dll",

I think some explanation for each of these would be good as well.

@@ +317,5 @@
> +    }
> +    for (char* lower = buf; *lower != 0; lower++) {
> +      *lower = tolower(*lower);
> +    }
> +    static const char* prefix = "libraries";

Should this be "libraries:"?

Comment 19

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

Attachment: Patch v5

Addressed review comments, carrying forward r+

Try push:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7d667d91d4b3

Comment 20

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

Attachment: Patch v6

This includes the `MAX_PATH * whitelistLen` change

Comment 21

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

Comment on attachment 8501226 [details] [diff] [review]
Patch v6

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

::: content/media/gmp/GMPChild.cpp
@@ +313,5 @@
> +    NS_WARNING("Failure opening info file for required DLLs");
> +    return false;
> +  }
> +
> +  char buf[MAX_PATH * whitelistLen];

You don't pass the same length to getline(), you pass just MAX_PATH there.

You could just use:

string line;
getline(stream, line);

?

Comment 22

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

Attachment: Patch v7

> You don't pass the same length to getline(), you pass just MAX_PATH there.

Oops, sloppy.
 
> You could just use:
> 
> string line;
> getline(stream, line);

Done, though to be able to continue using nsCRT::strtok I had to strdup the line.

Comment 23

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

Hmmm... My test gmp-clearkey started to fail to load yesterday when using WMF, even with your patch. It was working with your patch last week, but when I updated last night the PR_LoadLibrary call started failing when my GMP is statically linked to mfplat.lib. What changed in the past few days that would break this? I can go through and dynamically link to all the functions I need... That's a bit of a hassle though. The PR_LoadLibrary command still fails if I delay loading the mfplat.dll (I added it to the whitelist).

Comment 24

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

(In reply to Chris Pearce (:cpearce) from comment #23)
> Hmmm... My test gmp-clearkey started to fail to load yesterday when using
> WMF, even with your patch.

False alarm! Thankfully! I failed to correctly resolve merge conflicts. My bad.

Comment 25

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

(In reply to Chris Pearce (:cpearce) from comment #24)
> (In reply to Chris Pearce (:cpearce) from comment #23)
> > Hmmm... My test gmp-clearkey started to fail to load yesterday when using
> > WMF, even with your patch.
> 
> False alarm! Thankfully! I failed to correctly resolve merge conflicts. My
> bad.

*phew!*

I was a little worried about all those valgrind failures even though it seemed like there was no way that my patch caused them, so I did one final try push:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b19c2d294c89

And pushed to inbound:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/842efe1a471b

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/842efe1a471b

Comment 27

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

Mass update firefox-status to track EME uplift.