Closed Bug 1060179 Opened 10 years ago Closed 10 years ago

[EME] Expose node id to GMP

Categories

(Core :: Audio/Video, defect)

x86_64
All
defect
Not set
normal

Tracking

()

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

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 11 obsolete files)

996 bytes, patch
ajones
: review+
Details | Diff | Splinter Review
34.51 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
5.31 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
26.71 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
13.86 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
22.41 KB, patch
jesup
: review+
Details | Diff | Splinter Review
11.69 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
4.41 KB, patch
rbarnes
: feedback-
Details | Diff | Splinter Review
7.52 KB, patch
jesup
: review+
Details | Diff | Splinter Review
We must expose a node id to the GMP in GMPDecryptorHost::GetNodeId().
Depends on: 1035637
I think this is how Chromium does machine ids:

https://code.google.com/p/rlz/source/browse/trunk/win/lib/machine_id_win.cc
https://code.google.com/p/rlz/source/browse/trunk/mac/lib/machine_id_mac.cc
(There is no Linux implementation)

We'd need to get the parent process to pass over the origin id (generated in bug 1035637) and hash it with the machine id.
(In reply to [PTO until 15 Sep] Chris Pearce (:cpearce) from comment #1)
> We'd need to get the parent process to pass over the origin id (generated in
> bug 1035637) and hash it with the machine id.

We need just the salt associated with the origin id, not the origin id itself.
Assignee: nobody → cpearce
For EME, we'll segregate EME plugin instances by their "node id", where the "node id" is the origin being displayed in the URL bar plus the origin in which the EME content is running in.

So for example site A.com running EME in an <iframe> from CDN.com will be considered a different node id than B.com running EME in an <iframe> from CDN.com.

So mass-rename "origin" in the EME/Gecko Media Plugin code to "NodeId".
Attachment #8495637 - Flags: review?(bzbarsky)
I want to reuse the code to generate random salt from AndroidMediaResourceServer to generate random NodeId with which to segregate origins running EME. This is because the code to generate salt in AndroidMediaResourceServer ensures that the salt is printable, and I want to use the salt in the path to EME storage on disk, so it needs to be printable.
Attachment #8495638 - Flags: review?(cajbir.bugzilla)
Instead of using the origin to segregate the MediaKeys objects, generate a random value as the "node id". We don't store the salt, we'll just generate it for now.

Also, shutdown the MediaKeys/CDM object if its HTMLMediaElement changes (topLevelPrincipal, principal) (bug 1016709), and don't allow the MediaKeys to be set on an HTMLMediaElement with a (topLevelPrincipal, principal) other than the one in which the MediaKeys was created.
Attachment #8495639 - Flags: review?(bzbarsky)
Attached patch Patch 4: Store node id "salt" (obsolete) — Splinter Review
Store the nodeid "salt" generated in the previous patch, so that subsequent operations with the same (toplevelOrigin, origin) pair have the same salt. The salt is what we expose to the GMP in GMPDecryptorHost::GetNodeId().

We provide random salt to the GMP instead so the true origin to reduce the chance of the GMP being used as a user tracking mechanism.

We will provide a way to clear the stored node ids in future.
Attachment #8495640 - Flags: review?(rjesup)
Comment on attachment 8495640 [details] [diff] [review]
Patch 4: Store node id "salt"

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

::: content/media/gmp/GMPService.cpp
@@ +176,5 @@
>    }
>  
> +  // Directory service is main thread only, so cache the profile dir here
> +  // so that we can use it off main thread.
> +  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mStorageBaseDir));

Note: this fails in e10s Firefox. We'll need to fix this when bug 1057908 is fixed.

@@ +999,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +  if (!exists) {
> +    // No stored salt for this origin. Generate salt, and store it and
> +    // the origin on disk.
> +    nsresult rv = GeneratePathSalt(salt, NodeIdSaltLength);

GeneratePathSalt() fails in e10s Firefox; we can't init NSS in non-chrome process we'll need to fix that in bug 1057908.
Pass the EME node id through to the GMP. Note we must pass it through before the sandbox is initialized, as we must also hash it with some device binding data, which won't work once we drop privileges.
Attachment #8495642 - Flags: review?(rjesup)
Ensure GMPStorage respects private browsing mode.

Ehsan: Can you check that I did the PB mode stuff correctly?

* Figure out whether we should allow the GMPs storage to be persistent; we disallow persistent storage if we're in PB mode, or if either of the topLevelOrigin or origin are null (meaning we're loading a file from the local drive).
* For local files ("null" origins), just generate a random node id for every new file.
* For PB origins, we same node-ids to share CDM instances, that is in a PB session, every EME session with the same (topLevelOrigin, origin) pair will have the same node id. We do this by storing the node ids in memory in the GMPService. The node ids generated for PB session will be forgotten when the last PB context exists (in the "last-pb-context-exited" observer service notification).
* Change GMPStorageParent so that it has a delegate to do the actual storage; one that stores/reads data on disk for node ids that are allowed persistent storage (non-pb and non-local origins) and another delegate which just stores stuff in memory for non-persistent storage.
Attachment #8495650 - Flags: review?(rjesup)
Attachment #8495650 - Flags: review?(ehsan.akhgari)
Attached patch Patch 7: Import librlz (obsolete) — Splinter Review
Import a subset of librlz https://code.google.com/p/rlz/

This library contains a function to generate a "machine id", which we can use to hash with the EME "node id" in order to ensure that data stored by an EME plugin from one computer to another does not result in the same id being exposed to the EME GMP.

We'll use this on Windows only initially, and later I'll import the MacOSX machine id code too.
Attachment #8495651 - Flags: review?(mh+mozilla)
On Windows only, use librlz to get a device id for the machine we're running on and SHA1 that with the EME node id passed in from Gecko. Overwrite the device id data so that there's no chance of the GMP using the user's device id to uniquely identify the device.
Attachment #8495653 - Flags: review?(rjesup)
Attachment #8495653 - Flags: review?(hsivonen)
Skip running EME tests when e10s is enabled, as some of the code added here doesn't work under e10s, and needs bug 1057908 fixed before we can fix it.
Attachment #8495654 - Flags: review?(ajones)
Depends on: 1057908
Comment on attachment 8495638 [details] [diff] [review]
Patch 2: factor out random salt generation from AndroidMediaResourceServer

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

