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!

We should fix the design to not create the temp file at all.

I agree! :)

QA Contact update.

Hey Gagan, do you think M14 for this is reasonable? How much work will this take on your part?

Nonfatal and it was also present in Nav4.x, apparently. Moving to M17.

Rescheduling

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! :)

heh... I like your style pollmann. I am adding helpwanted in case someone on the net has some time to do this.

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!

Updating QA contact.

*** Bug 58690 has been marked as a duplicate of this bug. ***

*** Bug 43569 has been marked as a duplicate of this bug. ***

Transferring status from duplicate bugs...

*** Bug 58930 has been marked as a duplicate of this bug. ***

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...

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.

I'll submit a patch that appears to work for Gagan's test case. Eric and Gagan should verify.

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-.

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

> 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.

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! :)

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.

> Some of these sub-streams could be file streams, others could be in-memory buffers. Could some be the stdout of other programs?

*** Bug 64274 has been marked as a duplicate of this bug. ***

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?

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

*** Bug 70487 has been marked as a duplicate of this bug. ***

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.

What about that security hole your leaving behind?

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.

And how is umask 077 going to help on the Macintosh?

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.

Will try to fold this into my form submission rewrite for 0.9.1

*** Bug 76463 has been marked as a duplicate of this bug. ***

*** Bug 70488 has been marked as a duplicate of this bug. ***

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

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

Missed 0.9.3.

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

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.

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.

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

oops...I didn't read that. Thanks Jeffrey.

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

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.

taking... i have a plan which may allow us to eliminate the need for the temp file altogether.

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.

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

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.

targeting for moz 0.9.6

*** Bug 102968 has been marked as a duplicate of this bug. ***

*** Bug 103413 has been marked as a duplicate of this bug. ***

-> 0.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.

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.

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.

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).

upping severity to critical

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.

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.

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

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.

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.

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?

Rats, I meant umask(0077).

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.

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....

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 :(

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. :-(

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.

-> 0.9.7

On Mac, deleting a file that is open for reading or writing will just fail and return an error, I think.

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.

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.

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.

Comment on attachment 60555 [details] [diff] [review] patch - XP solution looks fine.

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.

Comment on attachment 60555 [details] [diff] [review] patch - XP solution sr=mscott

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.

bug 114106 tracks eliminating this temp file altogether.

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.

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

closing this in favor of bug 114778, scheduled for 0.9.8

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

still happening on win2k with moz 2002020406

2002020406-trunk? Or the 0.9.8 release? 0.9.8 branched before this got fixed.