Closed Bug 58580 Opened 24 years ago Closed 12 years ago

Temp files from sending drafts or posting news are (created with bad permissions and) left behind

Categories

(MailNews Core :: Backend, defect, P3)

Product:
MailNews Core
Component:
Backend
Platform:
x86
Linux
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED DUPLICATE of bug 235432
Milestone:
Future

People

(Reporter: sspitzer, Unassigned)

References

Details

(Keywords: privacy)

Attachments

(3 files)

fix, use 600 permissions when creating tmp files
10.52 KB, patch
Details | Diff | Splinter Review
diff -u version of the fix, mscott can you review?
10.52 KB, patch
Details | Diff | Splinter Review
new patch. please re-review
10.58 KB, patch
Details | Diff | Splinter Review 
looking at my /tmp, I see this:

-rw-rw-r--   1 seth     seth          944 Oct 29 00:34 /tmp/nscopy-1.tmp
-rw-rw-r--   1 seth     seth         3315 Oct 29 10:28 /tmp/nscopy-2.tmp
-rw-rw-r--   1 seth     seth         1673 Oct 29 00:32 /tmp/nscopy.tmp
-rw-------   1 seth     seth          856 Oct 29 00:34 /tmp/nsmail-1.eml
-rw-rw-r--   1 seth     seth          930 Oct 29 00:32 /tmp/nsmail-1.html
-rw-rw-r--   1 seth     seth          295 Oct 29 00:34 /tmp/nsmail-2-1.html
-rw-------   1 seth     seth         3313 Oct 29 10:28 /tmp/nsmail-2.eml
-rw-------   1 seth     seth          468 Oct 29 00:34 /tmp/nsmail-2.html
-rw-rw-r--   1 seth     seth         2536 Oct 29 10:28 /tmp/nsmail-3-1.html
-rw-------   1 seth     seth         2974 Oct 29 10:28 /tmp/nsmail-3.html
-rw-------   1 seth     seth         1585 Oct 29 00:32 /tmp/nsmail.eml
-rw-------   1 seth     seth         1195 Oct 29 00:32 /tmp/nsmail.html

Two things:

1) the temp files are left behind and we'll fill up /tmp.  I've talk to rhp
about this, and in addition to finding the problems and fixing them, we might
want to add some code to shutdown to remove temp files.

2) the permissions are bar.  we should make sure they are 600.  other users can
read my sent messages!
Egads, that smells like a /tmp race condition, which is a serious bad boy
security problem on unix systems with untrusted users.  Is Mozilla creating
files with serial names?

Looking through the code, the answer is "yes".  The action of checking for
non-existence of a filename and actually creating the file does not appear to be
atomic.  In particular, nsLocalFile::Exists checks for the existence of a file
with access(2), but the file isn't actually created until sometime later (in
nsOutputFileStream?).

In either case, it's a huge and obvious security hole.
> Is Mozilla creating files with serial names?
> In either case, it's a huge and obvious security hole.

We should use random names, i.e. "salt" them (compare bug 56002).

Even if it were no a security problem, it is annoying. Disks are slow and loud.
Some computers even turn off the hd after some time - accessing the hd will
reset it. We leave clutter behind.

Can't we get rid of those temp files completely? Maybe stuff them in mem cache,
or completely avoid them?
This is bug 49943
Well, the code is there to make EVERY temp file we create a 600 permission. 
Again, I would love to find the actual times that this does happen. Its not a 
default behavior...we clean up behind ourselves for most normal conditions, but 
crashing and failures could be an issue.

- rhp
Status: NEW → ASSIGNED
Target Milestone: --- → Future
At least the permissions problem should be investigated and fixed, if
problematic, for mozilla0.9.

My /tmp/ looks similar to sspitzer's. I don't know, which files are from 4.x and
which from Mozilla. IIRC, the following is from Mozilla:
-rw-rw-r--   1 ben      ben          1375 Oct 31 00:10 [Bug 56135] Changed -
Text copied from the Subject field is pasted into the body as a <pre>
Please note that Messenger 4.x/Mozilla is practically the only app which leaves
clutter in /tmp.
Keywords: mozilla0.9
Ben,
Every file I create in the compose back end is created with that permission 
mask. What I am saying is that with normal operations, these files are not left 
around, but you are a developer who I am sure crashes or kills processes in the 
debugger before total cleanup is completed.

