281284 - malicious local users can remove mozilla users files (insecure use of /tmp/plugtmp)

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Tavis Ormandy]

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)]

Attachment: Always create unique plugintmp directories for each browser instance, and only delete what was created.

Comment 2

[Johnny Stenback (:jst)]

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)]

s/should //

Comment 4

[Tavis Ormandy]

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]]

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]

Peter: confirmed, making that adjustment fixes it...works perfectly here.

Comment 7

[Johnny Stenback (:jst)]

Duh, new patch with that change coming up... Thanks for the testcase, and thanks
for testing!

Comment 8

[Johnny Stenback (:jst)]

Attachment: Fix problem identified by peterv.

Comment 9

[Peter Van der Beken [:peterv]]

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]]

definitely want this on the branches

Comment 11

[Daniel Veditz [:dveditz]]

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]]

Comment on attachment 173903 [details] [diff] [review]
Fix problem identified by peterv.

a=mkaply for the branches

Comment 13

[Johnny Stenback (:jst)]

(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)]

Attachment: Patch for checkin.

Comment 15

[Johnny Stenback (:jst)]

Attachment: Aviary branch version.

Comment 16

[Johnny Stenback (:jst)]

Fixed on trunk and branches.

Comment 17

[Tavis Ormandy]

can this bug be unrestricted now that it's RESOLVED?

Comment 18

[Daniel Veditz [:dveditz]]

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]

Ahh, Understood. Thanks Daniel.

Comment 20

[sairuh (rarely reading bugmail)]

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]

sairuh: yes, I tried the linux build from that directory and it doesnt suffer 
from this issue.

Comment 22

[sairuh (rarely reading bugmail)]

Tavis, thanks for checking! verifying per comment 21 (who also is the original
reporter).

Comment 23

[Nian Liu(n/a in a long time)]

Attachment: 1.4 branch patch

Comment 24

[Daniel Veditz [:dveditz]]

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]

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