Forms/Necko: Temp file (formpost) left after file upload

VERIFIED FIXED in mozilla0.9.7

Status

()

defect
P1
critical
VERIFIED FIXED
20 years ago
2 months ago

People

(Reporter: pollmann, Assigned: darin.moz)

Tracking

(4 keywords)

Trunk
mozilla0.9.7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ready-to-land], )

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

20 years ago
When we upload a file (multipart form submission), we generate a temp file
containing the contents of the post.  This file is read by Necko on another
thread and thus can't be deleted right away.  Need to clean this up.

Sidenote:  This bug is also present in Nav!

Comment 1

20 years ago
We should fix the design to not create the temp file at all.
Reporter

Updated

20 years ago
Status: NEW → ASSIGNED
Reporter

Comment 2

20 years ago
I agree! :)

Comment 3

20 years ago
QA Contact update.
Reporter

Updated

20 years ago
Target Milestone: M14
Reporter

Comment 4

20 years ago
Hey Gagan, do you think M14 for this is reasonable?  How much work will this
take on your part?
Target Milestone: M14 → M17
Nonfatal and it was also present in Nav4.x, apparently. Moving to M17.
Reporter

Comment 6

19 years ago
Rescheduling
Target Milestone: M17 → M19
Reporter

Comment 7

19 years ago
This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another 
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration.

P.S. Gagan, if you want to take ownership of this bug, please feel free!  :)
Target Milestone: M19 → Future

Comment 8

19 years ago
heh... I like your style pollmann. I am adding helpwanted in case someone on the 
net has some time to do this. 
Keywords: helpwanted
Reporter

Comment 9

19 years ago
Sorry Gagan, that probably sounded bad.  Um, what I meant was, I probably won't 
get to this for beta2.  :S  I'd really really like to see it in though, and if 
anyone can fit it in I'd be much more than happy to lend a helping hand!

Comment 10

19 years ago
Updating QA contact.
QA Contact: ckritzer → vladimire
Reporter

Comment 11

19 years ago
*** Bug 58690 has been marked as a duplicate of this bug. ***
Reporter

Comment 12

19 years ago
*** Bug 43569 has been marked as a duplicate of this bug. ***
Reporter

Comment 13

19 years ago
Transferring status from duplicate bugs...
Keywords: privacy, rtm
Priority: P3 → P1
Whiteboard: [rtm need info]
Target Milestone: Future → ---
Reporter

Comment 14

19 years ago
*** Bug 58930 has been marked as a duplicate of this bug. ***
Reporter

Comment 15

19 years ago
Re-adding people to the CC list that added themselves to dups.

Notes from other bugs:

- The files have the permissions set according to your umask on UNIX systems.
  That is, for most people, world readable

- These files will only get created for file upload (multipart/form-data), *not*
  general form post.  That means about 99% of the forms you post will not leave
  these kinds of files around, and I've *never* seen one that includes
  information like a password / userid combo

- The idea is to not create these files at all, though this will be a
  significant amount of rewrite to move the form post header generation logic
  over to necko.  In fact, adding logic to remove them will also be a
  significant amount of rewrite - possibly more than just not creating them...

Comment 16

19 years ago
I don't think a _significant_ amount of rewrite is necessary, but certainly 
more than a null pointer check. ;-)

What I recall Gagan showing me this afternoon is that the tmp file is opened 
and the stream data is written to it. Then the file length is detected and this 
length is prepended to the beginning of the stream before sending it off to 
necko to post.

What you should do instead is append the data into an nsString instead of 
writing it to a file. Then get the string length and prepend the length header 
to the string. Finally, make a string stream out of the result -- all done in 
memory. You should only have to change nsFormFrame::ProcessAsMultipart.

Comment 17

19 years ago
I'll submit a patch that appears to work for Gagan's test case. Eric and Gagan 
should verify.
Keywords: patch
Whiteboard: [rtm need info]

Updated

19 years ago
Keywords: mozilla0.9, review

Comment 19

19 years ago
I would just like to say that I support Warren's idea and 
do not believe that they will have any adverse impact on 
input helper applications (bug 46135), most if not all of 
which will probably have to write temporary files which 
should then be read into a string and removed, though.