Also, if you set TMPDIR in your environment, you can point the temp directory 
whereever you want.

- rhp
This problem with temp files is also a big deal on Mac. In Mac OS 9, there is a 
bug where the contents of 'Temporary Items' (the temp files folder) are not 
deleted each time you restart the machine, so copies of sent messages will just 
accumulate there.

This seems like a serious security risk to me; someone could come along later, 
and read all your sent messages.

But on a Mac if someone has access to your machine, can't they just go right 
into your sent folder and view your messages anyway?

- rhp
Not if I keep sent message on the server, or don't keep copies of sent messages 
at all. Some users will be paranoid enough to do this in a shared-machine 
setting. And it's easy to write an AppleScript to get at these files in the temp 
directory.

Is there agreement that this is limited to crashes and other such things that
prevent the cleanup from happening?

I've had this bug before. If you start CLEAN by going into your tmp directory 
and wiping out files in there, then run Seamonkey and send a message, you 
should be fine with no leftover files. Crashes, quitting in the debugger, 
etc... will cause problems where stuff doesn't get cleaned up.

If this is not the case, then it is new because I've fixed this bug in the 
past.

- rhp
This is a problem on Windows 2000 as well. I don't recall crashing while 
mailnews was open ... ever. But, I may be mistaken (I don't think so). You are 
correct that this does not happen everytime, so I will watch to see if I can 
find what does it.
Please note that with our current stability, crashing is not en exception ;-).
(BTW: I don't use a debugger, and have ~20 such temp files in tmp.)

sfraser, it's not only MacOS9 that doesn't clean up temp itself - many Linux
(/Unix?) distributions don't either. Debian 2.1 does, Redhat 6.0 doesn't.

Rich, I don't say it's your bug. But *anything* *does* seem to leave files with
bad permissions behind.

Does anybody want to test it? If on Unix, you could use this script to watch /tmp:

while ls -la /tmp;
do
sleep 3
clear;
rather

while ls -la /tmp;
do
sleep 1
clear;
done

I know one way to reproduce this.

postings a news message seems to leave tmp files behind.




Good! But that can't be the only source, because I also have them and can't use
news in Mozilla at all.
Is there some way that you can ensure the temp files are deleted (or at least 
badly corrupted) when Mozilla crashes? Like, I don't know, always having them 
open for write access except in the specific times you need to read them, or 
something?
Keywords: privacy
Hi All
I am getting these *.eml files left in temp on Win 95 B using
Build ID 2000091312 for every email, that's of course means all
entries.
None of these are from NS 4.75, haven't used it for weeks.
The above mentioned build is my daily browser and mail program.
These are being left there with no crashes of the mail program.
News program works fine and have seven news servers with many newsgroups.

From looking at the msg instances which are left behind for me, it might be
related to SaveAsDraft.
I just test the Branch Win32 bits and it is cleaning up properly.

- rhp
I was on linux.  I've seen the news posting problem on winnt, too.

we should be able to fix the file permissions for rtm, right?
Keywords: rtm
Whiteboard: [rtm need info]
Well, the problem is that I already "fixed" the permission problem. Let me 
rescan the code but I attempted to have every file I create in the compose back 
end be 600.

I'll look again.

- rhp
updating summary.  rhp and I have a fix.

we looked for all cases where we create an nsOutputFile stream and fixed them to
create the file with 600.

I'll attach the patch.

this will not fix the problem where we leave temp files behind, but that is
another bug, which I will go log on rhp.
Summary: temp files left behind with bad permissions → temp files from sending drafts or posting news are create with bad permissions
Seth and I worked on a permissions fix that is in his tree right now.

- rhp
Assignee: rhp → sspitzer
Status: ASSIGNED → NEW
Yes, this is a good fix!

r: rhp
looking for sr=.  mscott?
Status: NEW → ASSIGNED
Whiteboard: [rtm need info] → [rtm need info] r=rhp, sr=?
can you please attach diffs using
cvs diff -u

That way I can actually tell what lines were added and changed. I can't parse
these diffs.

Thanks!
Does PR_WRONLY | PR_CREATE_FILE represent any change from the previous behavior?
I believe the default behavior is:

(PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE)

this is a good point.

Seth, we should probably go with this kind of mask for newly created files.

- rhp

Hmm.  I assumed these were all the same.  Are any of them NOT opening new files
for creation?
I don't think we need to add the PR_TRUNCATE.

since we get unique (non-existing) names for temp files, truncating them is a no-op.

I'll repost the diff for mscott to sr.
True. 

Steve: yes, these are all new temp files being created.

- rhp
sr=mscott
Whiteboard: [rtm need info] r=rhp, sr=? → [rtm need info] r=rhp, sr=mscott
privacy issue, so worth running by the PDT.

feet don't fail me now!
Whiteboard: [rtm need info] r=rhp, sr=mscott → [rtm+] r=rhp, sr=mscott
rtm-, wouldn't a proper umask cover this situation where it matters?
Keywords: relnoteRTM
Whiteboard: [rtm+] r=rhp, sr=mscott → [rtm-] r=rhp, sr=mscott
Not the situation of leaving tmp files behind. Who wants to open their temp dir 
one day to see several hundred files left by Mozilla?
Jerry, the fix doesn't address that issue either.
selmer, yes umask would solve the problem.

jerry, see http://bugzilla.mozilla.org/show_bug.cgi?id=58979

yikes!  that patch is bad.  in nsMsgSend.cpp, we pass a deleted tFileSpec to the
nsOutputFileStream constructor!

lucky for me this didn't make it into rtm.

here's a new patch.  clearing reviews.
Whiteboard: [rtm-] r=rhp, sr=mscott → [rtm-]
This is scary. What on earth should the relnote be? "Wipe /tmp after using 
Mozilla Mail"?

Gerv
Whiteboard: [rtm-] → [rtm-] relnote-user
for a user relnote, you can tell people to set their umask to 077
umask on Win9x?
Win95/98/ME cannot be considered secure in any case.  You might as well assume
that your system is trojaned and anyon can read your mail, temp files or no.
I am not aware of anything in Windows 95/98/Me that saves your outgoing 
newsposts to a file. This bug is adding to the insecurity of Win 9x. Don't be 
fooled into thinking it is not making it worse.
What about WinNT? You can't really claim that it were inherently insecure, but
it has no umask either, does it?

Seth, on Unix, could we set the umask in the ./mozilla start script?
Changing umask is a bad idea. It's set by the user so you need to be careful not
to be less restrictive than the user. Fiddling with it is not a confidence
builder. In any case, it is never necessary to change it. You can always be more
restrictive on unix. You use chmod. Unfortunately, there seems to be no NSPR
equivalent.

Another way to look at this is that umask is a user security setting. Don't play
with it or you'll look bad.
We will look even more bad, if we leave "open" files around. I'm looking for a
good workaround for rtm, that catches as many cases as possible and is as less
risky as possible.

Is there no way to make the umask only more restrictive, e.g. by querying the
pre-existing umask and compute the new one (e.g. use the user bits from the user
preference and 7 for the group and world bits) or similar?
Let me clarify my position. It is *never* necessary to alter umask because umask can
only unset permission bits, not set them. On unix, the low-level open function
uses permission_bits & ~umask. If files are created with relaxed permissions, it's
Mozilla's fault; no one else's.

Furthermore, umask is inherited by child processes so changing it means determining
if such things as PSM will function appropriately. This is also true if a plugin
invokes a child process. It rapidly becomes a security issue. Condsider what happens if I
say:
      (umask 777; ./mozilla)
I expect that Mozilla will fail because it can't create files. If it does create
files, it is not secure.

Now, if you want, you can see if the file permissions are sufficiently restrictive
and use chmod if they are not, but that should also not be necessary.

There are probably other reasons why fiddling with umask is bad that I haven't
thought of. It's just a bad idea.
Missed something. Yes, you can make umask more restrictive
   oldmask = umask (0777);
   umask (oldmask | newbits);
but if you think you need this, you're wrong.
> it's Mozilla's fault; no one else's.

Nobody objected that. But we have no time to fix Mozilla for rtm.

> It's just a bad idea.

OK, what do you propose to do for RTM? Leave the files world-readable?

There's no time not to fix it. Quick and dirty hacks never work, though.

Having said that, why not create the temp files in the user's profile directory.
You might run into quota problems but it's certainly no less secure.
is a better relnote work around:

mkdir /tmp/mozilla
chmod 700 /tmp/mozilla
setenv TMPDIR /tmp/mozilla
./mozilla

we heed TMPDIR, and the directory would be non-readable, even though the files
in it would be.

comments?
- It would require as many changes as fixing the original problem would.
- Cluttering the user's dir with tmp files is not good Unix behaviour either,
for several reasons:
  - Often, the home dirs are on a different volume, so that they don't get so
many write accesses (-> less potential for corruption).
  - Considering that we don't do a good job cleaning up, the left-behind files
have a much better chance to be deleten, if they are in /tmp than if they were
somewhere else.
Well, if user A and user B both try to create /tmp/mozilla, one of them is______
screwed. I would do_____________________________________________________________
     mkdir -p <profile_dir>/tmp____________________________________________________
     chmod 1700 <profile_dir>/tmp__________________________________________________
     export TMPDIR=<profile_dir>/tmp_______________________________________________
     ./mozilla_____________________________________________________________________
but that potentially exposes the profile dir's name which someone seems to think
should be hidden.
I'd use /tmp/mozilla-<profilename>. This also lessens the cahnce that the user
used teh same dir for extracting a mozilla tarball or similar. I'd also add an
|rm -rf TMPDIR at startup, so we clean up files left behind from the last
run(/crash).
Posted with Mozilla? :)
You're right. In bash syntax,
  mkdir -p $HOME/moz-temp
  chmod 1700 $HOME/moz-temp
  TMPDIR=$HOME/moz-temp ./mozilla
