488181 - simplify code for exposing plugin paths/names

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

Attachment: dump plugin filename field

On UNIX systems not including Mac OS X, the "navigator.plugins" array exposes full plugin paths which can contain user names when a plugin is installed under a user directory. Easy to write a script that looks for paths starting with user dir components ("/home/", "\Users\") and then guesses that the next path component is a username.

This might also apply to Windows systems, I haven't tested there yet.

This is related to bug 88183.

Comment 1

[Josh Aas]

Attachment: fix v1.0

This seems like the right way to fix the problem, I haven't fully tested this yet so not requesting review.

Comment 2

[Daniel Veditz [:dveditz]]

I'm not seeing this, not on Ubuntu, Mac or Windows. Unless I set the expose_full_path pref to true (which is useful for diagnosing problems) I get just the file name on Firefox 2.0.0.x, 3.0.8, and today's Shiretoko nightly.

On which versions are Unix builds showing full paths?

Comment 3

[Daniel Veditz [:dveditz]]

Tried today's mozilla-central build on Linux, too.

Comment 4

[Josh Aas]

Attachment: fix v1.1

Build fix.

Comment 5

[Josh Aas]

I am unable to reproduce now too, but I've seen this on Linux and iirc Mac OS X too, though I originally reported not having seen it on Mac OS X. In some cases throwing away pluginreg.dat seems to fix the problem. I'm not sure how this is happening any more, I'll keep digging.

Comment 6

[Johnny Stenback (:jst)]

Not blocking until we understand more about what's going on here. Please renominate if more information turns up here.

Comment 7

[(not currently active) Ted Mielczarek]

Josh had me test this on my Ubuntu machine, but somehow I wound up with the plugins.expose_full_path pref set to "true" on the profile there, so the results were confusing. Re-testing with a stock Ubuntu 3.0.8 with a fresh profile gives me just filenames.

Comment 8

[Josh Aas]

Attachment: fix v1.2

Whether or not this turns out to be an issue we should probably take this patch to simplify things. This update just adds a couple more fixes.

Comment 9

[Daniel Veditz [:dveditz]]

I guess if you take away the debugging pref people can use pluginreg.dat to figure out where plugins are getting loaded from, now that that file isn't an unreadable binary blob. (It's been text for a long while, but wasn't when I fixed bug 88183).

I'm not convinced this is a security bug if full paths are being exposed only when the pref is used as intended, and so far we don't really have evidence to the contrary. If there's software flipping the pref without considering the privacy implications that might be a bug in that other code. Not that I mind if you simplify code, just trying to avoid hiding bugs that don't need it.

Comment 10

[Josh Aas]

I was never under the impression that the pref itself posed any risk. I saw the full paths exposed on one of my systems and then I asked Ted to confirm, neither of us knowing that he had "plugins.expose_full_path" set to true at the time. The idea behind the patch here was to kill off any way for full paths to leak into navigator.plugins.

I can't reproduce any more and neither can Ted, and I don't see an obvious problem in our code, so we should not consider this to be a bug any more as far as I'm concerned.

My simplification patch makes this code much easier to read and I think there are much better ways to deal with debugging plugin paths than the full paths pref. We should kill off that pref and just expose full paths in "about:plugins" for example.

Comment 11

[Daniel Veditz [:dveditz]]

I'm going to assume your prefs got flipped along the way then. If there's no security vulnerability a code-cleanup bug doesn't need to be hidden.

Comment 12

[Josh Aas]

Changing the title to reflect the purpose of the patch here. I'll file a bug about exposing full paths in about:plugins.

Comment 13

[Josh Aas]

Attachment: fix v1.3

Comment 14

[Josh Aas]

Attachment: fix v1.4

forgot another small fix

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 372698 [details] [diff] [review]
fix v1.4

- In modules/libpref/src/init/all.js:

-pref("plugin.expose_full_path", false); // if true navigator.plugins reveals full path

I'd be fine with removing this once we have a change to about:plugins to always show full paths, but until then I think we should leave this pref in, and make DOMPluginImpl::GetFilename() simply check the pref and return either the full path or only the filename. I don't see this as anything to worry about from a security point of view, and I'd hate to loose the convenient way to find out what plugins are actually being loaded by looking at about:plugins.

r+sr=jst with the pref and pref check left in.

Comment 16

[Josh Aas]

Attachment: fix v1.5

Add pref back, fix crash in UNIX impl.

Comment 17

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/7cd22106e8d9

Comment 18

[Peter Weilbacher]

There was a mixup of variable names in the OS/2 part of this patch. I therefore pushed http://hg.mozilla.org/mozilla-central/rev/accd95d7ba9d to fix the resulting build break (I adapted the existing variable names to the ones added in this patch).

Comment 19

[Peter van der Woude [:Peter6]]

it looks like this caused plugins not to load anymore on Win32, bug 493375

Comment 20

[Dave Garrett]

Is what's exposed via nsIPluginTag.filename changing with this on any OS?

Comment 21

[Josh Aas]

(In reply to comment #20)
> Is what's exposed via nsIPluginTag.filename changing with this on any OS?

Yes - on Windows and Linux it will always be the actual file name, never a full path. Is that a problem for some reason?

Comment 22

[Josh Aas]

I backed everything out due to bug 493375.

http://hg.mozilla.org/mozilla-central/rev/6e86393b03ad
http://hg.mozilla.org/mozilla-central/rev/706d26a82761 (merge)
http://hg.mozilla.org/mozilla-central/rev/687daa472dd4
http://hg.mozilla.org/mozilla-central/rev/01962a35aa90 (merge)
http://hg.mozilla.org/mozilla-central/rev/7ed22f21d95d (merge)

Comment 23

[Josh Aas]

Attachment: fix v1.6

This is still not quite ready, plugin unit test still fails on Linux.

Comment 24

[Bill Gianopoulos [:WG9s]]

Not sure why the test failed, but the issue I filed bug 493413 was Linux only and I attached a patch to fix it.

Comment 25

[Dave Garrett]

(In reply to comment #21)
> (In reply to comment #20)
> > Is what's exposed via nsIPluginTag.filename changing with this on any OS?
> Yes - on Windows and Linux it will always be the actual file name, never a full
> path. Is that a problem for some reason?

I use it in one of my extensions, Crash Report Helper. Not a problem though; just makes it in-line with Mac OS X behavior so I shouldn't need to change anything to handle it.

This should probably be added to the changes for developers MDC page. (3.5's if it lands there, or just Firefox.next's whenever it gets one)

Comment 26

[Josh Aas]

Attachment: fix v1.7

Includes fix from Bill Gianopoulos, thanks Bill!

Comment 27

[Josh Aas]

Attachment: fix v1.8

Clean up more file name/path confusion.

Comment 28

[Josh Aas]

Attachment: fix v1.9

This fixes the Linux unit test failure. Turns out the unit test was always writing out a Mac OS X v0.9 registry, so the test only worked on Mac OS X. The problem here sort of gets to the heart of this bug - our usage of file name vs. path is really confusing. With this patch, all registry fields mean the same thing on all platforms.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 378244 [details] [diff] [review]
fix v1.9

>+++ b/modules/plugin/base/src/nsPluginsDirBeOS.cpp
>+    nsCAutoString fullPath;
>+    if (NS_FAILED(rv = mPlugin->GetNativePath(path)))

That should be fullPath.

>+++ b/modules/plugin/base/src/nsPluginsDirWin.cpp

>+  NS_ConvertASCIItoUTF16 utf16Path(fullPath);

Can you please file a followup bug on this?  It looks pretty bogus to me...

sr=me with those nits.

Comment 30

[Josh Aas]

Attachment: fix v2.0

pushed this to mozilla-central, includes BeOS fix that bz pointed out

http://hg.mozilla.org/mozilla-central/rev/8fce20dd0cbe