Last Comment Bug 281284 - malicious local users can remove mozilla users files (insecure use of /tmp/plugtmp)
: malicious local users can remove mozilla users files (insecure use of /tmp/pl...
Status: VERIFIED FIXED
[sg:fix] needs landing
: fixed-aviary1.0.1, fixed1.4.5, fixed1.7.6
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
Depends on:
Blocks: sg-ff101 sg-tb101 sg-moz176
  Show dependency treegraph
 
Reported: 2005-02-06 10:57 PST by Tavis Ormandy
Modified: 2006-03-12 18:17 PST (History)
7 users (show)
dveditz: blocking1.7.6+
dveditz: blocking‑aviary1.0.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Always create unique plugintmp directories for each browser instance, and only delete what was created. (7.58 KB, patch)
2005-02-08 17:42 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Fix problem identified by peterv. (7.57 KB, patch)
2005-02-09 15:39 PST, Johnny Stenback (:jst, jst@mozilla.com)
peterv: review+
dveditz: superreview+
mozilla: approval‑aviary1.0.1+
mozilla: approval1.7.6+
Details | Diff | Splinter Review
Patch for checkin. (7.27 KB, patch)
2005-02-11 15:35 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Aviary branch version. (7.35 KB, patch)
2005-02-11 16:00 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
1.4 branch patch (6.76 KB, patch)
2005-06-27 23:23 PDT, Nian Liu(n/a in a long time)
dveditz: review+
dveditz: superreview+
Details | Diff | Splinter Review

Description Tavis Ormandy 2005-02-06 10:57:44 PST
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?
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-08 17:42:38 PST
Created attachment 173817 [details] [diff] [review]
Always create unique plugintmp directories for each browser instance, and only delete what was created.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-08 17:44:52 PST
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.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-08 17:45:22 PST
s/should //
Comment 4 Tavis Ormandy 2005-02-09 07:02:22 PST
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 Peter Van der Beken [:peterv] 2005-02-09 08:12:55 PST
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;
>+}
Comment 6 Tavis Ormandy 2005-02-09 12:26:59 PST
Peter: confirmed, making that adjustment fixes it...works perfectly here.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-09 15:19:58 PST
Duh, new patch with that change coming up... Thanks for the testcase, and thanks
for testing!
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-09 15:39:31 PST
Created attachment 173903 [details] [diff] [review]
Fix problem identified by peterv.
Comment 9 Peter Van der Beken [:peterv] 2005-02-09 15:47:40 PST
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.
Comment 10 Daniel Veditz [:dveditz] 2005-02-09 20:27:46 PST
definitely want this on the branches
Comment 11 Daniel Veditz [:dveditz] 2005-02-09 20:38:09 PST
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.
Comment 12 Mike Kaply [:mkaply] 2005-02-10 05:37:29 PST
Comment on attachment 173903 [details] [diff] [review]
Fix problem identified by peterv.

a=mkaply for the branches
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-11 15:32:53 PST
(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.
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-11 15:35:16 PST
Created attachment 174103 [details] [diff] [review]
Patch for checkin.
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-11 16:00:30 PST
Created attachment 174105 [details] [diff] [review]
Aviary branch version.
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-11 16:11:57 PST
Fixed on trunk and branches.
Comment 17 Tavis Ormandy 2005-02-12 01:11:23 PST
can this bug be unrestricted now that it's RESOLVED?
Comment 18 Daniel Veditz [:dveditz] 2005-02-12 03:01:24 PST
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.
Comment 19 Tavis Ormandy 2005-02-12 03:15:50 PST
Ahh, Understood. Thanks Daniel.
Comment 20 sairuh (rarely reading bugmail) 2005-02-18 10:52:00 PST
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/
Comment 21 Tavis Ormandy 2005-02-18 11:24:14 PST
sairuh: yes, I tried the linux build from that directory and it doesnt suffer 
from this issue.

Comment 22 sairuh (rarely reading bugmail) 2005-02-18 15:57:37 PST
Tavis, thanks for checking! verifying per comment 21 (who also is the original
reporter).
Comment 23 Nian Liu(n/a in a long time) 2005-06-27 23:23:20 PDT
Created attachment 187484 [details] [diff] [review]
1.4 branch patch
Comment 24 Daniel Veditz [:dveditz] 2005-06-28 15:48:03 PDT
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)
Comment 25 Ginn Chen 2005-06-29 22:52:46 PDT
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

Note You need to log in before you can comment on or make changes to this bug.