::: content/media/VideoUtils.h
@@ +256,5 @@
>                          int16_t& aLevel);
>  
> +// Use a cryptographic quality PRNG to generate raw random bytes
> +// and convert that to a base64 string suitable for use as a file or URL
> +// path. This is based on code from nsExternalAppHandler::SetUpTempFile.

"This is based on code..." should be in the implementation (ie. the .cpp file) not the header.

@@ +258,5 @@
> +// Use a cryptographic quality PRNG to generate raw random bytes
> +// and convert that to a base64 string suitable for use as a file or URL
> +// path. This is based on code from nsExternalAppHandler::SetUpTempFile.
> +nsresult
> +GeneratePathSalt(nsCString& aOutSalt, uint32_t aLength);

GenerateRandomPathName or similar would seem to be a better name since it's not a "salt" (which is defined as "random data that is used as an additional input to a one-way function that hashes a password or passphrase").
Attachment #8495638 - Flags: review?(cajbir.bugzilla) → review+
Comment on attachment 8495651 [details] [diff] [review]
Patch 7: Import librlz

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

Can you find someone else to review the code itself, or do you want me to do it?

Build system integration notes below (r+ applies to that):
- Please add a README.mozilla file telling where that comes from and noting that the base/ directory contains wrappers/dummies replacing the corresponding chromium headers.

::: content/media/gmp/rlz/moz.build
@@ +11,5 @@
> +    'lib/assert.h',
> +    'lib/crc8.h',
> +    'lib/machine_id.h',
> +    'lib/string_utils.h',
> +]

Please don't export the headers. Just add the parent directory to LOCAL_INCLUDES so that rlz/lib resolves there. I guess this code is only ever going to be used from content/media/gmp, and it'll be includeable from there anyways.

@@ +23,5 @@
> +if CONFIG['OS_TARGET'] == 'WINNT':
> +    SOURCES += [
> +        'win/lib/machine_id_win.cc',
> +    ]
> +    

trailing whitespaces.
Attachment #8495651 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #14)
> Comment on attachment 8495651 [details] [diff] [review]
> Patch 7: Import librlz
> 
> Review of attachment 8495651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you find someone else to review the code itself, or do you want me to do
> it?

I can find someone else. Thanks.
Attachment #8495654 - Flags: review?(ajones) → review+
Comment on attachment 8495653 [details] [diff] [review]
Patch 8: Hash device id with EME node id

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

::: content/media/gmp/GMPChild.cpp
@@ +251,5 @@
> +    int volume_id;
> +    if (!rlz_lib::GetRawMachineId(&deviceId, &volume_id)) {
> +      return false;
> +    }
> +    mozilla::SHA1Sum hash;

Uh maybe we shouldn't be using SHA1 for this, since it's not actually considered cryptographically secure any more... bug 830880.
Attachment #8495653 - Flags: review?(rjesup)
Attachment #8495653 - Flags: review?(hsivonen)
Comment on attachment 8495650 [details] [diff] [review]
Patch 6: Ensure GMPStorage and node ids respect Private Browsing mode

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

The PB specific bits look good in general.  I didn't review the rest very carefully, but I have some random comments below.

But more importantly, this definitely needs some tests.

::: content/media/gmp/GMPService.cpp
@@ +479,5 @@
>  
> +  // Note: we *must* call AllowPersistentStorage() before calling
> +  // GetGMPDecryptor() as GetGMPDecryptor() can cause the GMP to startup,
> +  // where storage would be accesible without knowing whether persistent
> +  // storage was allowed.

Can we somehow assert in debug builds that this invariant holds?

::: content/media/gmp/GMPStorageParent.cpp
@@ +124,5 @@
> +}
> +
> +class GMPDiskStorage : public GMPStorage {
> +public:
> +  GMPDiskStorage(const nsCString& aNodeId)

Nit: explicit.

@@ +208,5 @@
> +class GMPMemoryStorage : public GMPStorage {
> +public:
> +  virtual GMPErr Open(const nsCString& aRecordName) MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(!IsOpen(aRecordName));

This gets called from within the CDM, right?  I don't think we can just assert things like this, you need to actually do the error checking in release builds too.

@@ +292,4 @@
>  bool
>  GMPStorageParent::RecvOpen(const nsCString& aRecordName)
>  {
> +  if (mShutdown || !mStorage) {

It seems to me like there should be no situation where mShutdown is true but mStorage is not null.  Actually, I think mShutdown is redundant at this point.

::: content/media/gmp/GMPStorageParent.h
@@ +46,5 @@
>  
>  private:
>    ~GMPStorageParent() {}
>  
> +  nsAutoPtr<GMPStorage> mStorage;

Nit: please use UniquePtr.
Attachment #8495650 - Flags: review?(ehsan.akhgari) → review+
Comment on attachment 8495637 [details] [diff] [review]
Patch 1 v1: Rename "origin" to "NodeId" in GMP/EME code

BeginReading() should probably return a |const nsCString&| and you should use .get(), not BeginReading(), to get null-terminated pointers out of it (in several places).

Should CDMProxy really have autostring members?  That seems a bit fishy....

Do we really want to treat all "null" things as having the same nodeid?

>+  boolean hasPluginForAPI([optional] in ACString nodeId,

That assumes nodeId will be ASCII-only.  But you're using UTF origins, not ASCII origins...

Either make this AUTF8String, or use ASCII origins, please.

Same for the other APIs taking ACString nodeIds in IDL.

r=me with those fixed.
Attachment #8495637 - Flags: review?(bzbarsky) → review+
Comment on attachment 8495639 [details] [diff] [review]
Patch 3: Generate a random node id for every EME (origin,topLevelOrigin) pair.

>+HTMLMediaElement::GetTopLevelPrincipal()
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(OwnerDoc()->GetParentObject());

This can be null, no?  In that case, probably OK to fall back to NodePrincipal(), thougb even better would be failing out somehow.

>+    nsCOMPtr<nsIDocument> doc = top->GetExtantDoc();

Just nsIDocument* is fine.  But, again, this could be null unless you're guaranteed that this code can only run when the HTMLMediaElement is in a document that is currently loaded or something.

>+MediaKeys::Init(ErrorResult& aRv)
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(GetParentObject());
>+  nsIDocument* doc = window->GetExtantDoc();
>+  mPrincipal = doc->NodePrincipal();

Just QI GetParentObject to nsIScriptObjectPrincipal and get the principal from it directly, please.

>+  if (top && top->GetExtantDoc()) {

If !top, something went seriously wrong and you should be bailing out, I'd think.  I guess that happens because you get !mTopLevelPrincipal?

>+  ACString getNodeId(in AString origin,

Again, that's assuming ascii-only return value.

r=me with those fixed.
Attachment #8495639 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #18)
> Comment on attachment 8495637 [details] [diff] [review]
> Patch 1 v1: Rename "origin" to "NodeId" in GMP/EME code

Note, so we're clear, this is a preparatory patch for later work; some of the things here make more sense with the changes from later patches.

 
> BeginReading() should probably return a |const nsCString&| and you should
> use .get(), not BeginReading(), to get null-terminated pointers out of it
> (in several places).

Are you suggesting I should change BeginReading in our string API to return something different? There are lots of places that use BeginReading() the way I'm using it here:
http://mxr.mozilla.org/mozilla-central/search?string=BeginReading%28%29
 

> Should CDMProxy really have autostring members?  That seems a bit fishy....

I'll make them nsCStrings.

> Do we really want to treat all "null" things as having the same nodeid?

No. In a later patch I generate a new random node id for every new EME context's (origin, topLevelOrigin) pair which has either or both origins null. So no two EME contexts with either or both origin as null will have the same node id. So they won't share.


> 
> >+  boolean hasPluginForAPI([optional] in ACString nodeId,
> 
> That assumes nodeId will be ASCII-only.  But you're using UTF origins, not
> ASCII origins...
> 
> Either make this AUTF8String, or use ASCII origins, please.

We generate a node id that is ASCII in patch two, and store that, indexed by a hash of the origins. So the node ids are ASCII from patch two onwards.
(In reply to Chris Pearce (:cpearce) from comment #20)
> (In reply to Boris Zbarsky [:bz] from comment #18)
> > Comment on attachment 8495637 [details] [diff] [review]
> > Patch 1 v1: Rename "origin" to "NodeId" in GMP/EME code
> 
> Note, so we're clear, this is a preparatory patch for later work; some of
> the things here make more sense with the changes from later patches.
> 
>  
> > BeginReading() should probably return a |const nsCString&| and you should
> > use .get(), not BeginReading(), to get null-terminated pointers out of it
> > (in several places).
> 
> Are you suggesting I should change BeginReading in our string API to return
> something different? There are lots of places that use BeginReading() the
> way I'm using it here:
> http://mxr.mozilla.org/mozilla-central/search?string=BeginReading%28%29


OK, I think I get it. I should be using PromiseFlatCString(GetNodeId()).get()) for when I have an nsACString, and when I know the type is nsCString, I can use nsCString.get(). I should not be using BeginReading as the pointer returned is not guaranteed to be null terminated.
> I should not be using BeginReading as the pointer returned is not guaranteed to be null
> terminated.

Precisely.

In practice, GetNodeId() can returns a "const nsCString&", which has a .get(), because nsCString promises null-termination.
Comment on attachment 8495650 [details] [diff] [review]
Patch 6: Ensure GMPStorage and node ids respect Private Browsing mode

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

I'll rework how nodeids are marked as being allowed storage, and re-request review from jesup.

::: content/media/gmp/GMPService.cpp
@@ +479,5 @@
>  
> +  // Note: we *must* call AllowPersistentStorage() before calling
> +  // GetGMPDecryptor() as GetGMPDecryptor() can cause the GMP to startup,
> +  // where storage would be accesible without knowing whether persistent
> +  // storage was allowed.

If a caller forgets to call AllowPersistentStorage(), the effect is that storage is not persistent, but still appears to the CDM to work. The CDM can't distinguish (except via timing I suppose) between persistent and non-persistent storage.

So I don't see how we can assert that the caller remembers to call AllowPersistentStorage().

But I realise now there's another issue here; GMPService::SelectPluginForAPI() could select a GMP which has been allowed storage, and then we want it to not be allowed storage; there's no way to stop the storage being persistent in that case.

I think I need to re-work this section so that the GMPService just records the nodeids that are allowed persistent storage, and have the GMPStorageParent query the GMPService when it's created.

::: content/media/gmp/GMPStorageParent.cpp
@@ +208,5 @@
> +class GMPMemoryStorage : public GMPStorage {
> +public:
> +  virtual GMPErr Open(const nsCString& aRecordName) MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(!IsOpen(aRecordName));

No, this is called from GMPStorageParent, in the parent process, which checks IsOpen() before calling into these functions. We control the callers of this object. The asserts are there to ensure we always do this.
Attachment #8495650 - Flags: review?(rjesup)
(In reply to Boris Zbarsky [:bz] from comment #22)
> > I should not be using BeginReading as the pointer returned is not guaranteed to be null
> > terminated.
> 
> Precisely.
> 
> In practice, GetNodeId() can returns a "const nsCString&", which has a
> .get(), because nsCString promises null-termination.

Cool, thanks. I've updated the other patches in my queue based on this too.
Comment on attachment 8495640 [details] [diff] [review]
Patch 4: Store node id "salt"

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

I didn't mark all the instances of one-lining error returns, please consider them all pinged

::: content/media/gmp/GMPService.cpp
@@ +177,5 @@
>  
> +  // Directory service is main thread only, so cache the profile dir here
> +  // so that we can use it off main thread.
> +  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mStorageBaseDir));
> +  NS_ENSURE_SUCCESS(rv, rv);

please use the newer patterns (already used in this file except for NS_ENSURE_ARG())

@@ +180,5 @@
> +  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mStorageBaseDir));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = mStorageBaseDir->AppendNative(NS_LITERAL_CSTRING("gmp"));
> +  NS_ENSURE_SUCCESS(rv, rv);

ditto

@@ +183,5 @@
> +  rv = mStorageBaseDir->AppendNative(NS_LITERAL_CSTRING("gmp"));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = mStorageBaseDir->Create(nsIFile::DIRECTORY_TYPE, 0700);
> +  NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && rv != NS_ERROR_FILE_ALREADY_EXISTS, rv);

ditto

@@ +870,5 @@
>  
>  NS_IMETHODIMP
> +GeckoMediaPluginService::GetStorageDir(nsIFile** aOutFile)
> +{
> +  if (NS_WARN_IF(!mStorageBaseDir)) { return NS_ERROR_FAILURE; }

don't one-line it

@@ +881,5 @@
> +            const nsACString& aData)
> +{
> +  nsCOMPtr<nsIFile> path;
> +  nsresult rv = aPath->Clone(getter_AddRefs(path));
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }

and here the newer pattern is used!  hurrah! (except for the one-lining)
Attachment #8495640 - Flags: review?(rjesup) → review+
Comment on attachment 8495642 [details] [diff] [review]
Patch 5: Percolate EME node id to GMP

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

::: content/media/gmp/GMPChild.h
@@ +60,5 @@
>    virtual bool RecvPGMPVideoEncoderConstructor(PGMPVideoEncoderChild* aActor) MOZ_OVERRIDE;
>  
>    virtual PGMPDecryptorChild* AllocPGMPDecryptorChild() MOZ_OVERRIDE;
>    virtual bool DeallocPGMPDecryptorChild(PGMPDecryptorChild* aActor) MOZ_OVERRIDE;
> +  virtual bool RecvPGMPDecryptorConstructor(PGMPDecryptorChild* actor) MOZ_OVERRIDE;

undo this change
Attachment #8495642 - Flags: review?(rjesup) → review+
Comment on attachment 8495651 [details] [diff] [review]
Patch 7: Import librlz

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

dmajor kidnly offered to take a look.
Attachment #8495651 - Flags: review?(dmajor)
Updated previous patch to use NSS' SHA256 implementation.
Attachment #8497133 - Flags: review?(rjesup)
Attachment #8497133 - Flags: review?(hsivonen)
Comment on attachment 8495651 [details] [diff] [review]
Patch 7: Import librlz

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

Can we remove the string_utils files if they are not used?

::: content/media/gmp/rlz/base/memory/scoped_ptr.h
@@ +17,5 @@
> +public:
> +  scoped_array(T* t) : nsAutoArrayPtr<T>(t) {}
> +  void reset(T* t) {
> +    scoped_array<T> other(t);
> +    operator=(other);

Would simply |operator=(t)| work here?

::: content/media/gmp/rlz/lib/assert.h
@@ +5,5 @@
> +
> +#ifndef FAKE_ASSERT_H_
> +#define FAKE_ASSERT_H_
> +
> +#define ASSERT_STRING(x) {}

Are these debug-fatal in the real implementation?

::: content/media/gmp/rlz/lib/machine_id.cc
@@ +1,1 @@
> +#include "rlz/lib/machine_id.h"

Google's file didn't have a copyright/license. Do we need to do anything to clarify that it's not our work?

::: content/media/gmp/rlz/lib/string_utils.cc
@@ +10,5 @@
> +
> +namespace rlz_lib {
> +
> +bool IsAscii(char letter) {
> +  return (letter >= 0x0 && letter < 0x80);

MSVC char is signed, right? Does |letter < 0x80| do the right thing?
Actually, I don't see where this function is called...

@@ +47,5 @@
> +  if ((text[idx] == '0') &&
> +      (text[idx + 1] == 'X' || text[idx + 1] == 'x'))
> +    idx +=2;  // String is of the form 0x...
> +
> +  int number = 0;

This probably does the right thing for >INT_MAX but I had to stop and think about it.
Actually I don't see where this is called either...

::: content/media/gmp/rlz/win/lib/machine_id_win.cc
@@ +33,5 @@
> +  if (!GetVolumeInformationW(system_path, NULL, 0, &number_local, NULL, NULL,
> +                             NULL, 0))
> +    return false;
> +
> +  *number = number_local;

I'm surprised the compiler doesn't complain at assigning unsigned int to int*.

@@ +108,5 @@
> +
> +  wchar_t computer_name[MAX_COMPUTERNAME_LENGTH + 1] = {0};
> +  DWORD size = arraysize(computer_name);
> +
> +  if (GetComputerNameW(computer_name, &size)) {

If this fails, sid_string is never written to.
Attachment #8495651 - Flags: review?(dmajor) → review+
Review comments addressed. Using nsCString for node ids, so I don't need to call PromiseFlatCString anywhere.

Note: NodeIds are ASCII, so I didn't make any changes regarding that.
Attachment #8495637 - Attachment is obsolete: true
Attachment #8497239 - Flags: review+
Attachment #8495638 - Attachment is obsolete: true
Comment on attachment 8497133 [details] [diff] [review]
Patch 8v2: Hash device id (using SHA256) with EME node id

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

r+ from me, but you likely need an NSS person to review (brian smith?)
Attachment #8497133 - Flags: review?(rjesup) → review+
Comment on attachment 8497133 [details] [diff] [review]
Patch 8v2: Hash device id (using SHA256) with EME node id

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

Brina Smith: can you check that I'm using NSS correctly here? This runs in a child process, before we start up an EME plugin, so I need to init and then shutdown NSS.
Attachment #8497133 - Flags: review?(brian)
Review comments addressed; fail more aggressively in HTMLMediaElement::GetTopLevelPrincipal() and in MediaKeys::Init().

bz: how does MediaKeys::Init() look now?
Attachment #8495639 - Attachment is obsolete: true
Attachment #8497258 - Flags: review?(bzbarsky)
Attachment #8495653 - Attachment is obsolete: true
Review comments addressed.
Attachment #8495640 - Attachment is obsolete: true
Attachment #8497260 - Flags: review+
Review comments addressed.
Attachment #8495642 - Attachment is obsolete: true
Ehsan's comments addressed; use UniquePtr, removed redundant mShutdown/mStorage null check.

Still need the non-PB specific stuff reviewed.

Will follow up with tests.
Attachment #8495650 - Attachment is obsolete: true
Attachment #8497269 - Flags: review?(rjesup)
Thanks David! I stripped some more of the unused code out of the patch...

(In reply to :dmajor (away 3-7 October) from comment #29)
> Comment on attachment 8495651 [details] [diff] [review]
> Patch 7: Import librlz
> 
> Review of attachment 8495651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we remove the string_utils files if they are not used?

I'm using BytesToString from string_utils in the next patch.
 
I'm not using anything else, so I will delete everything else in that file.
 
> ::: content/media/gmp/rlz/base/memory/scoped_ptr.h
> @@ +17,5 @@
> > +public:
> > +  scoped_array(T* t) : nsAutoArrayPtr<T>(t) {}
> > +  void reset(T* t) {
> > +    scoped_array<T> other(t);
> > +    operator=(other);
> 
> Would simply |operator=(t)| work here?

Unfortunately that didn't compile for me (with VS2010 at least).

> ::: content/media/gmp/rlz/lib/assert.h
> @@ +5,5 @@
> > +
> > +#ifndef FAKE_ASSERT_H_
> > +#define FAKE_ASSERT_H_
> > +
> > +#define ASSERT_STRING(x) {}
> 
> Are these debug-fatal in the real implementation?

Yes, they're equivalent to assert(false), plus some logging.

I'll make them fatal.



> ::: content/media/gmp/rlz/lib/machine_id.cc
> @@ +1,1 @@
> > +#include "rlz/lib/machine_id.h"
> 
> Google's file didn't have a copyright/license. Do we need to do anything to
> clarify that it's not our work?

Hmm, actually we don't need this file; I thought I'd need to call functions in it, but it turns out I didn't. So I removed it.


> ::: content/media/gmp/rlz/lib/string_utils.cc
> @@ +10,5 @@
> > +
> > +namespace rlz_lib {
> > +
> > +bool IsAscii(char letter) {
> > +  return (letter >= 0x0 && letter < 0x80);
> 
> MSVC char is signed, right? Does |letter < 0x80| do the right thing?
> Actually, I don't see where this function is called...

I deleted this function.

> 
> @@ +47,5 @@
> > +  if ((text[idx] == '0') &&
> > +      (text[idx + 1] == 'X' || text[idx + 1] == 'x'))
> > +    idx +=2;  // String is of the form 0x...
> > +
> > +  int number = 0;
> 
> This probably does the right thing for >INT_MAX but I had to stop and think
> about it.
> Actually I don't see where this is called either...

I deleted this function.

 
> ::: content/media/gmp/rlz/win/lib/machine_id_win.cc
> @@ +33,5 @@
> > +  if (!GetVolumeInformationW(system_path, NULL, 0, &number_local, NULL, NULL,
> > +                             NULL, 0))
> > +    return false;
> > +
> > +  *number = number_local;
> 
> I'm surprised the compiler doesn't complain at assigning unsigned int to
> int*.

Added cast.

 
> @@ +108,5 @@
> > +
> > +  wchar_t computer_name[MAX_COMPUTERNAME_LENGTH + 1] = {0};
> > +  DWORD size = arraysize(computer_name);
> > +
> > +  if (GetComputerNameW(computer_name, &size)) {
> 
> If this fails, sid_string is never written to.

I changed GetRawMachineId to fail if GetComputerNameW fails.
Review comments addressed.
Attachment #8497286 - Flags: review+
Attachment #8495651 - Attachment is obsolete: true
Comment on attachment 8497258 [details] [diff] [review]
Patch 3v2: Generate a random node id for every EME (origin,topLevelOrigin) pair.

>+  if (!topWindow) {
>+    promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);
>+    return promise.forget();
>+  }
>+  nsCOMPtr<nsPIDOMWindow> top = do_QueryInterface(topWindow);
>+  if (!top || !top->GetExtantDoc()) {

do_QI is null-safe, so you could just remove the null-check of topWindow.

r=me
Attachment #8497258 - Flags: review?(bzbarsky) → review+
Comment on attachment 8497133 [details] [diff] [review]
Patch 8v2: Hash device id (using SHA256) with EME node id

I'm not familiar with the NSS API, but this seems reasonable on the general level at this point of development.

Considering the future, though, I'm very worried about dependencies on NSS and XPCOM strings. I expect that eventually, we'll need to copy and paste the hash function into the GMP host in order to be able to build the GMP host with static linkage for reuse by downstreams that ship their own NSS--and still end up with a compact program.
Attachment #8497133 - Flags: review?(hsivonen) → review+
Comment on attachment 8497269 [details] [diff] [review]
Patch 6v2: Ensure GMPStorage and node ids respect Private Browsing mode

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

::: content/media/gmp/GMPStorageParent.cpp
@@ +118,5 @@
>  
> +PLDHashOperator
> +CloseFile(const nsACString& key, PRFileDesc*& entry, void* cx)
> +{
> +  PR_Close(entry);

can PR_Close fail?  It appears to be able to...  error-check

@@ +194,5 @@
> +  virtual void Close(const nsCString& aRecordName) MOZ_OVERRIDE
> +  {
> +    PRFileDesc* fd = mFiles.Get(aRecordName);
> +    if (fd) {
> +      PR_Close(fd);

error-check
Attachment #8497269 - Flags: review?(rjesup) → review+
Attachment #8497133 - Flags: review?(brian) → review?(rlb)
(In reply to Randell Jesup [:jesup] from comment #42)
> Comment on attachment 8497269 [details] [diff] [review]
> Patch 6v2: Ensure GMPStorage and node ids respect Private Browsing mode
> 
> Review of attachment 8497269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/gmp/GMPStorageParent.cpp
> @@ +118,5 @@
> >  
> > +PLDHashOperator
> > +CloseFile(const nsACString& key, PRFileDesc*& entry, void* cx)
> > +{
> > +  PR_Close(entry);
> 
> can PR_Close fail?  It appears to be able to...  error-check

The only thing I can do here is warn on failure. What else can I do? Try calling close again?

> 
> @@ +194,5 @@
> > +  virtual void Close(const nsCString& aRecordName) MOZ_OVERRIDE
> > +  {
> > +    PRFileDesc* fd = mFiles.Get(aRecordName);
> > +    if (fd) {
> > +      PR_Close(fd);
> 
> error-check

Ditto.
With the changes in patch 3, it's no longer possible to use EME for URIs from a file:// URI, i.e. a local HTML file. This is because MediaKeys::CheckPrincipals() compares the principal of the document in which the MediaKeys was created with the HTMLMediaElement's "current principal", which is the principal of the video file being decoded. These two URIs will be different; the MediaKeys' URI will be the HTML file, and the media element's URI will be the video file being decoded. File:// URIs's principals are considered not equal if they're not the same file, so the principal check here will fail and we'll refuse to run EME content.

This is inconvenient for GMP authors, and me, as it makes it harder to test without a webserver setup. So this patch adds a hidden pref to disable this check. The principal checks remain on by default.
Attachment #8497793 - Flags: review?(bzbarsky)
(In reply to Chris Pearce (:cpearce) from comment #43)
> (In reply to Randell Jesup [:jesup] from comment #42)
> > Comment on attachment 8497269 [details] [diff] [review]
> > Patch 6v2: Ensure GMPStorage and node ids respect Private Browsing mode
> > 
> > Review of attachment 8497269 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: content/media/gmp/GMPStorageParent.cpp
> > @@ +118,5 @@
> > >  
> > > +PLDHashOperator
> > > +CloseFile(const nsACString& key, PRFileDesc*& entry, void* cx)
> > > +{
> > > +  PR_Close(entry);
> > 
> > can PR_Close fail?  It appears to be able to...  error-check
> 
> The only thing I can do here is warn on failure. What else can I do? Try
> calling close again?

No, just notice, maybe log, and don't mark as closed (change return in this case I'd guess)
Comment on attachment 8497793 [details] [diff] [review]
Patch 10: Add hidden pref to ignore EME principal check

We could inherit the principal into the video in the file://-in-our-directory case like we do for iframes.  That will incidentally make the video not taint canvases either, right?

I'm not a huge fan of "shoot me in the foot security-wise" prefs; if we do add a pref I'd prefer it were explicitly restricted to the file:// case.
Attachment #8497793 - Flags: review?(bzbarsky) → review-
Comment on attachment 8497133 [details] [diff] [review]
Patch 8v2: Hash device id (using SHA256) with EME node id

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

Keep in mind that I only reviewed the PSM/NSS-facing parts of this.  I don't know anything about RLZ, so I'm assuming that all those parts work as one would expect.

::: content/media/gmp/GMPChild.cpp
@@ +245,5 @@
>    mPluginPath = aPluginPath;
>    return true;
>  #endif
>  
> +#ifdef HASH_NODE_ID_WITH_DEVICE_ID

It would be helpful to have a comment here with an overview of what all this crypto does.

@@ +248,5 @@
>  
> +#ifdef HASH_NODE_ID_WITH_DEVICE_ID
> +  if (!aNodeId.empty() && aNodeId != "null") {
> +    string16 deviceId;
> +    int volume_id;

Nit: Camel case or underscores?

@@ +253,5 @@
> +    if (!rlz_lib::GetRawMachineId(&deviceId, &volume_id)) {
> +      return false;
> +    }
> +
> +    NSS_NoDB_Init(".");

Calling this function directly is almost always a bad sign.  You probably want EnsureNSSInitializedChromeOrContent() (in nsNSSComponent.h).  You might also add an nsNSSShutDownPreventionLock to be safe.

@@ +255,5 @@
> +    }
> +
> +    NSS_NoDB_Init(".");
> +    Digest digest;
> +    ScopedPK11Context digestContext(PK11_CreateDigestContext(SEC_OID_SHA256));

It seems like a lot of extra work to use both Digest and the PK11 interface.  It would be better to just assemble the hash input and pass it to Digest::DigestBuf(), and not much more work:

> string16 hashInput = deviceId + nodeId;
> digest.DigestBuf(...)

If you're concerned at all about the ability to change this in the future (say to SHA-3), you should include a version octet at the beginning of your hash.

@@ +292,5 @@
> +    if (!rlz_lib::BytesToString(digest.get().data, digest.get().len, &mNodeId)) {
> +      return false;
> +    }
> +    // Overwrite device id as it could potentially identify the user, so
> +    // there's no chance a GMP can read it and use it for identity tracking.

Note that hashing this information is still not a great protection against tracking.  If the nodeId is known to the GMP plugin, and the deviceId space is low-ish entropy, then the attacker can generate a rainbow table and reverse the hash.  It would be better if this were a salted hash:
* Generate a random salt (see nsIRandomGenerator)
* hash = SHA-256(salt || deviceId || nodeId)
* Return salt || hash
Attachment #8497133 - Flags: review?(rlb)
Attachment #8497133 - Flags: review-
Attachment #8497133 - Flags: review+
(In reply to Randell Jesup [:jesup] from comment #45)
> (In reply to Chris Pearce (:cpearce) from comment #43)
> > (In reply to Randell Jesup [:jesup] from comment #42)
> > > Comment on attachment 8497269 [details] [diff] [review]
> > > Patch 6v2: Ensure GMPStorage and node ids respect Private Browsing mode
> > > 
> > > Review of attachment 8497269 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: content/media/gmp/GMPStorageParent.cpp
> > > @@ +118,5 @@
> > > >  
> > > > +PLDHashOperator
> > > > +CloseFile(const nsACString& key, PRFileDesc*& entry, void* cx)
> > > > +{
> > > > +  PR_Close(entry);
> > > 
> > > can PR_Close fail?  It appears to be able to...  error-check
> > 
> > The only thing I can do here is warn on failure. What else can I do? Try
> > calling close again?
> 
> No, just notice, maybe log, and don't mark as closed (change return in this
> case I'd guess)

OK, I can not mark it as closed in the GMPDiskStorage::Close() case. It doesn't make sense to stop the iteration in the other case, as that only occurs when we kill the GMPStorageParent, where we do want to try to close all files if possible.
(In reply to Richard Barnes [:rbarnes] from comment #47)
> Comment on attachment 8497133 [details] [diff] [review]
> Patch 8v2: Hash device id (using SHA256) with EME node id

> @@ +253,5 @@
> > +    if (!rlz_lib::GetRawMachineId(&deviceId, &volume_id)) {
> > +      return false;
> > +    }
> > +
> > +    NSS_NoDB_Init(".");
> 
> Calling this function directly is almost always a bad sign.  You probably
> want EnsureNSSInitializedChromeOrContent() (in nsNSSComponent.h).  You might
> also add an nsNSSShutDownPreventionLock to be safe.

We're trying to use as little of NSS as possible, we're running this in a plugin child process, not in a content process, so Gecko/Xul/XPCOM won't be available. Does using EnsureNSSInitializedChromeOrContent() require XPCOM/XUL?

Actually XUL is available, but we're planning on removing that dependency in future, so if we can make this work with pulling in the XUL/XPCOM that would be ideal.

Maybe we can just copy-and-paste the code for SHA1 and build it inside the child process' plugin-container?


> @@ +255,5 @@
> > +    }
> > +
> > +    NSS_NoDB_Init(".");
> > +    Digest digest;
> > +    ScopedPK11Context digestContext(PK11_CreateDigestContext(SEC_OID_SHA256));
> 
> It seems like a lot of extra work to use both Digest and the PK11 interface.
> It would be better to just assemble the hash input and pass it to
> Digest::DigestBuf(), and not much more work:
> 
> > string16 hashInput = deviceId + nodeId;
> > digest.DigestBuf(...)
> 
> If you're concerned at all about the ability to change this in the future
> (say to SHA-3), you should include a version octet at the beginning of your
> hash.

OK.


> @@ +292,5 @@
> > +    if (!rlz_lib::BytesToString(digest.get().data, digest.get().len, &mNodeId)) {
> > +      return false;
> > +    }
> > +    // Overwrite device id as it could potentially identify the user, so
> > +    // there's no chance a GMP can read it and use it for identity tracking.
> 
> Note that hashing this information is still not a great protection against
> tracking.  If the nodeId is known to the GMP plugin, and the deviceId space
> is low-ish entropy, then the attacker can generate a rainbow table and
> reverse the hash.  It would be better if this were a salted hash:
> * Generate a random salt (see nsIRandomGenerator)
> * hash = SHA-256(salt || deviceId || nodeId)
> * Return salt || hash

I'm a crypto noob, so please forgive me if I'm being stupid...

The hashed nodeId we expose to the GMP has to be consistent across sessions on the same origin. The nodeId we get from the parent process is a random string that we generate (using nsIRandomGenerator) and store (in the parent) per origin, and when the user opts to "clear private data", we clear the stored nodeId and will re-generate a new random string the next time we visit that origin again. So the nodeId seems (at least to me) to be the salt here.

That is how we mitigate tracking here, as different origins don't see the same nodeId.

The hashed value that we expose to the CDM has to be consistent across sessions (when visiting the same origin, provided the user has not opted to clear private data), i.e. every time we visit site A the nodeId exposed to the CDM should be the same. So if we always salt the generated nodeId with a random value every time, we'll not get this behaviour.

So... How do we mitigate against a rainbow-table attack given that we need to keep the (after hashing) nodeId consistent across sessions? Do we need to store two lots of salt in the parent process per origin, one for the node/origin id, and one for use as salt?
Flags: needinfo?(rlb)
(In reply to Henri Sivonen (:hsivonen) from comment #41)
> Comment on attachment 8497133 [details] [diff] [review]
> Patch 8v2: Hash device id (using SHA256) with EME node id

> Considering the future, though, I'm very worried about dependencies on NSS
> and XPCOM strings. I expect that eventually, we'll need to copy and paste
> the hash function into the GMP host in order to be able to build the GMP
> host with static linkage for reuse by downstreams that ship their own
> NSS--and still end up with a compact program.

OK, I extracted NSS's sha256 implementation from http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/freebl/sha512.c and its pretty easy to just copy/paste into a separate file and we can link into the plugin host, it's all static code. This means we don't need to init NSS at all, or worry about adding dependencies to it, or it being torn down while we're using it.
WIP after addressing some of rbarnes's comments:
* Statically link NSS's sha512.c, which removes the dependency on initialing NSS, and facilitates making the plugin container downloadable separately in future.
* Add version byte to digest.
* We still need to agree upon the salting to be used here.

I also guess I need to be more careful with the unhashed node id? The std::string containing it is on the stack when we call into the plugin at the end of GMPChild::Init(), in the LoadPluginLibrary() function. So I guess to be secure we'll have to ensure the unhashed node id is not on the stack when we load the plugin. Probably need to move the node id creation code into either the constructor or move the LoadPluginLibrary() call into a separate function we call after Init() succeeds.
Attachment #8497133 - Attachment is obsolete: true
Attachment #8498631 - Flags: feedback?(rlb)
Comment on attachment 8498631 [details] [diff] [review]
WIP Patch 8v3: Hash device id (using SHA256) with EME node

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

::: content/media/gmp/moz.build
@@ +102,5 @@
> +    # ...but sha512.c emits C2220 warning, which breaks compilation and which
> +    # we can't disable individually, so disable all warnings for this file.
> +    if CONFIG['_MSC_VER']:
> +        CFLAGS += [
> +            '-w', # disable all warnings...

-wd2220 doesn't work?
(In reply to Mike Hommey [:glandium] from comment #52)
> Comment on attachment 8498631 [details] [diff] [review]
 
> -wd2220 doesn't work?

No, not for VS2010 at least which is what I'm building with here.
(In reply to Chris Pearce (:cpearce) from comment #53)
> (In reply to Mike Hommey [:glandium] from comment #52)
> > Comment on attachment 8498631 [details] [diff] [review]
>  
> > -wd2220 doesn't work?
> 
> No, not for VS2010 at least which is what I'm building with here.

Note that adding -w to CFLAGS disables *every* warning for *every* file in the directory. From that perspective, this is a r- from me.

So, C2220 is actually the error you get because of FAIL_ON_WARNINGS = True. So the question becomes: what is(are) the actual warning(s) you get when building sha512.c? From a build log on m-i, it would appear C4101 (unreferenced local variable) is the one.

Which means, -wd4101 should work around it. On the other hand, sha512.c should just be fixed.
(In reply to Mike Hommey [:glandium] from comment #54)
> Which means, -wd4101 should work around it. On the other hand, sha512.c
> should just be fixed.

-wd4101 works. Thanks!
Comment on attachment 8498631 [details] [diff] [review]
WIP Patch 8v3: Hash device id (using SHA256) with EME node

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

Pro: Salting is good, version number maybe good but probably unnecessary.
Con: Don't deep link into NSS.

::: content/media/gmp/GMPChild.cpp
@@ +32,5 @@
> +// that every time we revisit the same origin we get the same salt.
> +// We send this salt to the child on startup. The child collects some
> +// device specific data and munges that with the salt to create the
> +// "node id" that we expose to EME plugins. It then overwrites the device
> +// specific data, and activates the sandbox.

This sounds fine, especially since the sandbox doesn't get the salt.  I would observe, though, that if you're going to store the salt, you might as well just store the hash.

@@ +264,5 @@
> +
> +    string16 hashInput = deviceId + string16(aNodeId.begin(), aNodeId.end());
> +    unsigned char digest[SHA256_LENGTH + 1] = {0};
> +    digest[0] = NODE_ID_HASH_VERSION;
> +    unsigned int outLen = 0;

On second thought, this might not be necessary, if the consumer of the hash is treating it as an opaque value, you don't really need this.  If you change the hash, it will just show up as a new ID to the consumer.

@@ +268,5 @@
> +    unsigned int outLen = 0;
> +
> +    SHA256_HashBuf(digest + 1,
> +                   (uint8_t*)hashInput.c_str(),
> +                   hashInput.size() * sizeof(string16::value_type));

Please do not use FreeBL directly.  It is not intended as an external interface of NSS, and this will produce build failures on Windows and Linux.  If you really want to do this, you will need review by an NSS peer.

Either nsCryptoHash or Digest::DigestBuf() would be fine, together with EnsureNSSInitializedChromeOrContent().  If you really want to use NSS directly (really?  why?), include "pk11pub.h" and use PK11_HashBuf().
Attachment #8498631 - Flags: feedback?(rlb) → feedback-
Attachment #8497793 - Attachment is obsolete: true
(In reply to Richard Barnes [:rbarnes] from comment #56)
> Comment on attachment 8498631 [details] [diff] [review]
> WIP Patch 8v3: Hash device id (using SHA256) with EME node
> 
> Review of attachment 8498631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Pro: Salting is good, version number maybe good but probably unnecessary.
> Con: Don't deep link into NSS.
> 
> ::: content/media/gmp/GMPChild.cpp
> @@ +32,5 @@
> > +// that every time we revisit the same origin we get the same salt.
> > +// We send this salt to the child on startup. The child collects some
> > +// device specific data and munges that with the salt to create the
> > +// "node id" that we expose to EME plugins. It then overwrites the device
> > +// specific data, and activates the sandbox.
> 
> This sounds fine, especially since the sandbox doesn't get the salt.

Great, thanks for checking this for us!

> @@ +268,5 @@
> > +    unsigned int outLen = 0;
> > +
> > +    SHA256_HashBuf(digest + 1,
> > +                   (uint8_t*)hashInput.c_str(),
> > +                   hashInput.size() * sizeof(string16::value_type));
> 
> Please do not use FreeBL directly.  It is not intended as an external
> interface of NSS, and this will produce build failures on Windows and Linux.

It builds for me on Windows, NSS is loaded dynamically, at least on Windows.

> If you really want to do this, you will need review by an NSS peer.

OK.

The reason why we want to do this is that we need to statically link the hash function so that its code is in plugin-container.exe, not dynamically loaded in nss.dll as currently happens, at least on Windows.

Adobe provides us with a tool that produces a "voucher" for the code in plugin-container.exe which we will run as part of our build process. We make the voucher available to Adobe's plugin, which convinces itself the voucher is authentic and then compares the voucher with the memory space of the process at runtime to give itself some confidence that the plugin-container is not compromised/patched. The hash function needs to be included in the code which is vouched for so that the plugin knows it's not compromised.

We also want third party Linux distros to be able to download our plugin-container+voucher, since they can't build a plugin-container that the plugin will work without unless they have a relationship with Adobe in order to be able to get a voucher tool from Adobe. So we want the downloaded plugin-container to not depend on external libraries if possible, so that the download is as small as possible.


> Either nsCryptoHash or Digest::DigestBuf() would be fine, together with
> EnsureNSSInitializedChromeOrContent().  If you really want to use NSS
> directly (really?  why?), include "pk11pub.h" and use PK11_HashBuf().

Basically, we need to reduce the size of our code here as much as possible.

Maybe we can statically link NSS here and rely on the compiler to strip out the unused code.
Flags: needinfo?(rlb)
I've reworked patch 3, so that the EME raw node id is not in the command line arguments of the child process, but is instead passed over in its own message before a later message that instructs the child process to engage the sandbox and start the plugin.

This means the raw node id is not sitting in memory on the call stack or in the command line args somewhere else in memory when the EME GMP is running.
Attachment #8497262 - Attachment is obsolete: true
Attachment #8500835 - Flags: review?(rjesup)
Attachment #8500835 - Flags: review?(rjesup) → review+
I think the best way forward is to land my patches here but use Mozilla::SHA1 for hashing as I did in an the first version of patch 8, and then follow up in another bug to switch to SHA256 once we figure out the linkage/xul/nss issues in plugin-container. i.e. once we know whether we'll be using xul.dll in the plugin-container.exe, or if we statically link the bits we need or whatever, or if we have to rewrite plugin-container to not use xul...

Switching to SHA256 will block shipping EME.
Depends on: 1080335
Filed Bug 1080335 to track changing the hash used here to SHA256.
(In reply to Mike Hommey [:glandium] from comment #64)
> Fixup for unified builds:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/11ef2d1d2215

err, I meant non-unified.
Chris, sorry i had to backout this changes since we had perma failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2879080&repo=mozilla-inbound after the landing of this changes. Could you take a look, thanks!
Comment on attachment 8498631 [details] [diff] [review]
WIP Patch 8v3: Hash device id (using SHA256) with EME node

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

::: content/media/gmp/GMPChild.cpp
@@ +278,5 @@
> +    // Overwrite device id as it could potentially identify the user, so
> +    // there's no chance a GMP can read it and use it for identity tracking.
> +    volumeId = 0;
> +    memset(&deviceId.front(), '*', sizeof(string16::size_type) * deviceId.size());
> +    memset(&hashInput.front(), '*', sizeof(string16::size_type) * hashInput.size());

Should this be sizeof(string16::value_type), not sizeof(string16::size_type)? If value_type is char and size_type is size_t or uint32_t, then these memsets would overrun the deviceId and hashInput buffers.
(In reply to Chris Peterson (:cpeterson) from comment #67)
> Comment on attachment 8498631 [details] [diff] [review]
> WIP Patch 8v3: Hash device id (using SHA256) with EME node
> 
> Review of attachment 8498631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/gmp/GMPChild.cpp
> @@ +278,5 @@
> > +    // Overwrite device id as it could potentially identify the user, so
> > +    // there's no chance a GMP can read it and use it for identity tracking.
> > +    volumeId = 0;
> > +    memset(&deviceId.front(), '*', sizeof(string16::size_type) * deviceId.size());
> > +    memset(&hashInput.front(), '*', sizeof(string16::size_type) * hashInput.size());
> 
> Should this be sizeof(string16::value_type), not
> sizeof(string16::size_type)? I

Yes! Thanks!
(In reply to Carsten Book [:Tomcat] from comment #66)
> Chris, sorry i had to backout this changes since we had perma failures like
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=2879080&repo=mozilla-inbound after the landing of this changes.
> Could you take a look, thanks!

The failure is caused by GMPService::Init() opening the directory in which we'll store node ids and data in the content process, which is not allowed in B2G. It's not a good idea in e10s Firefox FWIW.

In the meantime, I will make this #def'd out on MOZ_EME, and disable EME on B2G, since it's not going to work there anyway until we overhaul GMPService to be multi-process friendly.
Try with the code that fails on B2G #ifdef'd out:
https://tbpl.mozilla.org/?tree=Try&rev=198e2737020d
Depends on: 1082550
This seems to have broken Android video (mp4) playback, see bug 1082550
Mass update firefox-status to track EME uplift.
See Also: → 1422669
You need to log in before you can comment on or make changes to this bug.