10.52 KB, patch
|Details | Diff | Splinter Review|
10.52 KB, patch
|Details | Diff | Splinter 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.
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?
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?
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
Created attachment 18552 [details] [diff] [review] diff -u version of the fix, mscott can you review?
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?
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.
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
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 → ---
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
17 years ago
Depends on: 74046
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.
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
Last Resolved: 18 years ago → 7 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
Last Resolved: 7 years ago → 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 235432
You need to log in before you can comment on or make changes to this bug.