471245 - Unpacked extensions inadvertently enable plugins when reading chrome file at startup

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Eric Jung [:ericjung]]

The attached extension (addon) is a minimal example that reads a chrome file at startup from an XPCOM component. If the extension is packaged with its content in a jar, there are no problems. However, if the extension is packaged "flat"; i.e., no jar, then any disabled plugins (note that I said plugins, not addons/extensions) are enabled at startup.

Tested with Flash 10.0.r12. Mozilla Default Plug-in, and the Microsoft Office 2003 plugins, all of which exhibit this behavior. The only exception that I've found is the Java Plug-in 1.6.0_07 which remains disabled after restarts.

This problem does not occur on Firefox 3.0.3 on Ubuntu 8.10.

I could really use a workaround. Packaging the extension with a jar isn't an option due to bug 456705.

Comment 1

[Eric Jung [:ericjung]]

Attachment: extension without jar -- causes the bug

Comment 2

[Eric Jung [:ericjung]]

Attachment: extension with jar -- no bug

Comment 3

[Dave Garrett]

Comment on attachment 354539 [details]
extension with jar -- no bug

Attachments are identical. You uploaded the same file twice by mistake. ;)

Comment 4

[Dave Garrett]

Attachment: extension with jar -- no bug

What you meant; repackaged with jar.

I can reproduce this. First attachment resets disabled status of Flash 10 plugin on startup and second does not. (Mozilla build of Firefox 3.0.5 on Kubuntu 8.04.1)

You said it doesn't work on "Firefox 3.0.3 on Ubuntu 8.10". Is that with the Ubuntu build or Mozilla's?

Comment 5

[Eric Jung [:ericjung]]

> You said it doesn't work on "Firefox 3.0.3 on Ubuntu 8.10". Is that with the
> Ubuntu build or Mozilla's?

That's with Ubuntu's build. Interesting that you've reproduced it with Kubuntu.

Comment 6

[Eric Jung [:ericjung]]

Attachment: extension with jar -- no bug

Comment 7

[Eric Jung [:ericjung]]

