Closed Bug 212123 Opened 20 years ago Closed 18 years ago

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

Categories

(MailNews Core :: Security, defect)

x86
Linux
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jonny, Assigned: dougt)

References

Details

(Keywords: qawanted, Whiteboard: [sg:fix] - need reviews and landing.)

Attachments

(1 file, 5 obsolete files)

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!).
QA Contact: junruh → esther
passing the buck to mscott.
Assignee: sspitzer → scott
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 :-) )
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Flags: blocking1.7.x?
Flags: blocking-aviary1.0?
Whiteboard: [sg:fix]
Flags: blocking-aviary1.0?
restoring nomination.
Flags: blocking-aviary1.0?
Product: MailNews → Core
This won't make the 1.0 Tbird train.
Flags: blocking-aviary1.0? → blocking-aviary1.0-
1.7.5 has shipped. Moving request to 1.7.6.
Flags: blocking1.7.5? → blocking1.7.6?
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1? → blocking-aviary1.1+
Flags: blocking-aviary1.0.1?
Assignee: mscott → roc
Flags: blocking1.7.6?
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1?
Flags: blocking-aviary1.0.1+
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #173230 - Flags: superreview?(darin)
Attachment #173230 - Flags: review?(darin)
Whiteboard: [sg:fix] → [sg:fix] have patch - need review darin
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.
Attachment #173230 - Flags: superreview?(darin)
Attachment #173230 - Flags: superreview+
Attachment #173230 - Flags: review?(darin)
Attachment #173230 - Flags: review+
Comment on attachment 173230 [details] [diff] [review]
fix

a=dveditz for branch checkins. (but I'd still like the mode 0600)
Attachment #173230 - Flags: approval1.7.6+
Attachment #173230 - Flags: approval-aviary1.0.1+
> 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.
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1+
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?
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1+
Whiteboard: [sg:fix] have patch - need review darin → [sg: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.
Attachment #173230 - Flags: approval1.7.6-
Attachment #173230 - Flags: approval1.7.6+
Attachment #173230 - Flags: approval-aviary1.0.1-
Attachment #173230 - Flags: approval-aviary1.0.1+
Whiteboard: [sg:fix] → [sg:fix] thunderbird only
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.
Whiteboard: [sg:fix] thunderbird only → [sg:fix] mail only
Attached patch trunk fix (obsolete) — Splinter Review
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.
Attachment #173230 - Attachment is obsolete: true
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.
Attachment #176626 - Flags: superreview+
Attachment #176626 - Flags: review+
Attachment #176626 - Flags: approval1.7.6?
Attachment #176626 - Flags: approval-aviary1.0.1?
Comment on attachment 176626 [details] [diff] [review]
trunk fix

a=dveditz per drivers mtg
Attachment #176626 - Flags: approval1.7.6?
Attachment #176626 - Flags: approval1.7.6+
Attachment #176626 - Flags: approval-aviary1.0.1?
Attachment #176626 - Flags: approval-aviary1.0.1+
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
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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/
Keywords: fixed1.7.6
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....

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Blocks: 285612
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. 
Depends on: 285628
Blocks: 285628
No longer depends on: 285628
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. ***
Flags: blocking1.7.9+
Flags: blocking-aviary1.0.5+
Whiteboard: [sg:fix] mail only → [sg:fix] mail only have patch
Any progress here?  Anyone else get a chance to poke around?
I bungled this one twice and I'd rather not touch it again.
Assignee: roc → nobody
Status: REOPENED → NEW
Reassigning to mscott to see if we can get some traction here.  If not, this
might miss the 1.0.5 train.
Assignee: nobody → mscott
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.
Flags: blocking1.8b3+
Flags: blocking1.7.9-
Flags: blocking1.7.9+
Flags: blocking1.7.6+
Flags: blocking1.7.10+
Flags: blocking-aviary1.0.6+
Flags: blocking-aviary1.0.5-
Flags: blocking-aviary1.0.5+
Flags: blocking-aviary1.0.1+
Removing "mail only have patch" from status whiteboard since this bug needs more
work.
Whiteboard: [sg:fix] mail only have patch → [sg:fix]
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.
Assignee: mscott → dougt
(In reply to comment #60)
> chrishofmann: mscott says he will kick in a few beers too...

I'm talking good stuff too. 

Attachment #177031 - Attachment is obsolete: true
Attachment #176626 - Attachment is obsolete: true
Attached patch patch v.3 (obsolete) — Splinter Review
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.
Attachment #186950 - Flags: superreview?(darin)
Attachment #186950 - Flags: review?(dveditz)
i posted a trunk build of tb with this patch:

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

Whiteboard: [sg:fix] → [sg:fix] - have a test build that needs shaking
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=+
Attachment #186950 - Flags: review?(dveditz) → review+
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)! ;-)
Flags: blocking1.7.9-
Flags: blocking1.7.9+
Flags: blocking1.7.10+
Flags: blocking-aviary1.0.6+
Flags: blocking-aviary1.0.5-
Flags: blocking-aviary1.0.5+
+'ed based on comments aboved.
Attachment #186950 - Attachment is obsolete: true
Attachment #187148 - Flags: superreview+
Attachment #187148 - Flags: review+
Attachment #186950 - Flags: superreview?(darin)
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.)
Attachment #187148 - Flags: superreview+ → superreview-
Let's try this one instead
Attachment #187148 - Attachment is obsolete: true
Attachment #187276 - Flags: superreview?(dougt)
Attachment #187276 - Flags: review?(timeless)
Whiteboard: [sg:fix] - have a test build that needs shaking → [sg:fix] - need reviews and landing.
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
Attachment #187276 - Flags: review?(timeless) → review+
Comment on attachment 187276 [details] [diff] [review]
more what I meant

why does the asserion have to be removed?
Attachment #187276 - Flags: superreview?(dougt) → superreview+
Changed NS_ASSERTION to NS_WARN_IF_FALSE after chatting w/timeless on irc.

Fixed on trunk and branches.
Status: NEW → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
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. 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
patch was landed on trunk and branch.

we added a new api that allows you to create a unique directory -- a requirment
for mailnews.
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
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.
Flags: blocking-aviary1.0.7?
Flags: blocking-aviary1.0.8? → blocking-aviary1.0.8-
Group: security
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.