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!).

passing the buck to mscott.

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

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

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.

anyone thought about a patch for this one yet?

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?

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

same as bug 58580 maybe?

restoring nomination.

This won't make the 1.0 Tbird train.

1.7.5 has shipped. Moving request to 1.7.6.

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.

roc: what about nsLocalFile::CreateUnique ??

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

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

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.

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

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 on attachment 173230 [details] [diff] [review] fix r+sr=darin roc: thanks for bearing with me on that question.

Comment on attachment 173230 [details] [diff] [review] fix a=dveditz for branch checkins. (but I'd still like the mode 0600)

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

checked in on branches. leaving open for trunk checkin. Didn't change the mode, sorry, didn't see these latest comments

okay, changed masks to 0600 on branches.

clearing blocking flags now that this is checked in.

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

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

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)

ok, sorry: just have seen the checkin from 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?

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

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 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 on attachment 173230 [details] [diff] [review] fix Changing approvals to minus for backed-out patch so it doesn't mess up our queries.

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.

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.

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

Is this patch ready for reviews?

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 on attachment 176626 [details] [diff] [review] trunk fix a=dveditz per drivers mtg

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.

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

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

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

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/

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.

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.

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

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

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.

(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.

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.

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

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

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

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

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

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.

Flipping flags to match Jay's statement.

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

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.

(In reply to comment #60) > chrishofmann: mscott says he will kick in a few beers too... I'm talking good stuff too.

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.

i posted a trunk build of tb with this patch: http://www.meer.net/~dougt/tb-212123.zip

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=?

I can't make you or this code happy. if CreateUnique fails, I will null the path of |*this|.

Comment on attachment 186950 [details] [diff] [review] patch v.3 dan is sitting right next to me and gave me a r=+

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.

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

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)! ;-)

+'ed based on comments aboved.

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

Let's try this one instead

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 on attachment 187276 [details] [diff] [review] more what I meant why does the asserion have to be removed?

Changed NS_ASSERTION to NS_WARN_IF_FALSE after chatting w/timeless on irc. Fixed on trunk and branches.

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.

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.

patch was landed on trunk and branch. we added a new api that allows you to create a unique directory -- a requirment for mailnews.

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

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

Adding distributors

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

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.