Closed Bug 1066326 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: TimAbraldes, Assigned: TimAbraldes)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 6 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
Per email discussions
(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?
> 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.
Attached file 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
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
(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. :)
: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
(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?
Blocks: EME
(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 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)
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #8488255 - Attachment is obsolete: true
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.
Attachment #8497879 - Flags: feedback+
Attached patch Patch v3 (obsolete) — Splinter Review
: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
Attachment #8497879 - Attachment is obsolete: true
Attachment #8499871 - Flags: review?(rjesup)
Attachment #8499871 - Flags: review?(cpearce)
Attachment #8499871 - Flags: review?(bobowencode)
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
Attachment #8499871 - Flags: review?(rjesup) → review-
Attached patch Patch v4 (obsolete) — Splinter Review
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
Attachment #8499871 - Attachment is obsolete: true
Attachment #8499871 - Flags: review?(cpearce)
Attachment #8499871 - Flags: review?(bobowencode)
Attachment #8500738 - Flags: review?(rjesup)
Attachment #8500738 - Flags: review?(cpearce)
Attachment #8500738 - Flags: review?(bobowencode)
> @@ +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.
Status: NEW → ASSIGNED
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());
Attachment #8500738 - Flags: review?(cpearce) → review+
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.
Attachment #8500738 - Flags: review?(rjesup) → review+
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:"?
Attachment #8500738 - Flags: review?(bobowencode) → review+
Attached patch Patch v5 (obsolete) — Splinter Review
Addressed review comments, carrying forward r+

Try push:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7d667d91d4b3
Attachment #8500738 - Attachment is obsolete: true
Attachment #8501220 - Flags: review+
Attached patch Patch v6 (obsolete) — Splinter Review
This includes the `MAX_PATH * whitelistLen` change
Attachment #8501220 - Attachment is obsolete: true
Attachment #8501226 - Flags: review+
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);

?
Attached patch Patch v7Splinter Review
> 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.
Attachment #8501226 - Attachment is obsolete: true
Attachment #8501416 - Flags: review+
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).
(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.
(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
https://hg.mozilla.org/mozilla-central/rev/842efe1a471b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1098186
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.