Closed
Bug 58580
Opened 24 years ago
Closed 13 years ago
Temp files from sending drafts or posting news are (created with bad permissions and) left behind
Categories
(MailNews Core :: Backend, defect, P3)
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 235432
Future
People
(Reporter: sspitzer, Unassigned)
References
Details
(Keywords: privacy)
Attachments
(3 files)
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!
Comment 1•24 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•24 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 4•24 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•24 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•24 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•24 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•24 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•24 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•24 years ago
|
||
Is there agreement that this is limited to crashes and other such things that prevent the cleanup from happening?
Comment 11•24 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•24 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•24 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;
Reporter | ||
Comment 15•24 years ago
|
||
I know one way to reproduce this. postings a news message seems to leave tmp files behind.
Comment 16•24 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•24 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•24 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•24 years ago
|
||
From looking at the msg instances which are left behind for me, it might be related to SaveAsDraft.
Reporter | ||
Comment 21•24 years ago
|
||
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•24 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
Reporter | ||
Comment 23•24 years ago
|
||
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
Reporter | ||
Comment 24•24 years ago
|
||
Comment 25•24 years ago
|
||
Seth and I worked on a permissions fix that is in his tree right now. - rhp
Assignee: rhp → sspitzer
Status: ASSIGNED → NEW
Reporter | ||
Comment 27•24 years ago
|
||
looking for sr=. mscott?
Status: NEW → ASSIGNED
Whiteboard: [rtm need info] → [rtm need info] r=rhp, sr=?
Comment 28•24 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•24 years ago
|
||
Does PR_WRONLY | PR_CREATE_FILE represent any change from the previous behavior?
Comment 30•24 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•24 years ago
|
||
Hmm. I assumed these were all the same. Are any of them NOT opening new files for creation?
Reporter | ||
Comment 32•24 years ago
|
||
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.
Reporter | ||
Comment 34•24 years ago
|
||
Comment 35•24 years ago
|
||
sr=mscott
Whiteboard: [rtm need info] r=rhp, sr=? → [rtm need info] r=rhp, sr=mscott
Reporter | ||
Comment 36•24 years ago
|
||
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•24 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•24 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?
Reporter | ||
Comment 40•24 years ago
|
||
selmer, yes umask would solve the problem. jerry, see http://bugzilla.mozilla.org/show_bug.cgi?id=58979
Reporter | ||
Comment 41•24 years ago
|
||
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-]
Reporter | ||
Comment 42•24 years ago
|
||
Comment 43•24 years ago
|
||
This is scary. What on earth should the relnote be? "Wipe /tmp after using Mozilla Mail"? Gerv
Whiteboard: [rtm-] → [rtm-] relnote-user
Reporter | ||
Comment 44•24 years ago
|
||
for a user relnote, you can tell people to set their umask to 077
Comment 46•24 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•24 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•24 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•24 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•24 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•24 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•24 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•24 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•24 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.
Reporter | ||
Comment 55•24 years ago
|
||
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•24 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•24 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•24 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 60•24 years ago
|
||
You're right. In bash syntax, mkdir -p $HOME/moz-temp chmod 1700 $HOME/moz-temp TMPDIR=$HOME/moz-temp ./mozilla
Reporter | ||
Comment 61•24 years ago
|
||
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•24 years ago
|
||
> mkdir -p $HOME/moz-temp
I'll kill you, if you create additional dirs/files in my home dir.
Comment 63•24 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•24 years ago
|
||
With a modern Linux system, TEMPNAM=/tmp/`uuidgen` mkdir -p $TEMPNAM chmod 1700 $TEMPNAM TMPDIR=$TEMPNAM ./mozilla
Comment 65•24 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•24 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•24 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 69•24 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•24 years ago
|
||
pdt: marking rtm++ , please check in ASAP into the branch
Whiteboard: [rtm-] relnote-user → [rtm++] relnote-user
Reporter | ||
Comment 72•24 years ago
|
||
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•24 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.
Reporter | ||
Comment 74•24 years ago
|
||
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 76•24 years ago
|
||
Cool that it got it for rtm! Thanks Jerry for the bug-number - I was just going to ask :).
Reporter | ||
Comment 78•24 years ago
|
||
sorry about that granrose. the fix has landed on the trunk and the branch.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 79•24 years ago
|
||
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=day&mindate=&maxdate=&cvsroot=%2Fcvsroot The webtools are your friend.
Reporter | ||
Comment 80•24 years ago
|
||
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++]
Reporter | ||
Comment 81•24 years ago
|
||
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•24 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...
Reporter | ||
Comment 83•24 years ago
|
||
re-assign to cavin. thanks cavin!
Assignee: sspitzer → cavin
Status: ASSIGNED → NEW
QA Contact: esther → stephend
Updated•20 years ago
|
Product: MailNews → Core
Comment 88•17 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•16 years ago
|
Product: Core → MailNews Core
Comment 89•14 years ago
|
||
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
Comment 90•14 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•14 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•13 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
Closed: 14 years ago → 13 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•