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

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CtPDefault:P2][CtPUR:+])

Attachments

(1 attachment, 3 obsolete attachments)

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

6 years ago
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?
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.
Whiteboard: [CtPDefault:P2]
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #711597 - Flags: review?(benjamin)

Comment 4

6 years ago
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?)
Attachment #711597 - Flags: review?(benjamin) → review-
What about e.g. npfoo3d.dll vs. npfood.dll? 
Could we just strip trailing digits & punctuation after stripping the extension?

Comment 6

6 years ago
Yes, that would be fine too. Too bad we can't use regexes in C++ ;-)
Posted patch patch v2 (obsolete) — Splinter Review
How does this look?
Attachment #711597 - Attachment is obsolete: true
Attachment #712639 - Flags: review?(benjamin)
Summary: nsPluginHost::GetPermissionStringForType should return a key based on MIME type, not plugin filename → nsPluginHost::GetPermissionStringForType should be more robust against some plugin filename changes

Comment 8

6 years ago
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.
Attachment #712639 - Flags: review?(benjamin) → review+
(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 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.
I'm going to take this for the UR study so that Flash updates don't accidentally change permission halfway through the study.
Whiteboard: [CtPDefault:P2] → [CtPDefault:P2][CtPDefault:+]
Whiteboard: [CtPDefault:P2][CtPDefault:+] → [CtPDefault:P2][CtPUR:+]
Posted patch patch v3 (obsolete) — Splinter Review
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)
Attachment #712639 - Attachment is obsolete: true
Attachment #713504 - Flags: review?(benjamin)
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("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9");

Out of curiosity, as the other unit test don't have this - what requires the AppInfo?
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.

Updated

6 years ago
Attachment #713504 - Flags: review?(benjamin) → review+
Posted patch patch v4Splinter Review
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.
Attachment #713504 - Attachment is obsolete: true
Comment on attachment 714193 [details] [diff] [review]
patch v4

Carrying over r+.
Attachment #714193 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fe1923122c44
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.