740620 - Find a better way to force plugin state updates (blocklist)

Status: RESOLVED FIXED

(Camino Graveyard :: Plug-ins, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Partial fix (v0.5)

+++ This bug was initially created as a clone of Bug #667441 +++

Bug 667441 Comment 0 Stuart Morgan 2011-06-27 09:13:34 EDT

Now that we'll be doing security-based plugin blocking (bug 662666), we really need a more reliable way of forcing plugin state updates when Camino is updated. I'll look again to see if I can find a way of poking the plugin system; failing that maybe we'll delete pluginreg.dat on the first quit after each update, so the second launch will rebuild (although I'd like to avoid it if we did an autoupdate and deleted it *before* the update already)

Flags: camino2.1+


Bug 667441 Comment 1 Smokey Ardisson 2011-06-27 14:55:12 EDT

One other thought I had: camino.disabled_plugin_names works "live"; can we use the same mechanism that pref uses to make Core learn about state changes to learn about state changes on launch due to blocklist updates?


Bug 667441 Comment 2 Smokey Ardisson 2011-09-09 23:58:42 EDT

(Somehow this was the only camino2.1+ bug that wasn't also Camino 2.1 milestone.)

Target Milestone: --- → Camino2.1


Bug 667441 Comment 3 Smokey Ardisson 2011-09-17 03:29:51 EDT

I tried to dig around with this, again with little luck.  It's easy to find where Firefox pushes state updates to pluginHost (in nsBlocklistService.js, via the private copy of |GetPluginBlocklistState|, but the magic incantation that makes the new states take effect is still elusive.

To see if it involved a restart, I tried using various sample blocklists per the assorted fixed bugs (using extensions.blocklist.url, in Fx 3.6 and 3.0, and with a 1s update-check time), to no avail.  The only way I was able to blocklist a plug-in was to edit the on-disk blocklist.xml, at which point Firefox noticed on launch (so, no restart required).

Er, wow; I finally had a network check work; that 1s check interval took about 10 minutes :P  The alert just popped up in front of my Camino window :P  So Fx alerted me QuickTime was blocked (and suggested I restart to completely disable the plug-in).  I declined, and opening the Extensions Manager showed it blocked; visiting Apple's trailers page gave me Apple's "download QT" fallback content (so it's at least showing up as uninstalled to Apple's QT detection routines), but http://www.computerhope.com/issues/ch000591.htm#4 showed me the "blocked for your protection" Gecko placeholder.

So, there's got to be some way of poking the plug-in system into checking for updated blocked plug-ins (just like camino.disabled_plugin_names disables things live), we're just overlooking it somehow.

Some bugs I looked at: 
* bug 330511 (pluginHost support for blocklisted state)
* bug 391633 (blocklist webservice only)
* bug 271559 (hooked the blocklisting support in pluginHost into the blocklist service)
* bug 419582 (re-enabling an unblocklisted plug-in)
* bug 455906 (severities in blocklist)

I also noticed that pluginHost has some interesting methods, Get/SetBlocklisted (which appear never to be called) and UpdatePluginInfo.

Oh, from bug 455906, I wondered about this "state" value that's getting passed around: http://hg.mozilla.org/releases/mozilla-1.9.2/diff/8291b82dd323/toolkit/mozapps/extensions/src/nsBlocklistService.js#l1.489 http://hg.mozilla.org/releases/mozilla-1.9.2/diff/8291b82dd323/modules/plugin/base/src/nsPluginHostImpl.cpp#l1.61

Any chance that's our magic incantation?


Bug 667441 Comment 4 Stuart Morgan 2011-09-26 00:57:55 EDT

Get/SetBlocklisted should be the backing for setting the disabled property on an nsIPluginTag. We could probably manually re-run our own blocklist, but I don't think that really gets us where we really want to be since we'd still have the "stale plugin version" problem.

I still don't see a good way of forcing the plugin system to reconstruct everything if it doesn't think the plugins on disk have changed :(


Bug 667441 Comment 5 Stuart Morgan 2011-09-26 01:00:43 EDT

Although I guess that would change the bug from not blocking dangerous versions to blocking safe versions (if the plugin system has a stale version); maybe we should combine that with the post-first-launch file nuking so that someone annoyed by an overblock who tried quitting and relauching would get the fix.


Bug 667441 Comment 6 Smokey Ardisson 2011-10-13 02:21:42 EDT

I guess maybe Bug 667441 Comment 4/5 are suggesting what I was thinking tonight after tracing the camino.disabled_plugin_names path.

If we had PreferenceManager call a method |updatePluginBlocklistState| (similar to the existing |updatePluginEnableState|) at launch, we could then be able to force an update in PluginHost before Gecko web content encountered a plug-in and decided (not) to ask the blocklist service about the plug-in.  It'd suck to have to duplicate the code in the blocklist service impl (or, I guess, refactor it), but I think it would get us a better result.

And I'd rather us block safe versions accidentally/temporarily than leave dangerous versions unblocked…


Bug 667441 Comment 7 Smokey Ardisson 2012-02-15 23:24:24 EST

So, when writing the patch for bug 727667, it appeared not to be working, and I couldn't figure out why (or what was wrong with my code).  Then I remembered this bug.

Since I was testing with a fresh profile, I should have had a completely fresh pluginreg.dat getting built and consulting our blocklist, but Flash wasn't being blocked.  I launched again with the same profile, and still no luck.  I deleted pluginreg.dat and restarted again with the same profile, still not blocked.  So I don't think anything related to manipulating pluginreg.dat is going to help, since apparently we still aren't being consulted until after Gecko has already decided the plug-in folder/plug-ins haven't changed:

(In reply to Stuart Morgan from bug 667441 comment #0)
> system; failing that maybe we'll delete pluginreg.dat on the first quit
> after each update, so the second launch will rebuild (although I'd like to
> avoid it if we did an autoupdate and deleted it *before* the update already)

Maybe after getting 2.1.1 out the door I'll have time to do some debugging and see how our camino.disabled_plugin_names makes Core learn about changes and how that differs from the "actual" blocklisting.

Flags: camino2.1.2?


Bug 667441 Comment 8 Smokey Ardisson 2012-02-15 23:42:22 EST

(In reply to bug 667441 comment #7)
> Maybe after getting 2.1.1 out the door I'll have time to do some debugging
> and see how our camino.disabled_plugin_names makes Core learn about changes
> and how that differs from the "actual" blocklisting.

Since I still have the NSLog in from trying to determine if my code is working, I see that we're getting asked about the plug-in each time an instance is started, so something is definitely calling our PluginBlocklistService::GetPluginBlocklistState implementation often.

As soon as I add the plug-in name to "camino.disabled_plugin_names", there's never another call.  There's not a call when I flip the pref (whether there's plug-in content on current pages or not), there's not a call when I load a new page, nothing; the plug-in is just disabled.


Bug 667441 Comment 9 Smokey Ardisson 2012-02-16 02:18:03 EST

Created attachment 597699 [details] [diff] [review]
Partial fix (v0.5)

This sort-of works.

I realized that PreferenceManager's updatePluginEnableState twiddles a bit directly on the pluginTag to turn the plugin off.  Turns out you can also directly twiddle a blocklisted bit, too.

If you just twiddle blocklisted, the plugin starts to load and then "goes away" leaving an empty space (the same thing that happens to a running plugin when you add it to camino_disabled_plugin_names); a reload will then show the "blocked" placeholder.

If you twiddle both blocklisted and disabled bits, it (at least on http://www.adobe.com/software/flash/about/) prevents the start-to-load scenario, but you still need a reload load to see the placeholder.  SetBlocklisted is supposed to set the disabled itself, though…

The other problem is that once we've set the state, we don't get asked again, so if the user during the session sets the dangerous plugin pref during the session, it won't enable said dangerous plugin.  Maybe we're OK with this, since people now (when the blocklist actually works) have to quit and trash pluginreg.dat; otherwise, we could probably do the notification dance.

I'm reasonably convinced that Firefox is doing roughly the same thing in http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/mozapps/extensions/src/nsBlocklistService.js#797-829.

I don't know if we can improve the "no notification on first plug-in instance" case by having PreferenceManager call GetPluginBlocklistState (or some wrapper or helper) early enough in startup that we have a pluginHost but no pages/pluginInstances (does Gecko read the plug-in state before consulting the blocklist on every load?), or if Gecko would still have to load once to check the flags.  Still, this is better than "may not ever work" situation I'm currently experiencing!

--

The "may not ever work" situation is described in the latter half of bug 667441, from which this bug was spun off.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The "10.6 machine" scenario on bug 667441 comment 20 is exactly what situation we're going to be in for 2.1.3 due to blocklisting being broken (though it's also generally what we see when we rev the minimum version), and that's why I think we should call SetBlocklisted ourselves, either by replicating the Firefox logic and calling it around launch (once per version?) or by just including it in our blocklist impl (like the patch here does) and let nsObjectLoadingContent::Instantiate's blocklist call on every plug-in load cause us to propagate our blocked settings into the PluginHost/pluginreg.dat.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I re-ran the blocklist test with Fx3.6 with a QuickTime instance loaded, to see what happens to it when Firefox processes the blocklist update.  Answer: absolutely nothing.  The running instance is still running.

So us calling nsPluginTag::SetBlocklisted inside of our PluginBlocklistService::GetPluginBlocklistState implementation as a response to nsObjectLoadingContent::Instantiate's blocklist call (and getting a start-to-load-but-ends-up-blank) isn't too much worse than the Firefox experience when a blocklist update happens while a plug-in is loaded.

Doing this would cause a bit of duplicated SetBlocklisted'ing in the (rare) cases when PluginHost does call our code on startup, but I think I'm OK with that tradeoff in exchange for making blocklisting of versions-that-become-bad-when-installing-new-Caminos work reliably.

FWIW, if you want to play along at home with the Firefox test, you can set your extensions.blocklist.url to http://www.ardisson.org/smokey/moz/blocklist.xml, which will block all versions of the QT plug-in on Mac OS X.  (I recommend also setting extensions.blocklist.interval to 1 so that you don't wait forever for a check to happen; it uses the same buggy timer impl as everything else in Gecko, which means if you leave Firefox alone, it might eventually run.)

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Thinking about this more, I guess I'm back to the Bug 667441 Comment 4/5/6 idea; I think that's much better than shoving a SetBlocklisted call into our PluginBlocklistService::GetPluginBlocklistState implementation.

We could still condition running an updatePluginBlocklistedState method on "have I already done this for this build" to keep from running it continually; once per build would "solve" the problem of the user's existing plug-in becoming dangerous but not being blocked.  If the user did then update the plug-in, pluginHost would update the blocked state if pluginHost detected a new version, but if pluginHost didn't detect a new version, re-rerunning a updatePluginBlocklistedState method wouldn't help.  Only the pluginreg.dat nuking code is going to help there, but I don't know that we want that running on every launch, either.

(As an added bonus, I *think* that would allow the "allow dangerous plug-ins" to work without requiring nuking pluginreg.dat; if we also wired up a pref change observer for that pref, we could probably also make it take effect on demand via about:config; I don't know if we care enough about that case to implement it, though.)

If I have any time this weekend, I'll see about trying to implement all of this, rather than just continually blathering on about it ad nauseam.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Call the blocklist service on during Gecko pref sync, v0.1

Here's the meat of the fix for this, running the blocklist ourselves during startup (inside syncMozillaPrefs), which was pretty easy to do.  It's the last 20% that's going to be more of a pain.

This part (which depends on the fix for bug 667441 to make us have a BlocklistService before we get to syncMozillaPrefs) definitely ensures that someone who's already running a version of a plug-in that should be blocklisted will have that plug-in blocklisted, regardless of whether PluginHost thinks it needs to consult the BlocklistService.

I thought about trying to share code between updatePluginEnableState and updatePluginBlockedState, but it looked like the combined method was going to be an ugly series of |if|s in order to be able to share the pluginTag-fetching code, so I went with a separate method.

Stuart, do you think we should only run this code once per build?  And can you give me any pointers on how to implement post-first-run deletion of pluginreg.dat (especially in such a way that we don't do it if we've just done it on autoupdate from bug 599084)?

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #4)
> I thought about trying to share code between updatePluginEnableState and
> updatePluginBlockedState, but it looked like the combined method was going
> to be an ugly series of |if|s in order to be able to share the
> pluginTag-fetching code, so I went with a separate method.

The other thing here is that now we're running plug-in enumeration *twice* during startup; hopefully PluginHost caches the PluginTags once it's made them?

Making a single method wouldn't necessarily help with that, though, since different places need to call the method at different times post-startup (e.g., in response to the pref change listener for camino.disabled_plugin_names, we only need to re-do the disabled twiddling, not the blocked twiddling).  At startup we could certainly run both at once, but afterwards I'm not sure how we'd only run one half if we used a combined method that ran both blocklisting and disabling concurrently at startup…

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to  comment #4)
> you give me any pointers on how to implement post-first-run deletion of
> pluginreg.dat (especially in such a way that we don't do it if we've just
> done it on autoupdate from bug 599084)?

So, Steven finally has a patch to fix bug 313700; if it's backportable (even just to our relbranch) and works, that would solve the need to delete pluginreg.dat here (and can let us remove the deleting added in bug 599084).

However, as I've noted earlier, when we ship a Camino update that now considers the user's unchanged, currently installed plug-in version to be dangerous, we still need this patch to blocklist that plug-in (because pluginHost won't re-scan and consult the blocklist in this scenario)--and we need it to undo the damage that bug 667441 has caused.

Comment 7

[Stuart Morgan]

Per my comments in the other bug, we should make this new method public and drive it from MC after the component registration. In fact... we could call it from checkForProblemAddOns, which gives us the once-per-version behavior for free (and seems relevant, since plugins that we block sound like problem add-ons to me ;) )

Actually, here's a potentially *much* better thought that address both parts of this bug: The reason we do the pre-upgrade-shutdown dance is that I was afraid to delete a file out from under Gecko. But now that I know a lot more about our startup, I know that before a certain point we haven't even told Gecko where the profile folder *is*, so it clearly hasn't read it yet. And I'm looking at the very tempting code after the CheckCompatibility call in PreferenceManager, and can't help but notice that its purpose is to delete some registration files in the profile before initing embedding any time the version has changed. Which, coincidentally, is exactly what we want to do as well (although for slight different reasons).

So I'm thinking we can fix all upgrade cases, for first launch (vs. second-ish), simply by adding pluginreg.dat to kVolatileProfileFiles (and the removing the then-pointless hack I put into MC for the update case). No shutdown dance, no extra code to force-re-examine the blocklist manually.

Can you give that a shot to make sure I'm not crazy?

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Make pluginreg.dat a volatile file to facilitate plug-in and blocklist updates

Yeah, that works nicely.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Backport Info.plist-based timestamp detection

And combined with this backport of Steven's Info.plist-based timestamp detection, we ought to reliably pick up when a user upgrades from a blocked version to an unblocked one and actually unblock (since Gecko will note a changed plug-in and do the dance).

I haven't tested the whole scenario through to blocklisting, but I did verify that editing the version number in the Flash Info.plist between runs of the same unrecompiled binary did allow about:plugins to pick up the edited version number.

It's a fairly straight backport of attachment 613403 [details] [diff] [review] from bug 313700, but I'd appreciate a sanity-check.  Besides the obvious file-location changes, I had to do some trivial changes for the PRBool->bool change in 2.0, Josh's sorting change in 7.0 (bug 653116) and the merging of the LocalFile impls in 2.0 (bug 571193.

The sorting change changed the variable names in PluginHost, but I think what is being passed is still identical.  The LocalFile impl merge caused the Mac functions to use the CHECK_mPATH macro from LocalFileUnix; I used the CHECK_INIT macro that the LocalFileOSX functions used pre-merge.  Those are the two things I'm most concerned about from a correctness/no-unforeseen-crashes standpoint (because the code does compile and appears to work otherwise).

Comment 10

[Stuart Morgan]

Comment on attachment 618943 [details] [diff] [review]
Make pluginreg.dat a volatile file to facilitate plug-in and blocklist updates

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

Woot! I wish I'd figured this out a lot sooner and saved us a bunch of trouble :(
sr=smorgan

Comment 11

[Stuart Morgan]

Comment on attachment 618947 [details] [diff] [review]
Backport Info.plist-based timestamp detection

Yep, looks good to me.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/9622d4f4367c for the Camino changes.

I'll land the Gecko fix once I have a place to put it and enough time to make sure I can fix something if it breaks.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #12)
> I'll land the Gecko fix once I have a place to put it and enough time to
> make sure I can fix something if it breaks.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0f6dd57eed4c (on CAMINO_2_1_SECBRANCH)

Nothing builds that branch yet, but hopefully tomorrow.  You can
hg update -r CAMINO_2_1_SECBRANCH
to switch to that branch.  (hg pull -u at that point *should* keep you on that branch until you update -r someotherbranch.)

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #13)
> (In reply to comment #12)
> > I'll land the Gecko fix once I have a place to put it and enough time to
> > make sure I can fix something if it breaks.
> 
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0f6dd57eed4c (on
> CAMINO_2_1_SECBRANCH)
> 
> Nothing builds that branch yet, but hopefully tomorrow.

Tonight's nightly should include this fix.

> hg update -r CAMINO_2_1_SECBRANCH

As philippe discovered and I had forgotten in the time between creating the branch and writing this comment, that won't work if you haven't pulled the changeset for that branch yet.

Either

hg pull -r CAMINO_2_1_SECBRANCH

or

hg pull && hg update -r CAMINO_2_1_SECBRANCH

should work.  Use

hg branch

to verify that your Gecko working directory is on CAMINO_2_1_SECBRANCH rather than on default.