97851 - temporary file handling in file uploads leaves file with world readable permissions

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Christopher Blizzard (:blizzard)]

Build is from Aug 31, 2001.

If you upload a file using multipart/form-data ( like filing an attachment for
example ) the file that is left in /tmp for the form upload is world readable. 
This isn't acceptable on multi-user systems like unix.

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: first pass at a patch

Comment 2

[Darin Fisher]

blizzard:

you'll want to make sure that the cstring path handling is correct on the mac...
cc'ing dougt to review this portion.

also, what about using a buffered output stream for writing to the file?

Comment 3

[Christopher Blizzard (:blizzard)]

Actually, I was going to ask you about that.  I think that I can tell what I
need to do.  I'll whip up another patch.

Comment 4

[Christopher Blizzard (:blizzard)]

Attachment: patch #2

Comment 5

[Christopher Blizzard (:blizzard)]

OK, this patch includes the buffered output stream, as requested.  What's the
deal with the file path names?

Comment 6

[Darin Fisher]

it happens that a mac file is not necessarily uniquely identified by a character
string.  use nsIDirectoryService instead of nsSpecialSystemDirectory, and you
can get a nsIFile directly without manipulating c-string paths.

Comment 7

[Christopher Blizzard (:blizzard)]

Attachment: patch that uses nsIFile

Comment 8

[Christopher Blizzard (:blizzard)]

OK, this is a patch that seems to work locally and uses nsIFile instead of
nsFileSpec.  Seems clean to me and uses the right stream classes as well.  Can
you guys review?  I'm especially interested in making sure that I haven't
created any leaks.

Comment 9

[Pete Collins (MDG)]

+ postDataFileName->Append("formpost");

should be:

rv = postDataFileName->Append("formpost");
if (NS_FAILED(rv))
  return rv;

It is good to test all accessors.

In the curent nsIFile reorganization i'm currenty working on, Append will be
taking an nsAString. (Just as a heads up).

I'll convert this locally after you check this in.

I didn't see any leaks. Anyone else?

r=pete

Comment 10

[Christopher Blizzard (:blizzard)]

Stealing this bug.

Comment 11

[Doug Turner (:dougt)]

After calling NS_GetSpecialDirectory it is sufficant to just check for the
resulting out nsIFile.  

Other than that nit, sr=me

Comment 12

[Chris Waterson]

+      if (NS_FAILED(rv)) {
+        NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to close post data file stream!\n");


You can use NS_ERROR() for unconditional assert-botch. Other than that, looks
good. r=waterson

Comment 13

[Jaime Rodriguez, Jr.]

This one looks worthy to me. Nominating for nsbranch+.

Comment 14

[Jaime Rodriguez, Jr.]

PDT+ and nsbranch + per PDT

Comment 15

[Christopher Blizzard (:blizzard)]

Attachment: final patch

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 48399 [details] [diff] [review]
final patch

a=dbaron on behalf of drivers

Comment 17

[Christopher Blizzard (:blizzard)]

Checked into the 0.9.4 branch.

Comment 18

[Christopher Blizzard (:blizzard)]

Checked in.

Comment 19

[Vladimir Ermakov]

Verifying on 2001-10-08-08-trunk The file is rw for root only..