83065 - multipart/form-data file upload with php corruption

Status: VERIFIED FIXED

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

Description

[Marc Jauvin]

From Bugzilla Helper:
User-Agent: Mozilla/4.77 [en] (X11; U; Linux 2.4.2-SGI_XFS_1.0 i686)
BuildID:    2001052706

When I upload a file, it's some bytes longer afterwards, and I have
"ntent-Transfer-Encoding: Binary" prepended at the beginning of the file.

Reproducible: Always
Steps to Reproduce:
just use a php based website to upload a file (using
enctype="multipart/form-data") and look at the uploaded file.

Comment 1

[Marc Jauvin]

To reproduce, do this:

- go to http://mail.domains.com
- use "demo" as the username and password
- once logged in, click on COMPOSE
- attach a file
- send the email to yourself
- try to open the attachment on the email upon reception.

Comment 2

[rods (gone)]

reassigning

Comment 3

[Vladimir Ermakov]

confirming

Comment 4

[Marc Jauvin]

There is another (probably linked) side effect; when a FUL PATH is specified as
the attachment source file, this is also sent as the attachment filename,
preventing you from opening the attachment in your mail program.

To see a quick exemple, look into the "sent-mail" folder on the demo account and
try to open the mail with "FULL PATH" as a subject. I tried opening this email
with Netscape 4.76 and it shows the same behavior.

Comment 5

[Eric Pollmann]

