212123 - Insecure file names/opening in /tmp/ when sending mail

Status: RESOLVED FIXED

(MailNews Core :: Security, defect)

Description

[Jonny Robertson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030508
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030508

Files owned by the user running mozilla mail can be clobbered, or worst case
scenario (if mozilla was started by root), root access is achievable via /tmp
race condition.

Netscape 4.x doesn't exhibit this problem (uses semi-random filenames in /tmp).


Reproducible: Always

Steps to Reproduce:
1.  
Open Mozilla mail as jonny.
Check no evil file is in my home directory:
[jonny@pichu jonny]$ ls -l /home/jonny/.badfile
ls: /home/jonny/.badfile: No such file or directory

2. 
Another user is evil:
[guest@pichu guest]$ ln -s /home/jonny/.badfile /tmp/nsmail.tmp

3.
Jonny sends an email to someone using Mozilla mail.

4.
[jonny@pichu jonny]$ ls -l /home/jonny/.badfile
-rw-------    1 jonny    jonny         239 Jul  9 14:36 /home/jonny/.badfile

:((
Actual Results:  
[jonny@pichu jonny]$ ls -l /home/jonny/.badfile
-rw-------    1 jonny    jonny         239 Jul  9 14:36 /home/jonny/.badfile

Expected Results:  
- use random /tmp file names
- use O_NOFOLLOW or O_EXCL on /tmp file opening to prevent this sort of trickery

Confirmed obtaining root priveledges via Michael Zalewski's pam_timestamp_check
bug using this race condition (if mozilla mail was being run as root!).

Comment 1

[(not reading, please use [e-mail] instead)]

passing the buck to mscott.

Comment 2

[David :Bienvenu]

confirming - I suppose the fix is to either use semi-random file names, or to
use exclusive open (oops, I guess the reporter said that already :-) )

Comment 3

[Brendan Eich [:brendan]]

Argh, nsFileSpec.  If you can use nsIFile, you get CreateUnique.  But it does
not return an open file descriptor (er, an open PRFileDesc).  So it's important
that the file have user-write access only (not group or other), or some
attacking code could unlink it and race to claim the name.

Cc'ing dougt and darin on the lack of a create-unique-and-return-open-filedesc
method.

/be

Comment 4

[Darin Fisher]

yeah, it's a known problem / limitation of our nsIFile impl.  we should really
be using mkstemp (or equivalent library function) when available to avoid this
kind of problem.

Comment 5

[chris hofmann]

anyone thought about a patch for this one yet?

Comment 6

[Doug Turner (:dougt)]

From comment #2, we can easily bandaide this problem by having a /tmp seeded
directory that has only the owner's rwx bits set, right?

Comment 7

[Daniel Veditz [:dveditz]]

Dropped through the cracks... this should really be in Thunderbird 1.0 final, if
only the band-aide from comment 6

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

same as bug 58580 maybe?

Comment 9

[Asa Dotzler [:asa]]

restoring nomination.

Comment 10

[Marcia Knous [:marcia]]

This won't make the 1.0 Tbird train.

Comment 11

[Asa Dotzler [:asa]]

1.7.5 has shipped. Moving request to 1.7.6.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This patch fixes a lot of issues by making nsFileSpec::MakeUnique actually
create the file in exclusive mode, guaranteeing that it's a new file that the
attacker will not be able to modify (assuming reasonable umask).

This is an API change that could cause problems if anyone calls MakeUnique and
then tries to open the file in exclusive mode, but I can't detect anyone doing
that in our code. Even if there are problems, they'll be easy to fix and it's
better than the insecurities that plague every single user of this function...

If this patch is good, it'd be good to land this ASAP so we can get some
testing before branch landing.

Comment 13

[Darin Fisher]

roc: what about nsLocalFile::CreateUnique ??

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I haven't touched it. What about it? You want me to call it somehow?

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 173230 [details] [diff] [review]
fix

Do you really want to create these "0666"? wouldn't "0600" prevent a future
"mail /tmp files are world-readable" bug?

Hm, not sure the default CreateDir mode of 0775 is all that great, either. That
one's used in nsDogbertProfileMigrator.cpp and a few other mail places plus
nsPrefMigration.cpp

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

0666 was what was currently being used; nsIFileSpec::GetOutputStream calls
NewTypicalOutputStream:
http://lxr.mozilla.org/mozilla/source/xpcom/obsolete/nsIFileStream.cpp#640

I wasn't 100% sure that changing that would work. Note that the user's umask
should stop world-readability --- at least on properly configured Unix.

It certainly would be easy to use 0600 instead.

Comment 17

[Darin Fisher]

(In reply to comment #13)
> roc: what about nsLocalFile::CreateUnique ??

Sorry, I should have been more clear.  The question I meant to ask was: do we
have to worry about the same problem with CreateUnique.  Do users of it
potentially suffer from the same bug.  (I admit that I haven't studied this bug
carefully yet, so maybe the answer is very obvious -- sorry!)

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsLocalFile::CreateUnique is already safe. It creates the file that it thinks is
unique (with PR_EXCL on Unix) and if the file already exists, it tries another
one. If it returns successfully, the a unique file was created and is owned by
the user.

Comment 19

[Darin Fisher]

Comment on attachment 173230 [details] [diff] [review]
fix

r+sr=darin

roc: thanks for bearing with me on that question.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 173230 [details] [diff] [review]
fix

a=dveditz for branch checkins. (but I'd still like the mode 0600)

Comment 21

[Darin Fisher]

> I wasn't 100% sure that changing that would work. Note that the user's umask
> should stop world-readability --- at least on properly configured Unix.
> 
> It certainly would be easy to use 0600 instead.

We might want to switch to 0600.  It appears that NSPR honors these permission
bits on NTFS (see _PR_NT_MakeSecurityDescriptorACL and its uses in
_PR_MD_OPEN_FILE and _PR_MD_MAKE_DIR).  Also, people do misconfigure their
umask, and might not realize there is a problem if their home directory has
restricted access.  We try to create all temp files with 0600.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in on branches. leaving open for trunk checkin.

Didn't change the mode, sorry, didn't see these latest comments

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

okay, changed masks to 0600 on branches.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

clearing blocking flags now that this is checked in.

Comment 25

[Wan-Teh Chang]

The mode argument is ignored by PR_Open on Windows.
It is recognized by PR_OpenFile on Windows.

Comment 26

[Darin Fisher]

hmm.. yes.. i wonder if nsLocalFile::OpenNSPRFileDesc should be using
PR_OpenFile instead of PR_Open.

Comment 27

[Wolfgang Rosenauer [:wolfiR]]

according to tinderboxes this patch is incomplete:
nsFileSpec.cpp: In member function `void nsFileSpec::MakeUnique()':
nsFileSpec.cpp:914: error: `NS_NewIOFileStream' undeclared (first use this function)

Comment 28

[Wolfgang Rosenauer [:wolfiR]]

ok, sorry: just have seen the checkin from dveditz

Comment 29

[Daniel Veditz [:dveditz]]

You don't clear the blocking flags, you add the "fixed" keywords (otherwise how
does QA know which bugs to test?)

This busted the tree due to a missing include and an incompatible type. I
changed enough to make the compiler happy but didn't test. Given the changes I
had to make, did *you* test the fix?

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah. I'm not sure what happened here. Perhaps I attached an old version of the
patch by mistake. Very sorry...

Comment 31

[Chase Phillips]

Comment on attachment 173230 [details] [diff] [review]
fix

Backed out on the aviary 1.0.1 branch:

Checking in xpcom/obsolete/nsFileSpec.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpec.cpp,v  <--  nsFileSpec.cpp
new revision: 1.5.36.1.2.4; previous revision: 1.5.36.1.2.3
done
Checking in xpcom/obsolete/nsFileSpec.h;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpec.h,v	<--  nsFileSpec.h
new revision: 1.6.16.2; previous revision: 1.6.16.1
done

Comment 32

[Chase Phillips]

Comment on attachment 173230 [details] [diff] [review]
fix

Also backed out on Mozilla 1.7 branch:

Checking in xpcom/obsolete/nsFileSpec.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpec.cpp,v  <--  nsFileSpec.cpp
new revision: 1.5.32.5; previous revision: 1.5.32.4
done
Checking in xpcom/obsolete/nsFileSpec.h;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpec.h,v	<--  nsFileSpec.h
new revision: 1.6.2.2; previous revision: 1.6.2.1
done

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 173230 [details] [diff] [review]
fix

Changing approvals to minus for backed-out patch so it doesn't mess up our
queries.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

oooh. I know what I did wrong. (cd xpcom;make) doesn't actually remake
xpcom/obsolete so my build and test did not actually remake the changed code.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: trunk fix

here's the corrected fix against trunk. It builds, it runs, it appears to
compse mail and save drafts correctly, but trunk Thunderbird is hopeless broken
so I can't really be sure.

Comment 36

[David :Bienvenu]

out of curiousity, how is tbird broken on the trunk? I use it all the time and
have since 1.0 shipped. There were packager problems for a long time, but those
only affected release builds. I can try this patch on the trunk since I use my
trunk build exclusively...

Comment 37

[Christopher Aillon (sabbatical, not receiving bugmail)]

Is this patch ready for reviews?

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 176626 [details] [diff] [review]
trunk fix

r/sr=dveditz (and carrying over darin's). This fixes the tree bustage but is
otherwise identical to the originally reviewed and approved patch.

Comment 39

[Daniel Veditz [:dveditz]]

Comment on attachment 176626 [details] [diff] [review]
trunk fix

a=dveditz per drivers mtg

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

David: well, it could be my config/build, but I had no "Quit" item in the File
menu among other issues.

I'll land this soon.

Comment 41

[sairuh (rarely reading bugmail)]

what would be the best way to test this from an enduser perspective?

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked into trunk, AVIARY_1_0_1_20050124_BRANCH and MOZILLA_1_7_BRANCH. hope it
sticks this time

Comment 43

[Wolfgang Rosenauer [:wolfiR]]

isn't it fixed for aviary1.0.2 as Firefox 1.0.1 is already released?

Comment 44

[sairuh (rarely reading bugmail)]

Jonny, how does this behave for you using this morning's build? is it fixed for you?

ftp://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-aviary1.0.1/

Comment 45

[Scott MacGregor]

I'm seeing some weird things after pulling with this patch. In particular, when
I delete a mail folder now, I now get several hundred messages getting dumped to
the console saying: 

Openign file tmprules-XXX.dat failed

where XXX starts at 001 and goes up to 999 before it bails out.

I started seeing this right after I pulled this change to CreateUnique which is
called here:

http://lxr.mozilla.org/seamonkey/source/mailnews/base/search/src/nsMsgFilterService.cpp#154

I suspect we're going to want to back this out of the 1.0.1 build and respin
before the release.

Comment 46

[Scott MacGregor]

This part of the patch:

        rv = NS_NewIOFileStream(getter_AddRefs(file), *this, 
                                PR_WRONLY | PR_CREATE_FILE | PR_EXCL |
PR_TRUNCATE, 0600);
        if (NS_SUCCEEDED(rv))
            break;

Always returns an error because the file doesn't exist yet. As a result, we
never fall out of the loop and end up trying to create 1,000 files every time
you call CreateUnique before eventually giving up. I'm pretty sure that's not
what Roc intended with his fix. 

Comment 47

[Scott MacGregor]

by the way, this is failing because we are passing PR_WRONLY into
NS_NewIOFileStream. NS_NewIOFileStream is intended to be used to open readable
and writeable streams. As a result, it returns an error if it trys to open a
stream with the passed in persmissions and it can't open the stream for reading.
I'm not sure how this patch could have worked....

Comment 48

[Daniel Veditz [:dveditz]]

Could this cause the "blank forwarded attachment" regression bug 285612?

Comment 49

[Scott MacGregor]

Attachment: additional change to try to make this patch work

This patch removes the check in NS_NewIOFileStream that returned an error code
if the stream was not open. 

We now no longer end up trying to create 1,000 files every time we call
CreateUnique. However, I still get an error message from deep within NSPR
saying "Opening file tmprules.dat failed"

So things still aren't right yet. I suspect my patch is just masking a deeper
problem.

Comment 50

[Tracy Walker [:tracy]]

(In reply to comment #48)
> Could this cause the "blank forwarded attachment" regression bug 285612?

seems so, the nightly build from the 8th works fine.

Comment 51

[Scott MacGregor]

David just pointed out another issue with this message. message replies are
getting corrupted too. I'm sending out messages that look like:

This is a multi-part message in MIME format.
--------------060901090707080001020407
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

I didn't back anything out yet.

-Scott

-----------

Obviously, that's not my vcard, that's part of the actual message text. 

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

gah. I'll do the backout and someone who knows what they're doing should take over.

Comment 53

[Scott MacGregor]

*** Bug 285415 has been marked as a duplicate of this bug. ***

Comment 54

[Jay Patel [:jay]]

Any progress here?  Anyone else get a chance to poke around?

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I bungled this one twice and I'd rather not touch it again.

Comment 56

[Jay Patel [:jay]]

Reassigning to mscott to see if we can get some traction here.  If not, this
might miss the 1.0.5 train.

Comment 57

[Jay Patel [:jay]]

This is going to miss the 1.0.5 train, but let's try to get this fixed on the
Trunk and hope for a fix for a future 1.0.x release.

Comment 58

[Daniel Veditz [:dveditz]]

Flipping flags to match Jay's statement.

Comment 59

[Jay Patel [:jay]]

Removing "mail only have patch" from status whiteboard since this bug needs more
work.

Comment 60

[chris hofmann]

chrishofmann: I need to find one of the braves mozilla hackers I know for a very
special bug....
chrishofmann: I'm looking at you ;-)
chrishofmann: https://bugzilla.mozilla.org/show_bug.cgi?id=212123
DougT   10: /me looks
DougT   10: /me finds a bottle of jack in his desk he saves for bugs like this.
chrishofmann: mscott says he will kick in a few beers too...
DougT   10: heh
DougT   10: it has moved to the top of my stack.  I will look at it now.

Comment 61

[Scott MacGregor]

(In reply to comment #60)
> chrishofmann: mscott says he will kick in a few beers too...

I'm talking good stuff too. 

Comment 62

[Doug Turner (:dougt)]

Attachment: patch v.3

makes nsFileSpec::MakeUnique use nsIFile::CreateUnique.

I tried most of the regressions cited above and could not reproduce with this
patch.	I would like broader testing before landing this based on the history
of this bug.

I am wondering why this approach was not considered;  is there something I am
missing?

Also, MakeUnique() doesn't return any error code.  that seams very broken.

Comment 63

[Doug Turner (:dougt)]

i posted a trunk build of tb with this patch:

http://www.meer.net/~dougt/tb-212123.zip

Comment 64

[Daniel Veditz [:dveditz]]

Comment on attachment 186950 [details] [diff] [review]
patch v.3

>+    nsCOMPtr<nsIFile> file = do_QueryInterface(localFile);
>+    file->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
>+
>+    nsCAutoString path;
>+    file->GetNativePath(path);

As unlikely as that is, if CreateUnique() fails this path now contains the
original (presumably unsafe) name, right?  You could null out the name to
signal the error, but then all the mail code is likely to fail very quickly :-)

Can you make me happy here so I can give you r=?

Comment 65

[Doug Turner (:dougt)]

I can't make you or this code happy.  

if CreateUnique fails, I will null the path of |*this|.

Comment 66

[Doug Turner (:dougt)]

Comment on attachment 186950 [details] [diff] [review]
patch v.3

dan is sitting right next to me and gave me a r=+

Comment 67

[timeless]

Comment on attachment 186950 [details] [diff] [review]
patch v.3

>+    NS_NewNativeLocalFile(nsDependentCString(*this), PR_TRUE, getter_AddRefs(localFile));
>+    nsCOMPtr<nsIFile> file = do_QueryInterface(localFile);
>+    file->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);

r=me if you make this code *not* crash when NS_NewNativeLocalFile fails.

Comment 68

[Daniel Veditz [:dveditz]]

sr=dveditz if you fix timeless's concerns based on the patch you showed me
yesterday (I did not r= attachment 186950 [details] [diff] [review])

Comment 69

[Jay Patel [:jay]]

Doug:  Let's get an updated patch submitted here with dveditz's changes and get
this checked in on the branches and the trunk. a=jay Also, plesae let us know
what kind of beer you want (see comment #60)! ;-)

Comment 70

[Doug Turner (:dougt)]

Attachment: patch w/ dveditz & timeless comments included.

+'ed based on comments aboved.

Comment 71

[Daniel Veditz [:dveditz]]

Comment on attachment 187148 [details] [diff] [review]
patch w/ dveditz & timeless comments included.

Maybe I'm confused by the interleaved +/- lines, but this doesn't really
fix timeless's complaint, does it? Even if you detect that localFile is
null you still QI to nsIFile and deref it.

And it doesn't really resolve my complaint because you null out path
too early -- right before you set it back to the original filename should
CreateUnique fail.

If nsILocalFile subclasses nsIFile you should be able to call
CreateUnique on it without having the QI back to nsIFile. (why
is CreateUnique part of nsIFile anyway? that seems like a very specific
local-file kind of thing.)

Comment 72

[Daniel Veditz [:dveditz]]

Attachment: more what I meant

Let's try this one instead

Comment 73

[timeless]

Comment on attachment 187276 [details] [diff] [review]
more what I meant

you have to remove this assertion:
>+    NS_ASSERTION(!path.IsEmpty(), "MakeUnique() failed!");
>+    *this = path.get(); // reset the filepath to point to the unique location

Comment 74

[Doug Turner (:dougt)]

Comment on attachment 187276 [details] [diff] [review]
more what I meant

why does the asserion have to be removed?

Comment 75

[Daniel Veditz [:dveditz]]

Changed NS_ASSERTION to NS_WARN_IF_FALSE after chatting w/timeless on irc.

Fixed on trunk and branches.

Comment 76

[Jay Patel [:jay]]

This might have caused a regression in Thunderbird where you are unable to
create a proper POP account (see bug 299133).  If that is the case, we need to
reopen this bug.

Comment 77

[Doug Turner (:dougt)]

it did cause a regression.

the mailnews code calls makeunique on a directory.  the prior behavior was that
this was simply a string operation.  After my change, we create a unique file. 
Subsequent operations fail.  David and I our working on a solution. 

Comment 78

[Doug Turner (:dougt)]

patch was landed on trunk and branch.

we added a new api that allows you to create a unique directory -- a requirment
for mailnews.

Comment 79

[Doug Turner (:dougt)]

Backing this out of MOZILLA_1_7_BRANCH.  It broken some mailnews extensions due
to the API change:

Checking in xpcom/obsolete/nsFileSpec.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpec.cpp,v  <--  nsFileSpec.cpp
new revision: 1.5.32.11; previous revision: 1.5.32.10
done
Checking in xpcom/obsolete/nsFileSpec.h;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpec.h,v  <--  nsFileSpec.h
new revision: 1.6.2.7; previous revision: 1.6.2.6
done
Checking in xpcom/obsolete/nsFileSpecImpl.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpecImpl.cpp,v  <--  nsFileSpecImpl.cpp
new revision: 1.5.4.2; previous revision: 1.5.4.1
done
Checking in xpcom/obsolete/nsIFileSpec.idl;
/cvsroot/mozilla/xpcom/obsolete/nsIFileSpec.idl,v  <--  nsIFileSpec.idl
new revision: 1.5.2.2; previous revision: 1.5.2.1
done
Checking in mailnews/base/util/nsMsgIncomingServer.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgIncomingServer.cpp,v  <-- 
nsMsgIncomingServer.cpp
new revision: 1.201.2.2; previous revision: 1.201.2.1
done


AVAIRY branch backout coming up

Comment 80

[Doug Turner (:dougt)]

Checking in xpcom/obsolete/nsFileSpec.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpec.cpp,v  <--  nsFileSpec.cpp
new revision: 1.5.36.1.2.10; previous revision: 1.5.36.1.2.9
done
Checking in xpcom/obsolete/nsFileSpec.h;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpec.h,v  <--  nsFileSpec.h
new revision: 1.6.16.7; previous revision: 1.6.16.6
done
Checking in xpcom/obsolete/nsFileSpecImpl.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpecImpl.cpp,v  <--  nsFileSpecImpl.cpp
new revision: 1.5.18.2; previous revision: 1.5.18.1
done
Checking in xpcom/obsolete/nsIFileSpec.idl;
/cvsroot/mozilla/xpcom/obsolete/nsIFileSpec.idl,v  <--  nsIFileSpec.idl
new revision: 1.5.16.2; previous revision: 1.5.16.1
done
Checking in mailnews/base/util/nsMsgIncomingServer.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgIncomingServer.cpp,v  <-- 
nsMsgIncomingServer.cpp
new revision: 1.201.6.9.2.2; previous revision: 1.201.6.9.2.1
done

Comment 81

[Daniel Veditz [:dveditz]]

Adding distributors

Comment 82

[Darin Fisher]

dougt: was that backout for this bug or for bug 299133?

Comment 83

[Daniel Veditz [:dveditz]]

No longer fixed on branches, removing keywords.

(In reply to comment #82)
> dougt: was that backout for this bug or for bug 299133?

Both. 299133 was the one causing the Engimail problems by changing the
interfaces, but it was a regression fix from this checkin and we have to revert
the whole thing.