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

RESOLVED DUPLICATE of bug 235432

Status

MailNews Core
Backend
P3
normal
RESOLVED DUPLICATE of bug 235432
18 years ago
6 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Unassigned)

Tracking

({privacy})

Trunk
Future
x86
Linux
privacy

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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!

Comment 1

18 years ago
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.

Comment 2

18 years ago
> 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?

Comment 3

18 years ago
This is bug 49943

Comment 4

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

Comment 5

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

Comment 6

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

Comment 7

18 years ago
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.

Comment 8

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

Comment 9

18 years ago
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.

Comment 10

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

Comment 11

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

Comment 12

18 years ago
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.

Comment 13

18 years ago
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;

Comment 14

18 years ago
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.



Comment 16

18 years ago
Good! But that can't be the only source, because I also have them and can't use
news in Mozilla at all.

Comment 17

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

Comment 18

18 years ago
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.

Comment 19

18 years ago
From looking at the msg instances which are left behind for me, it might be
related to SaveAsDraft.

Comment 20

18 years ago
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]

Comment 22

18 years ago
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
Created attachment 18493 [details] [diff] [review]
fix, use 600 permissions when creating tmp files

Comment 25

18 years ago
Seth and I worked on a permissions fix that is in his tree right now.

- rhp
Assignee: rhp → sspitzer
Status: ASSIGNED → NEW

Comment 26

18 years ago
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=?

Comment 28

18 years ago
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!

Comment 29

18 years ago
Does PR_WRONLY | PR_CREATE_FILE represent any change from the previous behavior?

Comment 30

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

Comment 31

18 years ago
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.

Comment 33

18 years ago
True. 

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

- rhp
Created attachment 18552 [details] [diff] [review]
diff -u version of the fix, mscott can you review?

Comment 35

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

Comment 37

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

Comment 38

18 years ago
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?

Comment 39

18 years ago
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-]
Created attachment 18751 [details] [diff] [review]
new patch.  please re-review
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

Comment 45

18 years ago
umask on Win9x?

Comment 46

18 years ago
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.

Comment 47

18 years ago
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.

Comment 48

18 years ago
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?

Comment 49

18 years ago
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.

Comment 50

18 years ago
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?

Comment 51

18 years ago
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.

Comment 52

18 years ago
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.

Comment 53

18 years ago
> 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?

Comment 54

18 years ago
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?

Comment 56

18 years ago
- 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.

Comment 57

18 years ago
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.

Comment 58

18 years ago
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).

Comment 59

18 years ago
Posted with Mozilla? :)

Comment 60

18 years ago
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.

Comment 62

18 years ago
> mkdir -p $HOME/moz-temp

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

Comment 63

18 years ago
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?

Comment 64

18 years ago
With a modern Linux system,
  TEMPNAM=/tmp/`uuidgen`
  mkdir -p $TEMPNAM
  chmod 1700 $TEMPNAM
  TMPDIR=$TEMPNAM ./mozilla

Comment 65

18 years ago
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.

Comment 66

18 years ago
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.

Comment 67

18 years ago
> 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.

Comment 68

18 years ago
Looks good to me: r: rhp

Comment 69

18 years ago
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 ;-)

Comment 70

18 years ago
pdt: marking rtm++ , please check in ASAP into the branch
Whiteboard: [rtm-] relnote-user → [rtm++] relnote-user

Comment 71

18 years ago
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++]

Comment 73

18 years ago
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.

Comment 75

18 years ago
For those that would like to track that: bug 58979

Comment 76

18 years ago
Cool that it got it for rtm!

Thanks Jerry for the bug-number - I was just going to ask :).

Comment 77

18 years ago
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
Last Resolved: 18 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

Comment 82

18 years ago
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
Depends on: 74046
QA Contact: esther → stephend
*** Bug 71031 has been marked as a duplicate of this bug. ***

Comment 85

16 years ago
Any new here? Can someone verify this?

Comment 86

16 years ago
Mass removing self from CC list.

Comment 87

16 years ago
Now I feel sumb because I have to add back. Sorry for the spam.
Product: MailNews → Core

Comment 88

11 years ago
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
(Assignee)

Updated

10 years ago
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
Last Resolved: 18 years ago7 years ago
Resolution: --- → INCOMPLETE

Comment 90

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

Updated

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

Comment 91

6 years ago
Looks like this could be fixed by bug 235432. Please reopen if you still see it (new temp files left behind).
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 235432
You need to log in before you can comment on or make changes to this bug.