The corruption / header data that bleeds into the document is because the server
side makes incorrect assumptions about the number of headers sent for each part
of a multipart form.  In short, this is a server-side bug and should be
corrected on the server side.  (For reference, the Content-Transfer-Encoding
header is recommended in the HTML spec here:
http://www.w3.org/TR/html4/interact/forms.html#h-17.13.4.2
and headers are not limited to a fixed number in the mime spec.

This problem became apparent as a result of the checkin for bug 58189.  The
server is assuming there are exactly two headers, followed by two bytes
(assuming those are CR and LF), then the data.  Since we checked in the patch on
bug 58189, we have three headers, so the two bytes that are ignored are the C
and o of "Content-Transfer-Encoding: Binary" instead of the expected CR and LF.

I will look into the full-path issue...

Comment 6

[Adrian Havill]

This is a known problem with the PHP multipart/form-data parser that is
mentioned in the comments for bug 44064.

PHP's broken parser expects the last header to be Content-Type... anything after
it it treats as data. (To give you an idea of how broken the parser is, PHP will
also fail if there's more or less whitespace around tokens, and will fail if
some values are not in double-quotes). Basically, the file main/rfc1867.c in the
PHP source code needs to be rewritten.

PHP is still popular and we can't ignore the existing parsers out there, so the
solution is to make sure Content-Type is last (and that the whitespace and
token-quoting usage follow what PHP 4.0.4pl1 expect)

As bug 44064 mentions, I've tested MS IIS/ASP, Perl's CGI.pm, and other popular
multipart/form-data server parsers-- PHP is the only (popular) server-side that
seems to be inflexible regarding the header order and whitespace/quote usage.

Later patches for bug 44064, which address Content-Transfer-Encoding, address
this issue.

As for the full-path issue, this is because someone (rods@?) removed the
nsFormFrame::GetFileNameWithinPath call/func from nsFormFrame.cpp. I assume they
did this because it makes non-crossplatform/I18N unsafe assumptions in its
current form. This also is addressed in the patches in bug 44064. (Made
GetFileNameWithinPath grok Unicode and Mac dir seps correctly)

Comment 7

[Adrian Havill]

Doh! Change all the references to "bug 44064" to "bug 44464" in the previous
comment. That'll teach me to not try to recall bug numbers from memory.

Comment 8

[Eric Pollmann]

FWIW, I removed GetFileNameWithinPath for bug 59408, to make us compatible with
IE and Nav 4.x - if we are now incompatible, we should determine what cases
cause IE/Nav 4.x to send the path, and which do not...

Comment 9

[Eric Pollmann]

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

Comment 10

[Eric Pollmann]

See fix on bug 44464

Comment 11

[Eric Pollmann]

After some investigation, the problem with uploading a file and sending it's
full path appears to be Linux specific.  (Have not tested Mac)  Nav 4.x and IE
5.x both send the full path on Windows, and sending the full path with
Moz/Netscape 6.x is handled properly on the server side.

I noticed that when I attached an image file on Windows, and sent it to another
account, even then I could not see the image.  The problem turns out to be the
corruption due to PHP, so should be fixed by the patch on 44464.  I'll update
the patch there to make only Linux truncate the file path to a leaf file name.

Comment 12

[Eric Pollmann]

On second thought, reviewing bug 59408, it should probably have just been marked
WONTFIX.  The security concerns raised there seem like they could be valid, and
I have not seen any bugs due to our differences from IE and Nav in the past (not
checked in until 11-April).  Will update that one...

Comment 13

[Eric Pollmann]

Since this is fixed by Adrian's patch on bug 44464, marking DUP to keep things
all under one roof...

*** This bug has been marked as a duplicate of 44464 ***

Comment 14

[Vladimir Ermakov]

verifying dupe

Comment 15

[Eric Pollmann]

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

Comment 16

[Eric Pollmann]

Sorry, on second thought, marking this as a duplicate confuses things more than
it helps.  I'll split out the section of the patch that fixes this bug and post
it here, since the rest of the patch likely won't be able to be checked in.

Comment 17

[Eric Pollmann]

Attachment: Fix corruption (turn off ContentTransferEncoding header by default)

Comment 18

[Eric Pollmann]

This first patch make generation of the Content-Transfer-Encoding header (fix
for bug 58189) controllable by a pref, and default to not sending it, as there
are known problems with some web sites.  It is unfortunate that we must do this
to create a browser with an overall better user experience, but for now, this is
the case.  It also moves the Content-Transfer-Encoding header up above the
Content-Disposition and Content-Type headers, which fixes compatability with the
broken PHP form upload parser.

Comment 19

[Eric Pollmann]

Attachment: Fix full path bug (includes previous patch, addresses security concern and incompatabilities with previous Linux browsers)

Comment 20

[Eric Pollmann]

This second patch includes the first patch, but also backs out bug 59408 so that
we will no longer send the full path of files that are being uploaded to the
server.  This fixes the problem with linux on the mail.domains.com testcase, and
addresses potential security issues with sending the full path.  It also
includes some i18n work done by Adrian to make GetFileNameWithinPath work for
Unicode file names.

Both patches were work done by Adrian Havill for bug 44464, but slightly
simplified and split out from that patch so that they can be checked in sooner
(as the patch there is now out of date).

Comment 21

[Eric Pollmann]

CC'ing people who might be interested in reviewing this code.  Adrian, Scott,
can you r/sr the second patch (which includes both the full path and header
fixes)?  Thanks!

Comment 22

[Adrian Havill]

Pollman: your patch looks good to me. I'm wondering whether sending the entire
path is a good idea (security concerns about revealing the path?)

Will play with IE to see whether it sends the full path or not.

Comment 23

[Eric Pollmann]

IE does send the entire path on Windows, as does Netscape Communicator 4.x.  We
traditionally have not (did not until the 0.9.1 milestone build).  I checked in
a patch that 'fixed' this incompatability and checked it in without thinking to
get a security review.

However, my fix caused problems with server side scripts that did *not* expect
the full path from UN*X systems (Communicator 4.x never sent the full path on
UN*X).  So, since there were never any problems reported all the way up to 0.9.1
with just sending the leaf file name, my suggestion is that we revert my 'fix'
(along with the changes you made to internationalize GetFileNameWithinPath), and
use the second patch above.  This will cause us to again, as before, only send
the leaf file name and not the full path.

Comment 24

[Scott MacGregor]

sr=mscott

Comment 25

[Eric Pollmann]

Adrian, thanks for the call.  :)

Adrian said (essentially) "I've looked at the patch and it looks good to me"
which I think qualifies as r=[e-mail] for code Adrian wrote and
r=[e-mail] for how I split it apart.

Comment 26

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 27

[Eric Pollmann]

Fix checked in.

To verify:
1) Corruption fix

Go to http://geocities.yahoo.com/filemanager/upload
Log in with: login: testcase83442, password: testcase

Try to upload an image file.  You should not get an error message "Invalid file"

2) Full path fix

On a Linux or other UN*X box, go to http://mail.domains.com
Log in with: login: demo, password: demo
Click on Compose
Attach a file (an image file, for example)
Send the message (to yourself or whoever)
Now go to the Sent folder, and find the message you sent.
Click on the Attachment link.  You should be able to view the image.

Thanks Adrian for analyzing the problem, and coming up with the fix for both of
these problems!

Comment 28

[Boris Zbarsky [:bzbarsky]]

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

Comment 29

[clayton]

Marking PDT+

Comment 30

[Eric Pollmann]

Oops, forgot to mark this FIXED.  See my previous comment for verification
instructions.

Comment 31

[Vladimir Ermakov]

Verifying fixed on build 2001-06-21-04-trunk windows 98
and build 2001-06-20-21-trunk linux red hat 6.2

Comment 32

[Grey Hodge (jX)]

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