I hope that there will be some way to get a 'gensymed' 
temporary source file name sprintf-%s-like string for 
reading in that manner, so that we could configure 
arguments and output for input helper applications.  

For example:
  /usr/local/bin/yarec -record -file %s
  C:\windows\sndrec32 /RECORD %s
  HD:Applications:MacSoundRecorder %s
  /bin/sh -c '(cat /dev/audio & sleep 10; exit) > %s'
etc.
based on Eric comment:

 "these files will only get created for file upload (multipart/form-data), *not*
  general form post.  That means about 99% of the forms you post will not leave
  these kinds of files around, and I've *never* seen one that includes
  information like a password / userid combo"

Marking rtm-.

Whiteboard: [rtm-]
for the relnotes, we could suggest users add "umask 077" to their .cshrc to make
sure the temp files we create aren't readable by the world.

at some point, we need to fix the form posting code to create the temp files
with permissions == 600

Comment 22

19 years ago
> at some point, we need to fix the form posting code to create the temp files
> with permissions == 600

At some point you should take my patch which doesn't drop temp files at all.
Reporter

Comment 23

19 years ago
Warren, thanks for the patch!  That is much nicer than the hack I was working
on, which put the temp files in the cache directory instead of the temp
directory (and still left them lying around).

My only problem with using an nsCAutoString is that it could potentially get
*large*.  Admittedly, it won't be often that someone tries to upload a 100
Megabyte file, but when they do, they will be allocating a lot of memory and
could potentially crash or at least cause problems.  Since this didn't make RTM,
it might be better to work on a solution to this that Gagan explained to me - a
Necko stream wrapper that can wrap multiple streams and will do the right thing
- separating them with boundary headers and whatnot - and strip the boundary
generation logic out of the form frame completely.  This would eliminate the
copying of files to a temporary space entirely!  :)

Comment 24

19 years ago
I didn't realize that the string could get that large. I thought it was just 
including in-memory stuff. If it really can include contents of files, then 
Gagan's suggestion is probably the way to go: Create a stream that has a list 
of sub-streams inside it. Read data from one until eof, and then move on to the 
next. Some of these sub-streams could be file streams, others could be 
in-memory buffers.

Comment 25

19 years ago
> Some of these sub-streams could be file streams, others could be in-memory 
buffers.

Could some be the stdout of other programs?

Updated

19 years ago
Blocks: 60991
Reporter

Comment 26

19 years ago
*** Bug 64274 has been marked as a duplicate of this bug. ***
Reporter

Comment 28

19 years ago
Hope we never have to use that hack, but just in case, I put it on this bug
report before deleting the changes from my tree.

Has anyone in necko land heard of any progress on the "stream with list of
sub-streams" thing?

Updated

19 years ago
Keywords: nsbeta1
Assignee

Comment 29

19 years ago
There hasn't been any implementation AFAIK... though gagan might have started
something.

Updated

19 years ago
Blocks: 62582
*** Bug 70487 has been marked as a duplicate of this bug. ***

Comment 31

19 years ago
Does Mozilla respect any of these environment variables: TMP, TEMP, TMPDIR,
TEMPDIR?  I'd test this, but file upload in the current build doesn't do
anything at all.
we have code in nsSpecialSystemDirectory.cpp to check for the env variable
TMPDIR, and if that fails, we use /tmp.

nsFormFrame.cpp uses nsSpecialSystemDirectory::OS_TemporaryDirectory, so we will
respect TMPDIR.

TMP, TEMP, TEMPDIR, aren't respected.  (convention is TMPDIR)
Setting milestone to future.
Target Milestone: --- → Future

Comment 34

18 years ago
What about that security hole your leaving behind?

Comment 35

18 years ago
that should be covered by [Comments From Seth Spitzer 2000-11-03 16:34]
for the relnotes, we could suggest users add "umask 077" to their .cshrc to 
make sure the temp files we create aren't readable by the world.