(In reply to comment #4)
> Created an attachment (id=354542) [details]
> extension with jar -- no bug

Dave, I obsoleted your attachment with mine because mine contains precisely the same files as the other jar (yours had things like build.sh and a readme, etc, which I know are from a template, but I want to keep the example minimal so people don't focus on the wrong items)

Comment 8

[Dave Garrett]

(In reply to comment #7)
> (In reply to comment #4)
> > Created an attachment (id=354542) [details] [details]
> > extension with jar -- no bug
> 
> Dave, I obsoleted your attachment with mine because mine contains precisely the
> same files as the other jar (yours had things like build.sh and a readme, etc,
> which I know are from a template, but I want to keep the example minimal so
> people don't focus on the wrong items)

Uh... no it doesn't. Mine and yours contain the exact same contents. I even just downloaded yours and mine, extracted both XPIs and their JARs and diffed the whole thing. There isn't a single difference. (I even named the JAR the same as you) I'm not sure what you're talking about. I see no build.sh or readme or anything anywhere here.

(In reply to comment #5)
> > You said it doesn't work on "Firefox 3.0.3 on Ubuntu 8.10". Is that with the
> > Ubuntu build or Mozilla's?
> 
> That's with Ubuntu's build. Interesting that you've reproduced it with Kubuntu.

Then this bug just may not be present in their builds for some reason. Odd.

Comment 9

[Dave Garrett]

The one difference between the XPIs is that you compressed the files in your JAR and I did not, because leaving them uncompressed actually leads to a better end compression ratio for the overall XPI and a smaller file size. It's basically a poor-man's solid archive that way. (useless here, granted, but that's just how I always pack XPIs) Other than the compression specifics the contents are identical.

Comment 10

[Eric Jung [:ericjung]]

Same bug exists on nightly builds (3.2a1pre). Should I change version to 1.9.1 branch?

(In reply to comment #8)
> Uh... no it doesn't. Mine and yours contain the exact same contents.

You're right, of course. I have too many versions of this XPI floating around my hard drive, and obviously I diff'd the wrong one. I do apologize.

Comment 11

[Dave Garrett]

(In reply to comment #10)
> Same bug exists on nightly builds (3.2a1pre). Should I change version to 1.9.1
> branch?

3.2a1pre -> Trunk

(In reply to comment #10)
> (In reply to comment #8)
> > Uh... no it doesn't. Mine and yours contain the exact same contents.
> 
> You're right, of course. I have too many versions of this XPI floating around
> my hard drive, and obviously I diff'd the wrong one. I do apologize.

Not a problem; it happens. ;)

Comment 12

[Dave Garrett]

It's in 3.1b3pre too, which is fairly obvious if it's in trunk and 1.9.0 branch.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

By "nightly builds" I'm assuming you mean mozilla.org builds (as opposed to ubuntu builds?)

Comment 14

[Eric Jung [:ericjung]]

(In reply to comment #13)
> By "nightly builds" I'm assuming you mean mozilla.org builds (as opposed to
> ubuntu builds?)

Right. I tested against http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-3.2a1pre.en-US.win32.zip. It would appear it's a problem on Ubuntu, too, providing you don't use Ubuntu's builds.

Comment 15

[Daniel Veditz [:dveditz]]

These are bizarre symptoms, what does the chrome registry and libjar have to do with plugins? Any useful Error console spew? I imagine not or you'd have said, guess I'll have to run this in debug.

Comment 16

[Dave Garrett]

Nope, no errors to speak of. Only messages are the ones dumped to the terminal from the tests:
reenablepluginproblem.js
***** Stream read succeeded
*** app-startup
*** profile-after-change

Same for both versions.

Comment 17

[Eric Jung [:ericjung]]

(In reply to comment #15)
> Any useful Error console spew?

Unfortunately, no.

Comment 18

[Dave Townsend [:mossop]]

The problem is basically one of trying to do things too early in the startup sequence. The component attempts to read data from a chrome channel during the app-startup topic. Through the following stack we see this causes the plugin host to be queried. It attempts to load the plugin registry but because this is too early for the profile folder to be defined it fails. For some reason it ignores any failure to read the registry, goes on to redetect all installed plugins and probably at some point later when the profile folder is available overwrites the registry.

#0	0x14c401ab in nsPluginHostImpl::FindPlugins at nsPluginHostImpl.cpp:5042
#1	0x14c405e6 in nsPluginHostImpl::LoadPlugins at nsPluginHostImpl.cpp:5003
#2	0x14c2f587 in nsPluginHostImpl::FindPluginEnabledForExtension at nsPluginHostImpl.cpp:4253
#3	0x14c2f673 in nsPluginHostImpl::IsPluginEnabledForExtension at nsPluginHostImpl.cpp:4007
#4	0x148f9a09 in nsExternalHelperAppService::GetTypeFromExtension at nsExternalHelperAppService.cpp:2581
#5	0x148f9668 in nsExternalHelperAppService::GetTypeFromFile at nsExternalHelperAppService.cpp:2717
#6	0x00d26a66 in nsFileChannel::MakeFileInputStream at nsFileChannel.cpp:302
#7	0x00d27208 in nsFileChannel::OpenContentStream at nsFileChannel.cpp:371
#8	0x00c72b0c in nsBaseChannel::Open at nsBaseChannel.cpp:505
#9	0x14b4b249 in NS_OpenURI at nsNetUtil.h:215
#10	0x14b47425 in nsExpatDriver::OpenInputStreamFromExternalDTD at nsExpatDriver.cpp:823
#11	0x14b475c3 in nsExpatDriver::HandleExternalEntityRef at nsExpatDriver.cpp:734
#12	0x14b478f5 in Driver_HandleExternalEntityRef at nsExpatDriver.cpp:214
#13	0x14b7c88a in doProlog at xmlparse.c:4010
#14	0x14b7bfd8 in prologProcessor at xmlparse.c:3809
#15	0x14b7ba5c in prologInitProcessor at xmlparse.c:3626
#16	0x14b76e6d in MOZ_XML_Parse at xmlparse.c:1528
#17	0x14b45180 in nsExpatDriver::ParseBuffer at nsExpatDriver.cpp:1025
#18	0x14b482cb in nsExpatDriver::ConsumeToken at nsExpatDriver.cpp:1128
#19	0x14b6218e in nsParser::Tokenize at nsParser.cpp:3041
#20	0x14b63c97 in nsParser::ResumeParse at nsParser.cpp:2249
#21	0x14b6351c in nsParser::OnDataAvailable at nsParser.cpp:2903
#22	0x1340c63a in nsDOMParser::ParseFromStream at nsDOMParser.cpp:299

Comment 19

[Dave Townsend [:mossop]]

I'm not totally sure why using a jar or not affects this, but the extension could avoid it by waiting until profile-after-change to perform its work. We could also put code into the plugin host to pretend there are no plugins installed until it can read the registry, I think that has impact on embedders though.

Comment 20

[Eric Jung [:ericjung]]

(In reply to comment #19)
> but the extension could avoid it by waiting until profile-after-change to 
> perform its work. 

Right. See line 25 of reenablepluginproblem.js in the attached XPI. It contains a commented-out this.read().

(In reply to comment #18)
> The component attempts to read data from a chrome channel during the
> app-startup topic. Through the following stack we see this causes the plugin
> host to be queried

Why? Is that appropriate?

Comment 21

[Dave Townsend [:mossop]]

> (In reply to comment #18)
> > The component attempts to read data from a chrome channel during the
> > app-startup topic. Through the following stack we see this causes the plugin
> > host to be queried
> 
> Why? Is that appropriate?

Specifically the stack shows us trying to guess a mimetype for a file extension. Plugins are one source of mimetype information.

Comment 22

[Dave Garrett]

Interesting. Any guesses as to why Ubuntu's builds don't seem to be affected?

Comment 23

[[e-mail]]

Bug (disabled plugins become enabled, essentially being unable to disable plugins) occurs also under Firefox 3.0.5 under Windows Visa when Foxyproxy is on, and everything is OK when I turn it off.

Comment 24

[Dave Garrett]

(In reply to comment #23)
> Bug (disabled plugins become enabled, essentially being unable to disable
> plugins) occurs also under Firefox 3.0.5 under Windows Visa when Foxyproxy is
> on, and everything is OK when I turn it off.

Makes sense, seeing as the author of that extension filed this report.  ;)

Comment 25

[Eric Jung [:ericjung]]

(In reply to comment #24)
> Makes sense, seeing as the author of that extension filed this report.  ;)

I was asked by timeless to file this as a new bug and attach a minimal test case.

Comment 26

[Boris Zbarsky [:bzbarsky]]

OK, so the reason jar vs non-jar matters is that file:// channels determine the content type at open time (per the above stack) while jar:// channels determine it only if someone asks for it.  Since this particular caller uses a sync Open() on the channel, and never asks for its type, it doesn't trigger this bug.

It seems that the real issue here is that the plugin host uses profile data but doesn't listen for profile changes.  If it did, the profile change when we finally activate our profile would cause it to reread the correct plugin data, right?

Comment 27

[Dave Townsend [:mossop]]

(In reply to comment #26)
> It seems that the real issue here is that the plugin host uses profile data but
> doesn't listen for profile changes.  If it did, the profile change when we
> finally activate our profile would cause it to reread the correct plugin data,
> right?

That is one way of looking at it. Another is to say that the plugin host shouldn't be making any claims about the state of or presumably allowing the use of plugins until the profile and other key services are up and running, otherwise it may allow the instantiation of a user disabled or even blocklisted plugin.

Comment 28

[Boris Zbarsky [:bzbarsky]]

I think both are true, and we should fix both.

Comment 29

[Eric Jung [:ericjung]]

Google Toolbar, which is not a plugin, is affected by this bug.

Expected behavior:
1. Install Google Toolbar http://www.google.com/tools/firefox/toolbar/FT3/intl/en/ and restart FF to complete the install
2. Perform some arbitrary searches using the toolbar
3. Open %profile%/GoogleToolbarData/searchhistory.xml
4. Close FF and watch searchhistory.xml get updated with the searches from step 2.

Buggy behavior:
5. Install the problematic reenableplugins.xpi and restart FF to complete the install
6. Perform some arbitrary searches using the toolbar
7. Open %profile%/GoogleToolbarData/searchhistory.xml
8. Close FF. searchhistory.xml isn't updated with the searches from step 6

Comment 30

[Boris Zbarsky [:bzbarsky]]

It's quite possible that it too should be watching for profile change.

The fact of the matter is, trying to use networking before the profile is set up _will_ initialize all sorts of stuff before profile setup, and then if said stuff doesn't watch for profile change it _will_ break when the profile is finally set up.

Comment 31

[Eric Jung [:ericjung]]

(In reply to comment #30)
> It's quite possible that it too should be watching for profile change.
> 
> The fact of the matter is, trying to use networking before the profile is set
> up _will_ initialize all sorts of stuff before profile setup, and then if said
> stuff doesn't watch for profile change it _will_ break when the profile is
> finally set up.

OK, but if you read commend #30 more closely, you'll se the issue is that google's searchhistory.xml isn't updated _at shutdown_ although it should be

Comment 32

[Boris Zbarsky [:bzbarsky]]

I did read closely.  If they grab the location at startup, then don't update it on profile change, they will write to the wrong place.  That's my best guess for what's going on there.

Comment 34

[u88484]

So what is needed to fix this bug?  The extension/plug-ins to fix their code?  Can something be done in Firefox to prevent this from happening?  Should an error message be relayed to the user through the add-ons manager or the notification bar that the plug-in failed to disable?

Nominating for blocking due to users thinking a plug-in they specifically told to disable (possibly for a security issue), re-enables upon restart and the user could potentially be exposed to a security threat they were trying to avoid.

Comment 35

[Boris Zbarsky [:bzbarsky]]

See comment 26 through comment 28 for what we should be doing on our end to mitigate.

But the original issue is that the extension involved is just buggy: it uses certain components before startup is far enough along to use them safely; the rest is corollaries.

Comment 36

[Nils Maier [:nmaier]]

(In reply to comment #35)
> See comment 26 through comment 28 for what we should be doing on our end to
> mitigate.
> 
> But the original issue is that the extension involved is just buggy: it uses
> certain components before startup is far enough along to use them safely; the
> rest is corollaries.

I guess this needs to be documented properly on MDC? At least I cannot remember reading that using file:/chrome: too early is a no go.
After all, loading a chrome URL actually works, but has side effects that might not be obvious, like this one.

Maybe a way to inform developers of their wrong doing could be implemented, like failing to load chrome:/file: before profile-change or at least spitting out some warning?

The development builds of DownThemAll! are triggering this too; the release builds are "buggy", too, but don't trigger this as chrome stuff is in a jar there. And until now I wasn't actually aware of this side effect.
We have some legacy "XPCOMUtils like" thingie we load as a subscript; from times where there was no XPCOMUtils.jsm yet for all platforms/apps we support.
I could imagine other scenarios where people try to load chrome:/file: early, like loading string bundles to localize components.

Another observation:
In my dTa! development profile (fx 3.5.2/win) disabling plugins just works as expected, although there is "flat" packaging. The extension is installed by an extension proxy file pointing to the real location of the extension files.
So this bad side effect isn't even triggered reliably in "flat" chrome and hence even less likely to get noticed by extension developers.

Comment 37

[Eric Jung [:ericjung]]

(In reply to comment #36)
> I could imagine other scenarios where people try to load chrome:/file: early,
> like loading string bundles to localize components.

FYI, this is precisely how FoxyProxy used to trigger this bug, and why this bugzilla bug was opened.

Comment 38

[Johnny Stenback (:jst)]

Blocking. Dave, do you think you could work on this? If not, please let me know.

Comment 39

[Dave Townsend [:mossop]]

Does the plugin host need to operate in some cases where there is never going to be a profile folder (some embedding scenarios f.e.)? If not then I ought to be able to come up with a fix for this, if so then I'd need to know how to detect those cases.

Comment 40

[Dave Townsend [:mossop]]

bsmedberg says that if the directory service key NS_APP_PROFILE_DIR_STARTUP exists then there will be a profile. So if that exists and there isn't a profile directory currently we should block attempts to load or scan for plugins.

Comment 41

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This seems like the simplest approach. In ReadPluginInfo if ProfD is unavailable but ProfDS is then it is an application that will have a profile directory but it is too early in startup for it to be available. In this case return a specific error that stops any attempt to scan for plugins, making it appear for the time being that there are none installed. Once the profile is available the next call in will naturally load the registry cache and scan for new plugins.

I've verified that this solves the problem for the test plugin and included a simple unit test to verify that the test plugin is unknown before the profile directory is defined.

Comment 42

[Dave Townsend [:mossop]]

Fixed on trunk: http://hg.mozilla.org/mozilla-central/rev/e259f581ac08

Comment 43

[Nickolay_Ponomarev]

Is a follow-up needed for bz's suggestion in comment 26 (make plugin host listen for profile changes)?

Comment 44

[Dave Townsend [:mossop]]

(In reply to comment #43)
> Is a follow-up needed for bz's suggestion in comment 26 (make plugin host
> listen for profile changes)?

No the plugin host will simply read the data from the profile the first time it is called after the profile is available.

Comment 45

[Johnny Stenback (:jst)]

Mossop, can this land for 1.9.2 now?

Comment 46

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bae033362017

Comment 47

[Dave Townsend [:mossop]]

Backed this out while I investigate the test failures.

Comment 48

[Dave Townsend [:mossop]]

Relanded, this turned out to just be a bad merge: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/80a00c027eee