or, if we could add that work around to the mozilla script, and have it use the
PID for the tmp dir name.

mkdir /tmp/mozilla-$$

Going forward (thinking about the trunk),

1) this bug has a fix, and I'll check in once I get it reviewed.
2) rhp owns the bug where we should be using a directory under tmp for all temp
files.
3) rhp owns the bug where we aren't cleaning up after ourselves.
> mkdir -p $HOME/moz-temp

I'll kill you, if you create additional dirs/files in my home dir.

You can't use the profile dir because there's no way for the shell script to kno
what it is. I also wouldn't remove files because of a crash. If Mozilla was guar
to always exit with a non-zero code on error one could deal with it in a script.

> Posted with Mozilla? :)
Cut and paste from a virtual console into Lynx because of a mid-air collision.

> /tmp/mozilla-$$

Nope. There just might be one of these left over from soemone else's crash.
Any simple scheme like this in /tmp will always be a problem.

Of course, you could use tempnam to get a unique name. You could even use a uuid
string if you could generate it.

> I'll kill you, if you create additional dirs/files in my home dir.
Write your own wrapper script?
With a modern Linux system,
  TEMPNAM=/tmp/`uuidgen`
  mkdir -p $TEMPNAM
  chmod 1700 $TEMPNAM
  TMPDIR=$TEMPNAM ./mozilla