If anyone is concerned about big programs, they probably want to do this 
anyways.
Keywords: rtmrelnote
Whiteboard: [rtm-]
And how is umask 077 going to help on the Macintosh?

Comment 37

18 years ago
on OSX i would expect it would work correctly.  On 8/9 umask doesn't exist, i 
know os/2 has a routine to create a temporary file buffer which is killed when 
the app quits, does macos? (and is this really a question for nspr?)
Nope. If you leave the file around, someone can read it.
Reporter

Comment 39

18 years ago
Will try to fold this into my form submission rewrite for 0.9.1
Target Milestone: Future → mozilla0.9.1
Reporter

Comment 40

18 years ago
*** Bug 76463 has been marked as a duplicate of this bug. ***
Reporter

Comment 41

18 years ago
*** Bug 70488 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 42

18 years ago
*** Bug 87603 has been marked as a duplicate of this bug. ***

Comment 43

18 years ago
reporter of (dup?) bug 87603 says /tmp/formpost files are also left around after
large <textarea> posts.

Comment 44

18 years ago
Missed 0.9.3.
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Comment 45

18 years ago
Win2K, NS6.1b1: The temp files left behind cannot be deleted until user quits
browser. Are the files left open?

Comment 46

18 years ago
Can I suggest this bug be fixed urgently?

I was just about to hand my old PC back to the IT department but decided to have
a little clean-up first. In c:/temp I found all the e-mails I'd written to my
girlfriend in the last month, readable by all! (Well, luckily I'm the only user
of the workstation and nearly always lock it, but the sys admins could have got
in there :-\).

This was Windows NT 4 SP 6a on an Intel platform running Mozilla 0.9.2, although
I think I saw it under Linux too. The files were formpost, formpost-1 etc. as
mentioned already, and in order to delete them I must kill the Mozilla process.

This problem DOES have an effect on real situations (see above) - I am using the
open source SquirrelMail (www.squirrelmail.org) webmail software (installed in
my web space). SquirrelMail uses the "multipart/form-data" enctype because it's
possible to attach a file to the message, and I'm sure there are other
situations where this could compromise personal or sensitive data.

Comment 47

18 years ago
Lars, this bug is for form posting temp files. What you are describing is an
issue with mail which was fixed in mozilla .9.3. 

Comment 48

18 years ago
mscott, lars said he was using webmail, not mail&news

Comment 49

18 years ago
oops...I didn't read that. Thanks Jeffrey.

Comment 50

18 years ago
eric is no longer around.  
we need some more time to figure out who can own this bug.
anybody have ideas?

unlikley this will make 0.9.4
Target Milestone: mozilla0.9.4 → mozilla0.9.5
The form posting code and HTTP needs to be reworked to use the new Upload API's 
that I was going to land for 0.9.5.  Darin or I should own this bug.
Assignee: pollmann → gagan
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.5 → ---
Assignee

Comment 52

18 years ago
taking... i have a plan which may allow us to eliminate the need for the temp
file altogether.
Assignee: gagan → darin

Comment 53

18 years ago
I'd like to comment on how this behaves in Mac OS 9.x:

1. The formpost files are created in the invisible Temporary Items folder.
2. Mozilla doesn't clean up the files when it quits.
3. When you next restart your machine, the Mac OS moves the formpost files to a
new "Rescued items" folder in the Trash, where they are now visible.

Comment 54

18 years ago
*** Bug 98915 has been marked as a duplicate of this bug. ***

Comment 55

18 years ago
First off, it's not just forms in which you upload a file, but any form with
sufficiently large amounts of data.  I do not know what the threshold is.

As far as the file upload is concerned, might I suggest a possible course of
action?  Since the file to be sent already exists on the local host, you do not
need to make a copy.  Simply place a link to the file at the relevant portion. 
Content lenght can easily be calculated as there is a direct relationship
between size of the file before encoding, and size after (eg, base64 causes file
size to increase to the lowest integer greater than 4/3 the file size).  Most
meta information is fixed length, or can at least be calculated before it is
required.

Assignee

Comment 56

18 years ago
targeting for moz 0.9.6
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.6
*** Bug 102968 has been marked as a duplicate of this bug. ***
*** Bug 103413 has been marked as a duplicate of this bug. ***

