Closed
Bug 83065
Opened 23 years ago
Closed 23 years ago
multipart/form-data file upload with php corruption
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
VERIFIED
FIXED
mozilla0.9.2
People
(Reporter: mjauvin, Assigned: pollmann)
References
Details
(Keywords: dataloss, Whiteboard: fix in hand, need approval,PDT+)
Attachments
(2 files)
5.37 KB,
patch
|
Details | Diff | Splinter Review | |
12.96 KB,
patch
|
Details | Diff | Splinter Review |
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•23 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.
Reporter | ||
Comment 4•23 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.
Assignee | ||
Comment 5•23 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•23 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•23 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•23 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 11•23 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•23 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•23 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
Closed: 23 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Comment 15•23 years ago
|
||
*** Bug 83442 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 16•23 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•23 years ago
|
||
Assignee | ||
Comment 18•23 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•23 years ago
|
||
Assignee | ||
Comment 20•23 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•23 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•23 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•23 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•23 years ago
|
||
sr=mscott
Assignee | ||
Comment 25•23 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•23 years ago
|
||
a= asa@mozilla.org for checkin to the trunk. (on behalf of drivers)
Blocks: 83989
Assignee | ||
Comment 27•23 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!
Comment 28•23 years ago
|
||
*** Bug 85742 has been marked as a duplicate of this bug. ***
Comment 29•23 years ago
|
||
Marking PDT+
Whiteboard: fix in hand, need approval → fix in hand, need approval,PDT+
Assignee | ||
Comment 30•23 years ago
|
||
Oops, forgot to mark this FIXED. See my previous comment for verification instructions.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 31•23 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•23 years ago
|
||
*** Bug 93860 has been marked as a duplicate of this bug. ***
Updated•5 years ago
|
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•