simplify code for exposing plugin paths/names

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

({privacy})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 10 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Posted 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.
(Assignee)

Updated

10 years ago
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.
(Assignee)

Comment 4

10 years ago
Posted patch fix v1.1 (obsolete) — Splinter Review
Build fix.
Attachment #372465 - Attachment is obsolete: true
(Assignee)

Comment 5

10 years ago
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.
(Assignee)

Comment 8

10 years ago
Posted 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)
(Assignee)

Comment 10

10 years ago
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)
(Assignee)

Comment 12

10 years ago
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
(Assignee)

Comment 13

10 years ago
Posted patch fix v1.3 (obsolete) — Splinter Review
Attachment #372653 - Attachment is obsolete: true
Attachment #372697 - Flags: superreview?(jst)
(Assignee)

Comment 14

10 years ago
Posted 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+
(Assignee)

Comment 16

10 years ago
Posted patch fix v1.5 (obsolete) — Splinter Review
Add pref back, fix crash in UNIX impl.
Attachment #372698 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #376637 - Flags: superreview?(jst)
Attachment #376637 - Flags: superreview?(jst)
Attachment #376637 - Flags: superreview+
Attachment #376637 - Flags: review+
(Assignee)

Comment 17

10 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/7cd22106e8d9
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 18

10 years ago
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

Comment 20

10 years ago
Is what's exposed via nsIPluginTag.filename changing with this on any OS?
(Assignee)

Comment 21

10 years ago
(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
(Assignee)

Comment 23

10 years ago
Posted 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.

Comment 25

10 years ago
(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)
(Assignee)

Comment 26

10 years ago
Posted patch fix v1.7 (obsolete) — Splinter Review
Includes fix from Bill Gianopoulos, thanks Bill!
Attachment #377890 - Attachment is obsolete: true
(Assignee)

Comment 27

10 years ago
Posted patch fix v1.8 (obsolete) — Splinter Review
Clean up more file name/path confusion.
Attachment #378059 - Attachment is obsolete: true
Depends on: 493545
(Assignee)

Comment 28

10 years ago
Posted 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)
(Assignee)

Updated

10 years ago
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+
(Assignee)

Comment 30

10 years ago
Posted 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
(Assignee)

Updated

10 years ago
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Duplicate of this bug: 494722
You need to log in before you can comment on or make changes to this bug.