Updated

18 years ago
Blocks: 104166
Assignee

Comment 59

18 years ago
-> 0.9.8
Target Milestone: mozilla0.9.6 → mozilla0.9.8
*** Bug 110283 has been marked as a duplicate of this bug. ***
I just discovered these files and they are a security problem for me. In comment
15 it was stated:

- These files will only get created for file upload (multipart/form-data), *not*
  general form post.  That means about 99% of the forms you post will not leave
  these kinds of files around, and I've *never* seen one that includes
  information like a password / userid combo

Well, I use it that way. I'm writing a system where files can either be uploaded
or fetched through http, so I have to use multipart/form-data for my form. Since
I want to fetch http that has to be authorized, I also have a username/password
field. I went looking at the form- files and found, in clear text, the HTTP
password.

Of course other people might use passwords for completely different reasons.
Leaving something like this open for so long is just amazing to me. I didn't
find a security keyword or severity setting, but IMO the severity of this bug is
definitely under-rated. 
Summary: Forms/Necko: Temp file left after file upload → Forms/Necko: Temp file (formpost) left after file upload
Assignee

Comment 62

18 years ago
eric: it sounds to me like your concerns would be solved if we changed the
default file permissions of the saved files.  if this is true, can you please
file a separate bug?  that might be a much quicker fix.  thx!
There is/was a bug for that, bug 76463 and it has been closed in favor of this one.

Certainly more restrictive permissions are somewhat better, but still not
adequate. A password should never be stored in a non-encrypted state on any
medium. If someone comprimises my machine or my account, they can then have
access to other accounts on other machines. 

Comment 64

18 years ago
Sorry Darin, but I respectfully disagree here. I don't think it fair to expect
users to allow their passwords and other personal information to be accessible
to system administrators, not to mention the risk of unauthorised access.

To be honest I'm very surprised and disappointed about the complacency shown
with respect to this bug. If this flaw existed in a Microsoft product it would
be headline news.

Ok, so Mozilla isn't at 1.0 yet, so in theory nobody should be using it for
anything important or sensitive, but the warnings in this respect are pretty
mild given Mozilla's real-world adoption.

Please make Mozilla generate a Big Fat Warning(tm) before submitting affected
forms until this is fixed. :-)
I've reopened bug 76463 to handle the permissions issue.  While Lars is correct
that that's not a full solution, it _is_ a much more pressing (and much simpler
to fix) issue.

Comment 66

18 years ago
In windows it's no solution to change it's rights, so it's a mojor problem
there, that will not be patched even by changing the security rights (because
normaly there are none).
Assignee

Comment 67

18 years ago
upping severity to critical
Severity: normal → critical
Assignee

Comment 68

18 years ago
on UNIX filesystems and probably other modern filesystems, its possible to open
a file for reading, unlink it, and then proceed to read it.  the file is only
visible in the filesystem for the time between the call to open and the call to
unlink.  afterwords it's only reference is the file descriptor owned by the
application.

i don't know if this is supported on windows or macos, but even if its not it
might be the right solution for XP_UNIX mozilla since that is where it's most
common for multiple users to be running mozilla at the same time.

obviously, we'll still need a solution for other platforms to ensure that
mozilla cleans up any temporary files at shutdown.

unlinking the file after it has been opened for reading is something the file's
creator can do, which should translate to a fairly simple patch.
Assignee

Comment 69

18 years ago
here's a patch that works on XP_UNIX.  i'll have to test this on other
platforms... not sure if this'll work on macos or win32.
Assignee

Comment 70

18 years ago
gordon, simon, beard: would HFS support this solution (attachment 60064 [details] [diff] [review])?
Assignee

Comment 71

18 years ago
this solution seems to work on win2k (tested both Fat32 and NTFS)... the only
difference is that the file is not actually deleted until the application shuts
down.  on linux, i noticed that the file was removed its parent immediately.

i wonder how win9x behaves.

Comment 72

