Closed Bug 488181 Opened 11 years ago Closed 11 years ago

simplify code for exposing plugin paths/names

Categories

(Core :: Plug-ins, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

()

Details

(Keywords: privacy)

Attachments

(2 files, 10 obsolete files)

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.
Attached patch fix v1.0 (obsolete) — Splinter Review
This seems like the right way to fix the problem, I haven't fully tested this yet so not requesting review.
Whiteboard: [sg:low]
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?
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Tried today's mozilla-central build on Linux, too.
Attached patch fix v1.1 (obsolete) — Splinter Review
Build fix.
Attachment #372465 - Attachment is obsolete: true
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.
Not blocking until we understand more about what's going on here. Please renominate if more information turns up here.
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1?
Flags: blocking1.9.1-
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.
Attached patch fix v1.2 (obsolete) — Splinter Review
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.
Attachment #372540 - Attachment is obsolete: true
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.
Whiteboard: [sg:low] → [sg:investigate] ("low" if really a bug)
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.
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.
Group: core-security
Whiteboard: [sg:investigate] ("low" if really a bug)
Changing the title to reflect the purpose of the patch here. I'll file a bug about exposing full paths in about:plugins.
Summary: user names can be exposed via non-global plugin paths → simplify code for exposing plugin paths/names
Attached patch fix v1.3 (obsolete) — Splinter Review
Attachment #372653 - Attachment is obsolete: true
Attachment #372697 - Flags: superreview?(jst)
Attached patch fix v1.4 (obsolete) — Splinter Review
forgot another small fix
Attachment #372697 - Attachment is obsolete: true
Attachment #372698 - Flags: superreview?(jst)
Attachment #372697 - Flags: superreview?(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.
Attachment #372698 - Flags: superreview?(jst)
Attachment #372698 - Flags: superreview+
Attachment #372698 - Flags: review+
Attached patch fix v1.5 (obsolete) — Splinter Review
Add pref back, fix crash in UNIX impl.
Attachment #372698 - Attachment is obsolete: true
Attachment #376637 - Flags: superreview?(jst)
Attachment #376637 - Flags: superreview?(jst)
Attachment #376637 - Flags: superreview+
Attachment #376637 - Flags: review+
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/7cd22106e8d9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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).
it looks like this caused plugins not to load anymore on Win32, bug 493375
Depends on: 493375
Is what's exposed via nsIPluginTag.filename changing with this on any OS?
(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?
Depends on: 493413
Attached patch fix v1.6 (obsolete) — Splinter Review
This is still not quite ready, plugin unit test still fails on Linux.
Attachment #376637 - Attachment is obsolete: true
Not sure why the test failed, but the issue I filed bug 493413 was Linux only and I attached a patch to fix it.
(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)
Attached patch fix v1.7 (obsolete) — Splinter Review
Includes fix from Bill Gianopoulos, thanks Bill!
Attachment #377890 - Attachment is obsolete: true
Attached patch fix v1.8 (obsolete) — Splinter Review
Clean up more file name/path confusion.
Attachment #378059 - Attachment is obsolete: true
Depends on: 493545
Attached patch fix v1.9 (obsolete) — Splinter Review
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.
Attachment #378119 - Attachment is obsolete: true
Attachment #378244 - Flags: superreview?(jst)
Attachment #378244 - Flags: superreview?(jst) → superreview?(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.
Attachment #378244 - Flags: superreview?(bzbarsky) → superreview+
Attached patch fix v2.0Splinter Review
pushed this to mozilla-central, includes BeOS fix that bz pointed out

http://hg.mozilla.org/mozilla-central/rev/8fce20dd0cbe
Attachment #378244 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 494722
You need to log in before you can comment on or make changes to this bug.