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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
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;
Comment 14•24 years ago
|
||
rather
while ls -la /tmp;
do
sleep 1
clear;
done
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.
Comment 20•24 years ago
|
||
I just test the Branch Win32 bits and it is cleaning up properly.
- rhp
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
Comment 26•24 years ago
|
||
Yes, this is a good fix!
r: rhp
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.
Comment 33•24 years ago
|
||
True.
Steve: yes, these are all new temp files being created.
- rhp
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?
Comment 39•24 years ago
|
||
Jerry, the fix doesn't address that issue either.
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 45•24 years ago
|
||
umask on Win9x?
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 59•24 years ago
|
||
Posted with Mozilla? :)
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 68•24 years ago
|
||
Looks good to me: r: rhp
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
Comment 71•24 years ago
|
||
sr=mscott on Seth's latest patch (11/5)
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 75•24 years ago
|
||
For those that would like to track that: bug 58979
Comment 76•24 years ago
|
||
Cool that it got it for rtm!
Thanks Jerry for the bug-number - I was just going to ask :).
Comment 77•24 years ago
|
||
has this fix been checked in yet? I'm waiting to respin.
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
|
||
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
Comment 84•23 years ago
|
||
*** Bug 71031 has been marked as a duplicate of this bug. ***
Comment 85•23 years ago
|
||
Any new here? Can someone verify this?
Comment 86•23 years ago
|
||
Mass removing self from CC list.
Comment 87•23 years ago
|
||
Now I feel sumb because I have to add back. Sorry for the spam.
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
•