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

VERIFIED FIXED

Status

()

Core
Plug-ins
--
critical
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Tavis Ormandy, Assigned: jst)

Tracking

({fixed-aviary1.0.1, fixed1.4.5, fixed1.7.6})

Trunk
x86
Linux
fixed-aviary1.0.1, fixed1.4.5, fixed1.7.6
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.6 +
blocking-aviary1.0.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix] needs landing)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

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

12 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)
Blocks: 278184, 278185, 278186
Component: Security: General → Plug-ins
Flags: blocking1.7.6?
Whiteboard: [sg:fix]
(Assignee)

Comment 1

12 years ago
Created attachment 173817 [details] [diff] [review]
Always create unique plugintmp directories for each browser instance, and only delete what was created.
Attachment #173817 - Flags: superreview?(brendan)
Attachment #173817 - Flags: review?(dveditz)
(Assignee)

Comment 2

12 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

12 years ago
s/should //
(Assignee)

Updated

12 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

12 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

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

12 years ago
Peter: confirmed, making that adjustment fixes it...works perfectly here.
(Assignee)

Comment 7

12 years ago
Duh, new patch with that change coming up... Thanks for the testcase, and thanks
for testing!
(Assignee)

Updated

12 years ago
Attachment #173817 - Attachment is obsolete: true
Attachment #173817 - Flags: superreview?(brendan)
Attachment #173817 - Flags: review?(dveditz)
(Assignee)

Comment 8

12 years ago
Created attachment 173903 [details] [diff] [review]
Fix problem identified by peterv.
Attachment #173903 - Flags: superreview?(dveditz)
Attachment #173903 - Flags: review?(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.
Attachment #173903 - Flags: review?(peterv) → review+
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 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

12 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+
Whiteboard: [sg:fix] → [sg:fix] needs landing
(Assignee)

Comment 13

12 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

12 years ago
Created attachment 174103 [details] [diff] [review]
Patch for checkin.
(Assignee)

Comment 15

12 years ago
Created attachment 174105 [details] [diff] [review]
Aviary branch version.
(Assignee)

Comment 16

12 years ago
Fixed on trunk and branches.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed-aviary1.0.1, fixed1.7.6
Resolution: --- → FIXED
(Reporter)

Comment 17

12 years ago
can this bug be unrestricted now that it's RESOLVED?
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

12 years ago
Ahh, Understood. Thanks Daniel.
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

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

Tavis, thanks for checking! verifying per comment 21 (who also is the original
reporter).
Status: RESOLVED → VERIFIED
Group: security
Created attachment 187484 [details] [diff] [review]
1.4 branch patch
Attachment #187484 - Flags: superreview?(dveditz)
Attachment #187484 - Flags: review?(peterv)
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

12 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
You need to log in before you can comment on or make changes to this bug.