Last Comment Bug 325947 - [FIX]Site can cause user's file to be uploaded by changing input type
: [FIX]Site can cause user's file to be uploaded by changing input type
Status: RESOLVED FIXED
[sg:high][rft-dl]
: fixed1.7.13, fixed1.8.1, verified1.8.0.2
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
http://www.vup.dk/test/changetype.php
Depends on: 334977
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-04 20:43 PST by Claus Jørgensen
Modified: 2006-06-03 00:22 PDT (History)
8 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
dveditz: blocking1.9a1+
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (824 bytes, text/html)
2006-02-05 15:45 PST, Boris Zbarsky [:bz]
no flags Details
Fix (2.13 KB, patch)
2006-02-05 18:34 PST, Boris Zbarsky [:bz]
jonas: review+
dveditz: superreview+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
jonas: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review
Patch I actually checked in on the braches (1.92 KB, patch)
2006-02-10 13:09 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review

Description Claus Jørgensen 2006-02-04 20:43:41 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; da; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; da; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

If you make a file called 1.txt and place it on c:\ on a windows machine, and then try the URL (http://www.vup.dk/test/changetype.php) the file will be uploaded, without your approval.

This is indeed a high risc security issue. If you don't want to enter the url, the JS code is this:

function changeType(obj) {
obj.type="text";
obj.value="C:\\1.txt";
obj.type="file";
setTimeout("videre()", 1);
}
function videre() {
	document.getElementById("submit").click();
}

Reproducible: Always

Steps to Reproduce:
1. Make a page, and run the javascript, and make, as an example php show the output

Actual Results:  
File gets uploaded, and content is printed

Expected Results:  
Denied the change of the input to .file
Comment 1 Jesse Ruderman 2006-02-04 21:20:20 PST
Confirmed on trunk using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060131 Firefox/1.6a1.
Comment 2 Daniel Veditz [:dveditz] 2006-02-04 21:35:22 PST
Confirming in Firefox 1.5.0.1, does not appear to work in 1.0.7 -- file upload controls back to haunt us.

I can't at the moment find the bug, but I know we've fixed this case at least once before.
Comment 3 Claus Jørgensen 2006-02-04 23:13:44 PST
its 1.5.0.1 only (i forgot to write that) , in 1.5.0.0 the browser would crash on the try.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2006-02-05 02:50:10 PST
Havn't had a chance to look at what's going on here. Btw, the expected behaviour here is not actually to deny setting the type to file, but that we clear the value when that is done.

Looking at this code things are currently way too complex. At least for file controls we should not meddle with sometimes keeping the value in the frame and sometimes in the node. Instead we should always keep the value in the node, that makes it easy to just set mValue to nsnull whenever the type is changed.

Right now we actually change the type before we set the value to "". That sort of scares be because if we fire an event, or some other way allow scripts to execute, at this point it could submit the file before we get a chance to set the value to "".
Comment 5 Boris Zbarsky [:bz] 2006-02-05 15:45:23 PST
Created attachment 210807 [details]
Testcase

With this testcase I see the problem in 1.7-era builds too, as well as current trunk and 1.8.0 branch.
Comment 6 Boris Zbarsky [:bz] 2006-02-05 18:32:54 PST
> Instead we should always keep the value in the node

We can do that on trunk, but not on branches where the user can edit the value in the textfield.

> Right now we actually change the type before we set the value to ""

And this is the crux of the problem, actually.  Patch coming up that fixes this bug (or at least my testcase).  The test site doesn't work for me, of course (no "C:\" anything here), so I'd appreciate if someone who can reproduce with that would test too.
Comment 7 Boris Zbarsky [:bz] 2006-02-05 18:34:13 PST
Created attachment 210816 [details] [diff] [review]
Fix
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2006-02-06 12:44:23 PST
Comment on attachment 210816 [details] [diff] [review]
Fix

We need to do more then this. We're currently playing whack-a-mole with the various ways this code breaks time and again.

One solution is to simply not share the value member between file inputs and other inputs. Why not add an |nsString* mFileName| member. And then let the filecontrol frame call a new method on nsITextControlElement to set it.

We'd never need to change mValue or mFileName when the type is chagning and this bug should never reel its ugly head again.
Comment 9 Boris Zbarsky [:bz] 2006-02-06 13:16:08 PST
We can't change nsITextControlElement on the 1.7 or 1.8 branches.  For 1.9, I agree that we should simplify all this stuff, but I really don't see a decent way of doing that on the branches short of creating new interfaces, etc.

If you really think that's the right approach for the stable branches, please let me know and I'll see what I can do.
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2006-02-06 14:07:43 PST
For 1.8.0.x i agree we'll have to go with something like the patch you attached, but i don't see a reason not go for the better solution on the 1.8.1 branch.

I'll start reviewing this with 1.8.0.x in mind.
Comment 11 Boris Zbarsky [:bz] 2006-02-06 14:11:26 PST
1.8.0.x and 1.7.x... The testcase I attached works fine with a 1.7 branch build over here.

For 1.8.1 we might be able to do better per comment 8, I guess.  For 1.9 we might be able to eliminate all storage in the file control frame also.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2006-02-06 14:22:13 PST
Comment on attachment 210816 [details] [diff] [review]
Fix

So is the issue that setting the type first makes us think the frame owns the value and so we set the empty value in a dying frame?


If so, r=me.


Setting the type after the value is sort of scary too I just realized. If scripts were to execute between setting the value and setting the type, it would be able to set the value back.

However, that should not be possible looking at the current code, so we should be safe.
Comment 13 Boris Zbarsky [:bz] 2006-02-06 14:25:20 PST
> So is the issue that setting the type first makes us think the frame owns the
> value and so we set the empty value in a dying frame?

Yes. More precisely, we set it in a frame that completely ignores the attempt to set the value in it -- it doesn't implement SetFormProperty, and the superclass impl is a no-op.

> However, that should not be possible looking at the current code, so we should
> be safe.

Exactly.
Comment 14 Daniel Veditz [:dveditz] 2006-02-09 05:36:04 PST
Comment on attachment 210816 [details] [diff] [review]
Fix

sr=dveditz
Comment 15 Boris Zbarsky [:bz] 2006-02-09 09:33:49 PST
Fixed on trunk.
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2006-02-09 13:37:19 PST
Comment on attachment 210816 [details] [diff] [review]
Fix

Shouldn't jst really approve this since he is mo? If not, a=me
Comment 17 Daniel Veditz [:dveditz] 2006-02-09 14:02:10 PST
Comment on attachment 210816 [details] [diff] [review]
Fix

a=dveditz for 3 older branches
Comment 18 Boris Zbarsky [:bz] 2006-02-10 13:08:25 PST
Sicking, I think 1.8.1 approval by peer's fine unless the peer thinks for sure the module owner should look.  And then he should reset the request.  ;)

*** Committing to MOZILLA_1_7_BRANCH... 
new revision: 1.336.2.9; previous revision: 1.336.2.8

*** Committing content/html/content/src/nsHTMLInputElement.cpp on new revision: 1.390.2.6.2.1; previous revision: 1.390.2.6

*** Committing content/html/content/src/nsHTMLInputElement.cpp on AVIARY_1_0_1_20050124_BRANCH
new revision: 1.336.2.1.2.5.2.2; previous revision: 1.336.2.1.2.5.2.1

*** Committing content/html/content/src/nsHTMLInputElement.cpp on new revision: 1.390.2.7; previous revision: 1.390.2.6
Comment 19 Boris Zbarsky [:bz] 2006-02-10 13:09:37 PST
Created attachment 211417 [details] [diff] [review]
Patch I actually checked in on the braches
Comment 20 Jay Patel [:jay] 2006-02-15 15:28:51 PST
v.fixed on 1.0.1 Aviary branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060215 Firefox/1.0.8
Comment 21 Jonas Sicking (:sicking) PTO Until July 5th 2006-02-25 13:29:51 PST
An event is indeed fired :( See bug 325947.
Comment 22 Jesse Ruderman 2006-02-26 00:22:37 PST
That's this bug.  I think sicking meant "See bug 328566."

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