Guys, there is already a patch attached to this bug.  I see no reason why we
would change the script that executes the app rather than taking the patch.
What you should be doing is producing evidence that this is a stop ship bug.

Let's stop discussing nuances of umask in this bug, it was mentioned as a
release note item for concerned users, not a code change for the product.
If Mozilla is only supposed to work in a single-user environment, then this is
not a serious problem. If Mozilla is intended to work in a multi-user environment,
then this becomes a serious security issue and, perhaps, a denial of service issue.
Someone needs to decide which position is correct and clearly state so in the
release notes.
> What you should be doing is producing evidence that this is a stop ship bug.

I thought, the severity where out of question? In crash situations, we leave
sent files with world-readable permissions. On a multi-user system, this means
that other people  can read some of your sent mail. IMO, this *is* severe.

If anything, we need evidence that the *patch* is good, and preferably that it
fixes the problem completely.

Looks good to me: r: rhp
It is not just crash situations. All news postings are left whether you crash or 
not. Mail is left if there is a crash. All the talk about umask this and umask 
that is fine for Unix systems. But that leaves Mac and Windows users nowhere. 
Please don't give us any of the "Windows is not secure, so we can be a privacy 
leaking walking security disaster" bs about this either. Thanks ;-)
pdt: marking rtm++ , please check in ASAP into the branch
Whiteboard: [rtm-] relnote-user → [rtm++] relnote-user
sr=mscott on Seth's latest patch (11/5)
this this is going to land on the branch and the trunk, we no longer need to
relnote it.

