838290 - nsPluginHost::GetPermissionStringForType should be more robust against some plugin filename changes

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

As per bug 821892 comment 10, plugin permissions should be keyed on the MIME type (and not the filename as it currently is). This type would need to be canonical, so nsPluginHost would basically map {a mime type} -> {the plugin that has registered to handle that mime type} -> {the first mime type handled by that plugin}. (Although, I suppose the ordering of that list could change, so maybe it should be the first one alphabetically? Or maybe this won't happen enough for us to care?)

Comment 1

[Benjamin Smedberg]

I guess that depends on the permissions and how we want to display them. My plan for the global "enabled/disabled/CtP" pref in bug 830267 comment 4:

* store the permission on *all* the MIME types for a plugin
* load the permission for all the MIME types, and take the most restrictive permission

Unfortunately thinking about this some more, there will be problems with this at least for the Java case, because Java adds a new MIME type for each release. This means that Java would be automatically reset back to CtP every time a new version of Java is released, even if the user had chosen to always enable in the addon manager.

Perhaps we should follow this basic plan but for a few well-known plugins have a "key MIME type" that we use instead of the full list? Or something?

Looking through my list of Windows plugins, many of them have their "primary" MIME type first in the list, so perhaps that is good enough. QuickTime is the outlier, because their first MIME type is image/jp2 (and they have over 90 entries in their MIMEtype list).

Bah this is complicated. Perhaps we should store them by filename, but normalize the filenames for Java and Flash in our code?

Comment 2

[Georg Fritzsche [:gfritzsche]]

If i haven't missed anything we have two separate things to address: 
(1) global per-plugin settings (enabled/disabled/CtP)
(2) per-site, per-plugin permissions, overriding the global ones

With this bug breaking out (2) from bug 830267, i think simply making (2) per-mimetype should mostly work fine.
I can only think of edge-cases like e.g.:
* a mimetype is handled by two different plugins, A and B
* A sorts before B, user chooses to always activate it for a site
* B gets updated, now sorts before A
* B now got the permission without user-consent
I'm not sure if this should be a concern.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Luckily, we already detect if a plugin is java/flash, so we can special case those easily. How does this look? It doesn't address all concerns, but it gets us most of the way there, I think.

Comment 4

[Benjamin Smedberg]

Comment on attachment 711597 [details] [diff] [review]
patch

How about this heuristic:

* special case java and flash as you have here
* use the filename, *except* remove digits and punctuation, and lowercase the string so e.g.:

npqtplugin7.dll,npqtplugin7.dll,npqtplugin6.dll,npqtplugin6.dll,npqtplugin5.dll,npqtplugin5.dll,npqtplugin4.dll,npqtplugin4.dll,npqtplugin3.dll,npqtplugin3.dll,npqtplugin2.dll,npqtplugin2.dll,npqtplugin.dll,npqtplugin.dll would all become "npqtplugindll".

npGoogleUpdate3.dll would become "npgoogleupdatedll"

npdeployJava1.dll would become "npdeployjavadll"

nppdf32.dll would become "nppdfdll"

npqmp071706000001.dll would become "npqmpdll"

I'm afraid that mimeTypes[0] will be too volatile and in some cases may even be random (are windows registry keys always enumerated in a set order?)

Comment 5

[Georg Fritzsche [:gfritzsche]]

What about e.g. npfoo3d.dll vs. npfood.dll? 
Could we just strip trailing digits & punctuation after stripping the extension?

Comment 6

[Benjamin Smedberg]

Yes, that would be fine too. Too bad we can't use regexes in C++ ;-)

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2

How does this look?

Comment 8

[Benjamin Smedberg]

Comment on attachment 712639 [details] [diff] [review]
patch v2

I think you want RFind instead of Find to find the last dot, in case there is more than one?

I wonder if there's a straightforward way to test this. We'll also probably want to expose the canonical name on the plugintag so that we can use the same string for the other bug.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> I think you want RFind instead of Find to find the last dot, in case there
> is more than one?

Well, I was thinking it would catch something like libplugin.so.2.0.0, but it looks like that's not a valid filename for a plugin anyway, so I'll change it.

> I wonder if there's a straightforward way to test this. We'll also probably
> want to expose the canonical name on the plugintag so that we can use the
> same string for the other bug.

We can test for the two plugins we have - they should map to the same thing each time. The only other thing I can think of is to rename the files on disk in an xpcshell test or something and re-trigger a plugin scan.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 712639 [details] [diff] [review]
patch v2

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

::: dom/plugins/base/nsPluginTags.cpp
@@ +446,5 @@
> +  int32_t niceNameLength = mFileName.Find(".");
> +  NS_ASSERTION(niceNameLength != kNotFound, "mFileName doesn't have a '.'?");
> +  while (niceNameLength > 0) {
> +    char chr = mFileName[niceNameLength - 1];
> +    if ((chr < 'A') || (chr > 'Z' && chr < 'a') || (chr > 'z'))

You could use std::isalpha() here.

Comment 11

[Benjamin Smedberg]

I'm going to take this for the UR study so that Flash updates don't accidentally change permission halfway through the study.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v3

Changed to make use of std::isalpha. Also added some tests. The tests play some tricks with the plugin library files that I'm not sure are okay, so let me know if that's going to work... (also, if you have ideas for more test cases to run, I'd appreciate that too)

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 713504 [details] [diff] [review]
patch v3

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

Looks good, i have the patch for 830267 running on top of this.

::: dom/plugins/test/unit/test_nice_plugin_name.js
@@ +107,5 @@
> +  do_check_true(gIsWindows || gIsOSX || gIsLinux);
> +  do_check_true(!(gIsWindows && gIsOSX) && !(gIsWindows && gIsLinux) &&
> +                !(gIsOSX && gIsLinux));
> +
> +  createAppInfo("[e-mail]", "XPCShell", "1", "1.9");

Out of curiosity, as the other unit test don't have this - what requires the AppInfo?

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

The plugin host asks the blocklist service if the plugin corresponding to a mime type is ctp-blocklisted in determining the permission string (it's the difference between "plugin:blah" and "plugin-vulnerable:blah"). I guess I could make a fake blocklist service and register it, but since creating an AppInfo worked in toolkit/mozapps/extensions/test/xpcshell/test_pluginBlocklistCtp.js, that's what I did here.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Attachment: patch v4

So i completely missed that the plugin filename has to be of the form "np*.dll".
Trivially fixed as well as one check. Added a check for the current form of Windows Flash names.

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 714193 [details] [diff] [review]
patch v4

Carrying over r+.

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://tbpl.mozilla.org/?tree=Try&rev=c11a3fc97f64
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1923122c44

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fe1923122c44