Closed
Bug 281284
Opened 20 years ago
Closed 20 years ago
malicious local users can remove mozilla users files (insecure use of /tmp/plugtmp)
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: taviso, Assigned: jst)
References
Details
(Keywords: fixed-aviary1.0.1, fixed1.4.5, fixed1.7.6, Whiteboard: [sg:fix] needs landing)
Attachments
(4 files, 1 obsolete file)
7.57 KB,
patch
|
peterv
:
review+
dveditz
:
superreview+
mkaply
:
approval-aviary1.0.1+
mkaply
:
approval1.7.6+
|
Details | Diff | Splinter Review |
7.27 KB,
patch
|
Details | Diff | Splinter Review | |
7.35 KB,
patch
|
Details | Diff | Splinter Review | |
6.76 KB,
patch
|
dveditz
:
review+
dveditz
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041123 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041123 Firefox/1.0
Mozilla creates the /tmp/plugtmp directory for storing plugin files, on exit, it
empties this directory.
A malicious local user could create a symlink at /tmp/plugtmp to a directory,
when another user runs firefox the directory will be emptied. This is a serious
issue on multiuser systems.
Reproducible: Always
Steps to Reproduce:
1. ln -s /home/victim /tmp/plugtmp
2. wait for victim to run and exit firefox
3. victim just lost the files in his homedir.
Actual Results:
Example:
$ pwd
/home/taviso
$ mkdir test
$ cd test
$ for ((i=0;i<10;i++)); do touch ${RANDOM}.jpg; done
$ ls
10659.jpg 16835.jpg 26339.jpg 4062.jpg 8234.jpg
15120.jpg 22838.jpg 29316.jpg 724.jpg 9053.jpg
# now malicious user wants to remove these files (i'll use user nobody for this
example)
$ sudo -u nobody ln -s /home/taviso/test /tmp/plugtmp
$ ls -l /tmp/plugtmp
lrwxrwxrwx 1 nobody nobody 17 Feb 6 18:43 /tmp/plugtmp -> /home/taviso/test/
# now malicious user waits until I run firefox...
$ firefox
<exit firefox>
$ ls
$ echo "arghhh, my files!"
Expected Results:
perhaps use a mkdtemp()-style directory instead of hardcoded /tmp/plugtmp?
Updated•20 years ago
|
Flags: blocking-aviary1.0.1?
Summary: malicious local users can remove mozilla users files → malicious local users can remove mozilla users files (insecure use of /tmp)
Updated•20 years ago
|
Assignee | ||
Comment 1•20 years ago
|
||
Attachment #173817 -
Flags: superreview?(brendan)
Attachment #173817 -
Flags: review?(dveditz)
Assignee | ||
Comment 2•20 years ago
|
||
Taking bug. The attached patch should fixes this problem, but I've yet to find a
page that uses a plugin in a way that would exercise this code. So I can't
verify that it still works, but it sure does look like it would do the right thing.
Assignee: dveditz → jst
Assignee | ||
Comment 3•20 years ago
|
||
s/should //
Assignee | ||
Updated•20 years ago
|
Summary: malicious local users can remove mozilla users files (insecure use of /tmp) → malicious local users can remove mozilla users files (insecure use of /tmp/plugintmp)
Assignee | ||
Updated•20 years ago
|
Summary: malicious local users can remove mozilla users files (insecure use of /tmp/plugintmp) → malicious local users can remove mozilla users files (insecure use of /tmp/plugtmp)
Reporter | ||
Comment 4•20 years ago
|
||
Hi Johnny, this site uses the feature: http://www.spreadshirt.net/shop.php?
sid=44009
I've applied your patch but without success:
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1084152192 (LWP 1580)]
nsPluginStreamListenerPeer::SetupPluginCacheFile(nsIChannel*) (
this=0x44cbd568, channel=0x44cbde58) at nsCOMPtr.h:848
(gdb) bt
#0 nsPluginStreamListenerPeer::SetupPluginCacheFile(nsIChannel*) (
this=0x4544f378, channel=0x454fe340) at nsCOMPtr.h:848
#1 0x4323c735 in nsPluginStreamListenerPeer::SetUpStreamListener(nsIRequest*,
nsIURI*) (this=0x4544f378, request=0x454fe340, aURL=0x4544f948)
at nsCOMPtr.h:842
#2 0x4323b97c in nsPluginStreamListenerPeer::OnStartRequest(nsIRequest*,
nsISupports*) (this=0x4544f378, request=0x454fe340, aContext=0x0) at nsCOMPtr.h:
842
#3 0x40e835ea in nsHttpChannel::CallOnStartRequest() (this=0x454fe340)
at nsCOMPtr.h:848
#4 0x40e83a7a in nsHttpChannel::ProcessNormal() (this=0x454fe340)
at nsHttpChannel.cpp:847
#5 0x40e8387e in nsHttpChannel::ProcessResponse() (this=0x454fe340)
at nsHttpChannel.cpp:783
#6 0x40e8ba06 in nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) (
this=0x454fe340, request=0x454fec38, ctxt=0x0) at nsHttpChannel.cpp:3687
#7 0x40e1193e in nsInputStreamPump::OnStateStart() (this=0x454fec38)
at nsCOMPtr.h:848
#8 0x40e118ad in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (
this=0x454fec38, stream=0x454feb38) at nsInputStreamPump.cpp:337
#9 0x40117d9a in nsInputStreamReadyEvent::EventHandler(PLEvent*) (plevent=0x0)
at nsCOMPtr.h:848
...
Comment 5•20 years ago
|
||
Comment on attachment 173817 [details] [diff] [review]
Always create unique plugintmp directories for each browser instance, and only delete what was created.
>Index: modules/plugin/base/src/nsPluginHostImpl.cpp
>===================================================================
>+nsresult
>+nsPluginHostImpl::GetPluginTempDir(nsIFile **aDir)
>+{
>+ if (!sPluginTempDir) {
...
>+ tmpDir.swap(sPluginTempDir);
>+
>+ if (aDir) {
>+ sPluginTempDir->Clone(aDir);
>+ }
I think you need to move the above three lines outside of the if
(!sPluginTempDir)
>+ }
>+
>+ return NS_OK;
>+}
Reporter | ||
Comment 6•20 years ago
|
||
Peter: confirmed, making that adjustment fixes it...works perfectly here.
Assignee | ||
Comment 7•20 years ago
|
||
Duh, new patch with that change coming up... Thanks for the testcase, and thanks
for testing!
Assignee | ||
Updated•20 years ago
|
Attachment #173817 -
Attachment is obsolete: true
Attachment #173817 -
Flags: superreview?(brendan)
Attachment #173817 -
Flags: review?(dveditz)
Assignee | ||
Comment 8•20 years ago
|
||
Attachment #173903 -
Flags: superreview?(dveditz)
Attachment #173903 -
Flags: review?(peterv)
Comment 9•20 years ago
|
||
Comment on attachment 173903 [details] [diff] [review]
Fix problem identified by peterv.
>Index: modules/plugin/base/src/nsPluginHostImpl.cpp
>===================================================================
>+nsresult
>+nsPluginHostImpl::GetPluginTempDir(nsIFile **aDir)
>+{
>+ if (aDir) {
>+ sPluginTempDir->Clone(aDir);
I think you should set rv here to propagate the error if Clone fails.
Attachment #173903 -
Flags: review?(peterv) → review+
Comment 10•20 years ago
|
||
definitely want this on the branches
Flags: blocking1.7.6?
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1?
Flags: blocking-aviary1.0.1+
Comment 11•20 years ago
|
||
Comment on attachment 173903 [details] [diff] [review]
Fix problem identified by peterv.
ditto what peter said about passing back Clone()'s rv. You could NS_ENSURE aDir
at the top then just always return the sPluginTempDir clone at the end.
sr=dveditz with some change along those lines.
Attachment #173903 -
Flags: superreview?(dveditz)
Attachment #173903 -
Flags: superreview+
Attachment #173903 -
Flags: approval1.7.6?
Attachment #173903 -
Flags: approval-aviary1.0.1?
Comment 12•20 years ago
|
||
Comment on attachment 173903 [details] [diff] [review]
Fix problem identified by peterv.
a=mkaply for the branches
Attachment #173903 -
Flags: approval1.7.6?
Attachment #173903 -
Flags: approval1.7.6+
Attachment #173903 -
Flags: approval-aviary1.0.1?
Attachment #173903 -
Flags: approval-aviary1.0.1+
Updated•20 years ago
|
Whiteboard: [sg:fix] → [sg:fix] needs landing
Assignee | ||
Comment 13•20 years ago
|
||
(In reply to comment #11)
> (From update of attachment 173903 [details] [diff] [review] [edit])
> ditto what peter said about passing back Clone()'s rv. You could NS_ENSURE aDir
> at the top then just always return the sPluginTempDir clone at the end.
Turns out that was a leftover check from an earlier version where I had one
caller who didn't care about the directory handle, that's since removed, so I
removed the check alltogether.
Assignee | ||
Comment 14•20 years ago
|
||
Assignee | ||
Comment 15•20 years ago
|
||
Assignee | ||
Comment 16•20 years ago
|
||
Fixed on trunk and branches.
Status: NEW → RESOLVED
Closed: 20 years ago
Keywords: fixed-aviary1.0.1,
fixed1.7.6
Resolution: --- → FIXED
Reporter | ||
Comment 17•20 years ago
|
||
can this bug be unrestricted now that it's RESOLVED?
Comment 18•20 years ago
|
||
We'd prefer to wait until we get the 1.0.1 release into people's hands (which
should be next week), but as the bug reporter you can disclose at any time if
you think we're being too slow about it.
Reporter | ||
Comment 19•20 years ago
|
||
Ahh, Understood. Thanks Daniel.
Comment 20•20 years ago
|
||
Hi Tavis,
Does this look fixed for you using a recent firefox 1.0.1 nightly build?
ftp://mozilla.isc.org/pub/mozilla.org/firefox/nightly/latest-aviary1.0.1/
Reporter | ||
Comment 21•20 years ago
|
||
sairuh: yes, I tried the linux build from that directory and it doesnt suffer
from this issue.
Comment 22•20 years ago
|
||
Tavis, thanks for checking! verifying per comment 21 (who also is the original
reporter).
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Group: security
Comment 23•19 years ago
|
||
Updated•19 years ago
|
Attachment #187484 -
Flags: superreview?(dveditz)
Attachment #187484 -
Flags: review?(peterv)
Comment 24•19 years ago
|
||
Comment on attachment 187484 [details] [diff] [review]
1.4 branch patch
r/sr=dveditz (just superreview is OK for a straight branch port with no
complicated merges)
Attachment #187484 -
Flags: superreview?(dveditz)
Attachment #187484 -
Flags: superreview+
Attachment #187484 -
Flags: review?(peterv)
Attachment #187484 -
Flags: review+
Comment 25•19 years ago
|
||
Checking in base/src/nsPluginHostImpl.h;
/cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.h,v <--
nsPluginHostImpl.h
new revision: 1.92.4.2; previous revision: 1.92.4.1
done
Checking in base/src/nsPluginHostImpl.cpp;
/cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v <--
nsPluginHostImpl.cpp
new revision: 1.475.4.5; previous revision: 1.475.4.4
done
Keywords: fixed1.4.5
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•