mscott has sr the latest patch.  I'll land this right now.
Keywords: relnoteRTM
Whiteboard: [rtm++] relnote-user → [rtm++]
Does this patch fix Mozilla leaving the temp files behind, or is it just the 
Unix-only umask fix? I'll open a new bug for leaving the files behind in temp if 
this one needs to be closed.
this is only the permissions fixed.

there is already a bug on rhp about leaving the files behind, no need to log a
new one.
For those that would like to track that: bug 58979
Cool that it got it for rtm!

Thanks Jerry for the bug-number - I was just going to ask :).
has this fix been checked in yet?  I'm waiting to respin.

sorry about that granrose.

the fix has landed on the trunk and the branch.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
esther was able to verify this on linux, but on winnt there is a problem.

on my winnt, temp files are getting created with permissions 644 instead of 600.

reopening.  removing rtm++, this will not be a stop ship bug.

investigating.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [rtm++]
accepting.

from my nt box:

-rw-r--r--   1 0        everyone      387 Nov  8 13:58 nscopy.tmp
-rw-r--r--   1 0        everyone        5 Nov  8 13:58 nsmail-1.html
-rw-r--r--   1 0        everyone      385 Nov  8 13:58 nsmail.eml
-rw-r--r--   1 0        everyone       42 Nov  8 13:58 nsmail.html
Status: REOPENED → ASSIGNED
Is there something special about your NT box, or do all NT boxes do this?  Seems
like something we should have known before the checkin...
re-assign to cavin.

thanks cavin!
Assignee: sspitzer → cavin
Status: ASSIGNED → NEW
QA Contact: esther → stephend
*** Bug 71031 has been marked as a duplicate of this bug. ***
Any new here? Can someone verify this?
Mass removing self from CC list.
Now I feel sumb because I have to add back. Sorry for the spam.
Product: MailNews → Core
if patch fixed the persmissions can't this be closed? 
I don't see an inherent dependency on bug 58979


(In reply to comment #0)
> looking at my /tmp, I see this:
> 
> -rw-rw-r--   1 seth     seth          944 Oct 29 00:34 /tmp/nscopy-1.tmp
>...
> 1) the temp files are left behind and we'll fill up /tmp.  I've talk to rhp
> about this, and in addition to finding the problems and fixing them, we might
> want to add some code to shutdown to remove temp files.

also windows bug 77810
Assignee: cavin → nobody
QA Contact: stephend → backend
Product: Core → MailNews Core
No one has confirmed or denied fixture for years. As far as I can tell, this has been fixed (then again, the last comment mentioned not working on WINNT...).
Status: NEW → RESOLVED
Closed: 24 years ago → 14 years ago
Resolution: --- → INCOMPLETE
From initial description:
> 2) the permissions are bar.  we should make sure they are 600.

This seems to be fixed.

> 1) the temp files are left behind and we'll fill up /tmp.

This bug still exists:

-rw-------  1 ben  ben    1619 2010-10-28 13:07 nscopy-2.tmp
-rw-------  1 ben  ben    1383 2010-10-26 11:20 nscopy.tmp
-rw-------  1 ben  ben    1526 2010-10-28 13:07 nsemail-2.eml
-rw-------  1 ben  ben    1290 2010-10-26 11:20 nsemail.eml

This is from just a few days. When running longer, I had dozens, if not a hundred of them in /tmp/.
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Summary: temp files from sending drafts or posting news are create with bad permissions → Temp files from sending drafts or posting news are (created with bad permissions and) left behind
Looks like this could be fixed by bug 235432. Please reopen if you still see it (new temp files left behind).
Status: REOPENED → RESOLVED
Closed: 14 years ago → 12 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: