Closed Bug 1032814 Opened 10 years ago Closed 10 years ago

Gecko media plugins should not use well-known locations for plugins

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(1 file, 2 obsolete files)

gmps-paths v3
(deleted), patch
Details | Diff | Splinter Review 
Currently gecko media plugins search in the same basic set of locations as NPAPI plugins: /Library/Internet Plugins/ and so forth.

This is unnecessary and gets us stuck hitting the filesystem (probably synchronously) which is something that has sucked about NPAPI plugins.

Since we know in advance the exact set of plugins Firefox intends to support, we should not use the generic API: instead we should have a specific set of locations. Specifically for the OpenH264 plugin, these locations should be searched:

<profile>/openh264/<version>
<appdir>/openh264/<version>
OS-specific location:
/usr/lib/openh264/
/Library/Application Support/OpenH264
Key HKLM/Cisco/OpenH264 "InstallPath"
Key HKCU/Cisco/OpenH264 "InstallPath"

open question: around upgrade time, we may have multiple versions of the OpenH264 plugin: the version for the "old" Firefox and we're downloading a new version for the "new" Firefox. How do we inform the media code which version is the correct one to use? What about the case for "distro" openh264?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #0)
> Currently gecko media plugins search in the same basic set of locations as
> NPAPI plugins: /Library/Internet Plugins/ and so forth.
> 
> This is unnecessary and gets us stuck hitting the filesystem (probably
> synchronously) which is something that has sucked about NPAPI plugins.
> 
> Since we know in advance the exact set of plugins Firefox intends to
> support, we should not use the generic API: 

I don't believe this assumption is correct. Ultimately, we would like to
be able to (for instance) allow users to install a Daala plugin without
a explicit support in Firefox.



> instead we should have a
> specific set of locations. Specifically for the OpenH264 plugin, these
> locations should be searched:
> 
> <profile>/openh264/<version>
> <appdir>/openh264/<version>
> OS-specific location:
> /usr/lib/openh264/
> /Library/Application Support/OpenH264
> Key HKLM/Cisco/OpenH264 "InstallPath"
> Key HKCU/Cisco/OpenH264 "InstallPath"
> 
> open question: around upgrade time, we may have multiple versions of the
> OpenH264 plugin: the version for the "old" Firefox and we're downloading a
> new version for the "new" Firefox. How do we inform the media code which
> version is the correct one to use? What about the case for "distro" openh264?
I strongly disagree with the notion that we're adding another generic plugin interface into Firefox. The fundamental difference with GMP is that we're strongly controlling the software we load and its compatibility.

If people want to install something like Daala, it should be through a Firefox extension so that we can use all the normal compatibility and control systems.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> I strongly disagree with the notion that we're adding another generic plugin
> interface into Firefox. The fundamental difference with GMP is that we're
> strongly controlling the software we load and its compatibility.

Not from my perspective. The fundamental difference is that it's single
purpose and isolated. So, no, it's not generic. But !generic != hard-coded.


> If people want to install something like Daala,

There's no meaningful difference between Daala and OpenH264 in this
context.


> it should be through a
> Firefox extension so that we can use all the normal compatibility and
> control systems.
In terms of the UX, there is a significant difference between OpenH264 and Daala. We have decided to install, enable, and update OpenH264 by default in Firefox, and we intend to do careful QA and version control to make sure that Firefox is using a tested version of OpenH264.

Daala is, for now, an untested addon that users can choose to install but isn't support. As such, we have fairly strict UI requirements: users should choose to install these types of addons (they should not be found by default). And since we don't want to find them by default, we don't need the legacy code here which does this finding of arbitrary plugins, which has known performance costs and is making OpenH264 difficult.
All file i/o for Gecko Media Plugins is off the main thread, btw. That includes searching directories.
Just a note that in bug 1009816 I'm going to install into the current documented locations in https://wiki.mozilla.org/GeckoMediaPlugins .  If this changes though from this bug, once the patch for the changes is available, I'll update to the new paths to install into.  To verify things are being installed correctly I need to code currently to match reality of what's being loaded.
bbondy, the current documented location for Windows plugins involves making registry changes to HKLU, which requires elevation. I don't think that is something we should do.
Sorry, I meant HKLM.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> bbondy, the current documented location for Windows plugins involves making
> registry changes to HKLU, which requires elevation. I don't think that is
> something we should do.

I wondered if that was a bug in documentation since OSX has both  /Library and ~/Library.  (Or perhaps a bug in code). BTW the work in my bug is being done in OSX but I'll port to Windows once done.
Having thought about this some more here's my short-term proposal:

* The primary way to tell the GMP about a plugin will be to explicitly add a directory at runtime: mozIGeckoMediaPluginService.addPlugin(nsIFile) and .removePlugin(nsIFile) if that can be made scriptable, or some new interface with those methods.
* Short-term, the addon manager provider will add the OpenH264 plugin. We will default to <profile>/openh264/<version>
* In order to enable local testing of other plugins, the MediaPluginHost will read the MOZ_MEDIAPLUGINS_PATH envvar and load any other plugins listed there
* To support end-users installing arbitrary other GMPs like Daala, we'll need bug 950933

This solves the primary goals of avoiding on-by-default software sideloads and making sure that users can see and control extra software. It also gives a straightforward non-privileged place to put the OpenH264 plugin.

I'll take this.
Assignee: nobody → benjamin
(In reply to Eric Rescorla (:ekr) from comment #1)
> I don't believe this assumption is correct. Ultimately, we would like to
> be able to (for instance) allow users to install a Daala plugin without
> a explicit support in Firefox.

I'm not convinced we should do this for media playback.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> * In order to enable local testing of other plugins, the MediaPluginHost
> will read the MOZ_MEDIAPLUGINS_PATH envvar and load any other plugins listed
> there

For media playback we will need a hard coded list of acceptable signatures. Setting an environment variable is too easy to circumvent.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> Having thought about this some more here's my short-term proposal:
>

This sounds generally satisfactory. I have some questions, but I suspect
they have simple answers.


> * The primary way to tell the GMP about a plugin will be to explicitly add a
> directory at runtime: mozIGeckoMediaPluginService.addPlugin(nsIFile) and
> .removePlugin(nsIFile) if that can be made scriptable, or some new interface
> with those methods.

What will invoke this API for (say) an extension? The extension?
I would nitpick the term "file" here since isn't this a directory?


> * Short-term, the addon manager provider will add the OpenH264 plugin. We
> will default to <profile>/openh264/<version>
> * In order to enable local testing of other plugins, the MediaPluginHost
> will read the MOZ_MEDIAPLUGINS_PATH envvar and load any other plugins listed
> there

This seems satisfactory. I'd also be willing to have it require >1 user action
if people are really worried about this. E.g., a command line flag or
about:config or something.




> * To support end-users installing arbitrary other GMPs like Daala, we'll
> need bug 950933
> 
> This solves the primary goals of avoiding on-by-default software sideloads
> and making sure that users can see and control extra software. It also gives
> a straightforward non-privileged place to put the OpenH264 plugin.
> 
> I'll take this.

SGTM.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #11)
> (In reply to Eric Rescorla (:ekr) from comment #1)
> > I don't believe this assumption is correct. Ultimately, we would like to
> > be able to (for instance) allow users to install a Daala plugin without
> > a explicit support in Firefox.
> 
> I'm not convinced we should do this for media playback.

Sure. I'm primarily thinking of WebRTC, where we can deploy
as many versions of Daala as we want because we have a negotiation
mechanism.



> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> > * In order to enable local testing of other plugins, the MediaPluginHost
> > will read the MOZ_MEDIAPLUGINS_PATH envvar and load any other plugins listed
> > there
> 
> For media playback we will need a hard coded list of acceptable signatures.
> Setting an environment variable is too easy to circumvent.

Hmm... How do you intend that one develops such plugins?
(In reply to Eric Rescorla (:ekr) from comment #13)
> Hmm... How do you intend that one develops such plugins?

By patching gecko.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #14)
> (In reply to Eric Rescorla (:ekr) from comment #13)
> > Hmm... How do you intend that one develops such plugins?
> 
> By patching gecko.

Yeah, that seems pretty lame.
(In reply to Eric Rescorla (:ekr) from comment #15)
> (In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #14)
> > (In reply to Eric Rescorla (:ekr) from comment #13)
> > > Hmm... How do you intend that one develops such plugins?
> > 
> > By patching gecko.
> 
> Yeah, that seems pretty lame.

It seems totally reasonable to me that you'd use a self-built debug build of Firefox to develop GMPs.

I strongly think we should only load GMPs with acceptable signatures. Otherwise, there will be sites that tell users to run a full-privileged setup.exe to dump a GMP on the disk and who knows what else. We shouldn't have an environment variable, pref or thing like that that would be too easy for third-party software to manipulate without unambiguously positioning itself as malware (as third-party software replacing pieces of the installed Firefox code would be positioned as).
(In reply to Henri Sivonen (:hsivonen) from comment #16)
> (In reply to Eric Rescorla (:ekr) from comment #15)
> > (In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #14)
> > > (In reply to Eric Rescorla (:ekr) from comment #13)
> > > > Hmm... How do you intend that one develops such plugins?
> > > 
> > > By patching gecko.
> > 
> > Yeah, that seems pretty lame.
> 
> It seems totally reasonable to me that you'd use a self-built debug build of
> Firefox to develop GMPs.
>
> I strongly think we should only load GMPs with acceptable signatures.
> Otherwise, there will be sites that tell users to run a full-privileged
> setup.exe to dump a GMP on the disk and who knows what else.

Well, if I get you to run a malicious .exe, it seems like you have
more serious problems than what I install in Firefox.


> We shouldn't
> have an environment variable, pref or thing like that that would be too easy
> for third-party software to manipulate without unambiguously positioning
> itself as malware (as third-party software replacing pieces of the installed
> Firefox code would be positioned as).

This seems like a lot of inconvenience for developers with marginal security
value. I suggest that we schedule a meeting to discuss this. Proposed
attendees: DougT, Anthony, CPearce, BSmedberg, me.
For right now, we are going to do the following things:

* Have a gecko API to programmatically register GMPs at runtime
* Also support an environment variable for automatic registration
* I'll prepare an example restartless Firefox extension to show how things like experimental-Daala can be packed for AMO

I don't think we need to meet about this now. If you want to discuss removing the environment variable, we can do that as a followup.
Attached patch gmp-paths (obsolete) (deleted) — — Splinter Review 
This lacks most testing.
Attachment #8452580 - Flags: review?(ekr)
Comment on attachment 8452580 [details] [diff] [review]
gmp-paths

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

Bouncing this to Josh
Attachment #8452580 - Flags: review?(ekr) → review?(joshmoz)
(In reply to Eric Rescorla (:ekr) from comment #17)
> Well, if I get you to run a malicious .exe, it seems like you have
> more serious problems than what I install in Firefox.

I agree that in principle, all is lost if a site convinces you to run a .exe. However, I think making decisions based on that kind of security defeatism is harmful, though, because it ignores shades of gray, like anti-virus software vendors maybe being more likely to take action against an .exe patching Firefox without Mozilla's cooperation than an .exe dropping a plug-in and maybe also some foistware that's not outright objectively malicious per se.

Also, giving rise to completely non-dodgy GMP installers would still train users to accept the idea that having to run third-party installers to make video to work is something that one legitimately needs to do, which would provide the cover of plausibility for dodgy installers--i.e. perpetuate the problems that exist with NPAPI plug-ins and with codec packs for Windows XP.
Got a test GMP working and did some testing:

* init via envvar works
* init via API works
* uninit via API stops the GMP. It appears that some other parts of gecko aren't aware that the GMP is now gone, so the page still tries to use it. re-init via the API doesn't seem to help. I'd like to address this in a followup, because I don't think it will affect OpenH264.

There is one typo in the patch: In RemoveOnThread, the loop should start at 0. I also needed to add content_geckomediaplugins.xpt to package-manifest.in. I'm happy to upload a new version or a followup patch.
Comment on attachment 8452580 [details] [diff] [review]
gmp-paths

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

::: content/media/gmp/GMPService.cpp
@@ +339,5 @@
> +void
> +GeckoMediaPluginService::LoadFromEnvironment()
> +{
> +  MOZ_ASSERT(NS_GetCurrentThread() == mGMPThread);
> +  

ws

@@ +352,5 @@
> +  }
> +
> +  uint32_t pos = 0;
> +  while (pos < allpaths.Length()) {
> +    int32_t next = allpaths.FindChar(XPCOM_ENV_PATH_SEPARATOR[0], pos);

a comment for this block wouldn't hurt

@@ +384,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  nsCOMPtr<nsIRunnable> r = new PathRunnable(this, aDirectory, true);
> +  thread->Dispatch(r, 0);

Symbolic instead of 0 (DISPATCH_NORMAL)

@@ +389,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +GeckoMediaPluginService::RemovePluginDirectory(const nsAString& aDirectory)

AddPluginDirectory and RemovePluginDirectory are the same except for "true" vs "false", consider the obvious code reuse options here.

@@ +398,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  nsCOMPtr<nsIRunnable> r = new PathRunnable(this, aDirectory, false);
> +  thread->Dispatch(r, 0);

ditto DISPATCH_NORMAL

::: content/media/gmp/GMPService.h
@@ +62,5 @@
> +    { }
> +
> +    NS_DECL_NSIRUNNABLE
> +
> +    private:

left edge
Attachment #8452580 - Flags: review+
Attached patch gmp-paths v2 (obsolete) (deleted) — — Splinter Review 
Updated to jesup's comment except I did not do any additional code unification around {Add,Remove}PluginDirectory since they are already pretty small.
Attachment #8452580 - Attachment is obsolete: true
Attachment #8452580 - Flags: review?(joshmoz)
Attachment #8453166 - Flags: review?(joshmoz)
Comment on attachment 8453166 [details] [diff] [review]
gmp-paths v2

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

::: content/media/gmp/GMPService.cpp
@@ +460,5 @@
>    mPlugins.AppendElement(gmp);
>  }
>  
>  void
> +GeckoMediaPluginService::RemoveOnThread(const nsAString& aDirectory)

I'd prefer it if signatures like this references the thread in question, e.g. "RemoveOnGMPThread".

::: content/media/gmp/mozIGeckoMediaPluginService.idl
@@ +70,5 @@
> +
> +  /**
> +   * Add a directory to scan for gecko media plugins.
> +   * @note Main-thread API.
> +   */

Would be nice to have consistent comment style in this file. I don't care which one you choose.
Attachment #8453166 - Flags: review?(joshmoz) → review+
Backed out for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=43535475&tree=Mozilla-Inbound
In file included from /builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.cpp:6:0:
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.h:53:3: error: expected class-name before '{' token
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.h:62:5: error: 'NS_DECL_NSIRUNNABLE' does not name a type
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.h: In constructor 'mozilla::gmp::GeckoMediaPluginService::PathRunnable::PathRunnable(mozilla::gmp::GeckoMediaPluginService*, const nsAString_internal&, bool)':
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.h:57:9: error: class 'mozilla::gmp::GeckoMediaPluginService::PathRunnable' does not have any field named 'mService'
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.cpp: At global scope:
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.cpp:295:44: error: no 'nsresult mozilla::gmp::GeckoMediaPluginService::PathRunnable::Run()' member function declared in class 'mozilla::gmp::GeckoMediaPluginService::PathRunnable'
In file included from /builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.cpp:6:0:
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.h:52:9: error: 'class mozilla::gmp::GeckoMediaPluginService::PathRunnable' is private
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.cpp:295:44: error: within this context
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.cpp: In member function 'virtual nsresult mozilla::gmp::GeckoMediaPluginService::AddPluginDirectory(const nsAString_internal&)':
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.cpp:314:68: error: conversion from 'mozilla::gmp::GeckoMediaPluginService::PathRunnable*' to non-scalar type 'nsCOMPtr<nsIRunnable>' requested
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.cpp: In member function 'virtual nsresult mozilla::gmp::GeckoMediaPluginService::RemovePluginDirectory(const nsAString_internal&)':
/builds/slave/m-in-lx-d-00000000000000000000/build/content/media/gmp/GMPService.cpp:328:69: error: conversion from 'mozilla::gmp::GeckoMediaPluginService::PathRunnable*' to non-scalar type 'nsCOMPtr<nsIRunnable>' requested
make[5]: *** [GMPService.o] Error 1

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3916efc0bf
Attached patch gmps-paths v3 (deleted) — — Splinter Review 

  

  



        Attachment #8453166 -
        Attachment is obsolete: true

    
    

    
  
This version builds in non-unified builds; it needed an extra nsThreadUtils.h include. Sorry for the earlier bustage!

  

  


Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/3141212527eb

  

  


Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

    
  
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: