325947 - [FIX]Site can cause user's file to be uploaded by changing input type

Status: RESOLVED FIXED

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

Description

[Claus Jørgensen]

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]

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]]

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]

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) No longer reading bugmail consistently]

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 [:bzbarsky]]

Attachment: 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 [:bzbarsky]]

> 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 [:bzbarsky]]

Attachment: Fix

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 [:bzbarsky]]

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) No longer reading bugmail consistently]

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 [:bzbarsky]]

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) No longer reading bugmail consistently]

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 [:bzbarsky]]

> 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]]

Comment on attachment 210816 [details] [diff] [review]
Fix

sr=dveditz

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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]]

Comment on attachment 210816 [details] [diff] [review]
Fix

a=dveditz for 3 older branches

Comment 18

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Patch I actually checked in on the braches

Comment 20

[Jay Patel [:jay]]

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) No longer reading bugmail consistently]

An event is indeed fired :( See bug 325947.

Comment 22

[Jesse Ruderman]

That's this bug.  I think sicking meant "See bug 328566."