18 years ago
Bug 52413 Comment 8 just seemed so fitting (think about the unix file system 
implementation of deleting an open file)
on windows we can probably use the sharing apis to lock files for exclusive 
use.

Comment 73

18 years ago
I think that there is another issue, besides the files being left behind.  The
temporary files should not be created in a publicly-readable directory.  Such
files may contain sensitive information, so, if there is as much as a
*possibility* that they can be read by another user, by employing a race, or due
to the application crash, the it the solution is not the right one.  The
unlinking solution is definitely not the right one from the security standpoint.

I think that if the temporary files are absolutely needed at all, then, at the
very least, they must be created somewhere under ~/.mozilla, and not in /tmp. 
*And* Mozilla should be forcing umask(0700) on them.

It would be nice to get rid of the temp files altogether.  I don't know
Mozilla's internals, but since the forms contents are entered in the
application, it should be possible to keep the info in memory, without dumping
it to a temp file.  If it's a file upload, then you already have the file on
disk: what's the need to create a second copy?

Comment 74

18 years ago
Rats, I meant umask(0077).

Comment 75

18 years ago
so run w/ a umask, or setenv TMPDIR to something stupid.

You can currently do either of these things.  If you want changes to happen 
sooner then provide patches. Ranting is not an acceptable way to speed up 
development.

And in case you didn't notice, this is the wrong bug to rant in, you want bug 
76463 to handle the permissions issue. And WE don't want you to rant anywhere.

Comment 76

18 years ago
timeless@bemail.org writes:

> so run w/ a umask, or setenv TMPDIR to something stupid.
> You can currently do either of these things.  

So, you are saying that the application's developers should not be
concerned with security, and that that concern should be delegated to
the user?  This is, at the least, silly.

> Ranting is not an acceptable way to speed up development.

What *you* wrote was a rant.  What I wrote was a criticism of a
proposed solution.

> And in case you didn't notice, this is the wrong bug to rant in, you
> want bug 76463 to handle the permissions issue. And WE don't want
> you to rant anywhere.

What makes you think that you can speak for the entire development
community?  For one, I can't spend time to personally fix bugs in
every application I use, so I'm trying to contribute by sending bug
reports and participating in discussions of fixing them.  Secondly, if
you look at comments # 1,2,15,16,21,22,23 and more, you'll see that
I'm not alone in my opinion.  Bug 76463 is about permissions.  This
bug is about temp files, which are the root of the problem.  If they
can be done without, so it should be.
> What I wrote was a criticism of a proposed solution.

It's not clear to me that this is a proposed solution.  It seems more likely
that it's a temporary workaround to make this problem not as immediately
pressing while the real solution is worked on....
Assignee

Comment 78

