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

Confirmed on trunk using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060131 Firefox/1.6a1.

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.

its 1.5.0.1 only (i forgot to write that) , in 1.5.0.0 the browser would crash on the try.

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 "".

With this testcase I see the problem in 1.7-era builds too, as well as current trunk and 1.8.0 branch.

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

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.

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.

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

> 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 on attachment 210816 [details] [diff] [review] Fix sr=dveditz

Fixed on trunk.

Comment on attachment 210816 [details] [diff] [review] Fix Shouldn't jst really approve this since he is mo? If not, a=me

Comment on attachment 210816 [details] [diff] [review] Fix a=dveditz for 3 older branches

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

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

An event is indeed fired :( See bug 325947.

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