multipart/form-data file upload with php corruption

VERIFIED FIXED in mozilla0.9.2

Status

()

Core
HTML: Form Submission
--
major
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: Marc Jauvin, Assigned: Eric Pollmann)

Tracking

({dataloss})

Trunk
mozilla0.9.2
x86
Linux
dataloss
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fix in hand, need approval,PDT+)

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
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

17 years ago
reassigning
Assignee: rods → pollmann

Comment 3

17 years ago
confirming
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 4

17 years ago
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.

Updated

17 years ago
Keywords: dataloss
Target Milestone: --- → mozilla0.9.2
(Assignee)

Comment 5

17 years ago
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...
Status: NEW → ASSIGNED

Comment 6

17 years ago
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

17 years ago
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.
(Assignee)

Comment 8

17 years ago
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...

(Assignee)

Comment 9

17 years ago
*** Bug 85577 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 10

17 years ago
See fix on bug 44464
Whiteboard: fix in hand
(Assignee)

Comment 11

17 years ago
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.
(Assignee)

Comment 12

17 years ago
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...
(Assignee)

Comment 13

17 years ago
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 ***
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → DUPLICATE

Comment 14

17 years ago
verifying dupe
Status: RESOLVED → VERIFIED
(Assignee)

Comment 15

17 years ago
*** Bug 83442 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 16

17 years ago
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.
Status: VERIFIED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 17

17 years ago
Created attachment 38891 [details] [diff] [review]
Fix corruption (turn off ContentTransferEncoding header by default)
(Assignee)

Comment 18

17 years ago
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.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 19

17 years ago
Created attachment 38892 [details] [diff] [review]
Fix full path bug (includes previous patch, addresses security concern and incompatabilities with previous Linux browsers)
(Assignee)

Comment 20

17 years ago
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).

(Assignee)

Comment 21

17 years ago
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!
Whiteboard: fix in hand → fix in hand, need r/sr/a

Comment 22

17 years ago
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.
(Assignee)

Comment 23

17 years ago
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

17 years ago
sr=mscott
(Assignee)

Comment 25

17 years ago
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=pollmann@netscape.com for code Adrian wrote and
r=havill@redhat.com for how I split it apart.
Whiteboard: fix in hand, need r/sr/a → fix in hand, need approval

Comment 26

17 years ago
a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)
Blocks: 83989
(Assignee)

Comment 27

17 years ago
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!




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

Comment 29

17 years ago
Marking PDT+
Whiteboard: fix in hand, need approval → fix in hand, need approval,PDT+
(Assignee)

Comment 30

17 years ago
Oops, forgot to mark this FIXED.  See my previous comment for verification
instructions.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 31

17 years ago
Verifying fixed on build 2001-06-21-04-trunk windows 98
and build 2001-06-20-21-trunk linux red hat 6.2
Status: RESOLVED → VERIFIED

Comment 32

17 years ago
*** Bug 93860 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.