18 years ago
exactly, in combination with bz's fix for bug 76463, we'll be creating very
short lived temporary files in /tmp with proper permissions (similar to those
that you'd have if the file were created in the users profile directory).  this
solution should be fairly sufficient from a security point of view.  a user with
root permission however would still be able to access the file, but i don't
think mozila is in a position yet to solve the problem of protecting the user
against a compromised root account.  IMO it's really a meta bug, blocked by a
large? number of bugs.

of course, in the long run we do plan to eliminate this temp file, since there
really isn't anything forcing us to use it.  i think it was just a quick
implementation to get form posting working that unfortunately has stuck with us
for longer than it really should have :(

Comment 79

18 years ago
Will this temporary fix affect all versions of the browser? I use it on Windows
XP as well as Linux.

I know it would be unfair to expect developers to fix everything, but until it
is fixed *properly* please ensure there is an adequate warning for ordinary
users at the point of use. For better or worse, Mozilla is now being
distributed/used in some places as though it were a finished product (e.g. new
Linux distros).

The way this bug has been dealt with has substantially changed my views in the
open vs closed security debate. :-(
Keywords: edt0.9.4
Assignee

Comment 80

18 years ago
Lars: it looks like this solution is going to work fine on win32 (see my
comments above), and i think that it is also a sufficient solution.

IMO it's unreasonable to expect that a user's browser session can be immune to a
compromised root account.  afterall, someone with root access could easily
replace the browser application with _anything_ they like.
Assignee

Comment 81

18 years ago
-> 0.9.7
Target Milestone: mozilla0.9.8 → mozilla0.9.7
On Mac, deleting a file that is open for reading or writing will just fail and 
return an error, I think.

Comment 83

18 years ago
I don't believe that unlinking (deleting) an open file on Mac OS 9/8 will 
work, it will return an error. Here's a reference:

http://developer.apple.com/techpubs/mac/Files/Files-193.html

Likely this will only work in the mach-o build on Mac OS X.

Comment 84

18 years ago
Should NSPR have a file open primitive which has a "delete on close" option? 
OpenVMS has a "delete on close" option on open(), as well as another option 
"temporary" which means "don't bother entering this file into the directory". I 
could use both of these on OpenVMS, given half a chance. Don't hack it with 
open/unlink which might work on some platforms and may break on those platforms 
tomorrow.
Assignee

Comment 85

18 years ago
oh well... it was worth a shot :)

sounds like the right solution may then be to add a parameter to
NS_NewLocalFileInputStream... something like deleteOnClose.  the impl could do
the right thing depending on the platform.  on XP_UNIX and XP_WIN the existing
solution should be good. on other platforms, the impl could hold onto the
nsIFile object and unlink the file after the file descriptor is closed.  this
latter solution would incur some bloat since the impl currently doesn't hold
onto the nsIFile object.
Assignee

Comment 86

18 years ago
Attachment #18633 - Attachment is obsolete: true
Attachment #22317 - Attachment is obsolete: true
Attachment #60064 - Attachment is obsolete: true
Comment on attachment 60555 [details] [diff] [review]
patch - XP solution

looks fine.
Attachment #60555 - Flags: review+
Assignee

Updated

18 years ago
Whiteboard: [patch needs r/sr=]
Assignee

Updated

18 years ago
Whiteboard: [patch needs r/sr=] → [patch needs sr=]

Comment 88

18 years ago
I'm psyched we're finally fixing this on the trunk. going to minus on the 0.9.4
branch though. Doug and I just had a healthy debate about shoving lifetime info
into the init API; I lost.
Keywords: edt0.9.4edt0.9.4-

Comment 89

18 years ago
Comment on attachment 60555 [details] [diff] [review]
patch - XP solution

sr=mscott
Attachment #60555 - Flags: superreview+
Assignee

Updated

18 years ago
Whiteboard: [patch needs sr=] → [ready-to-land]
Assignee

Comment 90

18 years ago
checked in attachment 60555 [details] [diff] [review], marking this bug FIXED.

i'm going to open another bug to track eliminating the temporary file altogether.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Assignee

Comment 91

18 years ago
bug 114106 tracks eliminating this temp file altogether.

Comment 92

18 years ago
win32 build 2001121003, win98se

I gather from the debates going on here that the "fix" for this bug was to purge
the temp files on close so they vanish when their useful life is over.  If this
is correct, then this bug is not fixed on Win98.  Tested by running some html
files through http://validator.w3.org/file-upload.html.  formpost files are
still collecting in the temp folder and don't go away when I shut down the browser.
No, the fix here is to set the files as readable only to the user. Follow bug
114106 for the final fix.
Assignee

Comment 94

18 years ago
no no no no... the patch that landed to "fix" this bug was supposed to make the
temp files go away.  if the temp files are still being left around on Win98,
then something is wrong.  i'll try to test this out on Win98 as soon as i get a
chance.

-> reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 95

18 years ago
closing this in favor of bug 114778, scheduled for 0.9.8
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED

Updated

18 years ago
No longer blocks: 62582

Comment 96

18 years ago
There is no more temp files in linux 2002-01-28-08-trunk and 2002-01-25-03 on
windows 2000, and 2002-01-29-03-trunk windows 98
Status: RESOLVED → VERIFIED

Updated

18 years ago
Keywords: topembed

Comment 98

18 years ago
still happening on win2k with moz 2002020406
2002020406-trunk?  Or the 0.9.8 release?  0.9.8 branched before this got fixed.

Updated

17 years ago
Blocks: 51666
Component: HTML: Form Submission → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.