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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Layout: Form Controls
P1
critical
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Claus Jørgensen, Assigned: bz)

Tracking

({fixed1.7.13, fixed1.8.1, verified1.8.0.2})

Trunk
mozilla1.9alpha1
fixed1.7.13, fixed1.8.1, verified1.8.0.2
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.9a1 +
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high][rft-dl], URL)

Attachments

(3 attachments)

(Reporter)

Description

12 years ago
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
Component: Security → Layout: Form Controls
Product: Firefox → Core
QA Contact: firefox → layout.form-controls
Version: unspecified → 1.8 Branch
Flags: blocking1.9a1?
Flags: blocking1.8.0.2?
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?

Comment 1

12 years ago
Confirmed on trunk using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060131 Firefox/1.6a1.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Automatical upload of file from users harddisk, without approval → Site can cause user's file to be uploaded by changing input type
Whiteboard: [sg:high]
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.
Flags: blocking1.9a1?
Flags: blocking1.9a1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2+
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
(Reporter)

Comment 3

12 years ago
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 "".
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.
> 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.
Created attachment 210816 [details] [diff] [review]
Fix
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #210816 - Flags: superreview?(peterv)
Attachment #210816 - Flags: review?
Attachment #210816 - Flags: review? → review?(bugmail)
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: 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
Target Milestone: --- → mozilla1.9alpha
Version: 1.8 Branch → Trunk
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.
Attachment #210816 - Flags: review?(bugmail) → review+
> 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.
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
Comment on attachment 210816 [details] [diff] [review]
Fix

sr=dveditz
Attachment #210816 - Flags: superreview?(peterv)
Attachment #210816 - Flags: superreview+
Attachment #210816 - Flags: approval1.8.0.2?
Attachment #210816 - Flags: approval1.7.13?
Attachment #210816 - Flags: approval-aviary1.0.8?
Attachment #210816 - Flags: branch-1.8.1?(bugmail)
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Comment on attachment 210816 [details] [diff] [review]
Fix

Shouldn't jst really approve this since he is mo? If not, a=me
Attachment #210816 - Flags: branch-1.8.1?(bugmail) → branch-1.8.1+
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Comment on attachment 210816 [details] [diff] [review]
Fix

a=dveditz for 3 older branches
Attachment #210816 - Flags: approval1.8.0.2?
Attachment #210816 - Flags: approval1.8.0.2+
Attachment #210816 - Flags: approval1.7.13?
Attachment #210816 - Flags: approval1.7.13+
Attachment #210816 - Flags: approval-aviary1.0.8?
Attachment #210816 - Flags: approval-aviary1.0.8+
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
Keywords: fixed-aviary1.0.8, fixed1.7.13, fixed1.8.0.2, fixed1.8.1
Created attachment 211417 [details] [diff] [review]
Patch I actually checked in on the braches

Comment 20

11 years ago
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
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8

Updated

11 years ago
Whiteboard: [sg:high] → [sg:high][rft-dl]

Updated

11 years ago
Keywords: fixed1.8.0.2 → verified1.8.0.2
Depends on: 328566
An event is indeed fired :( See bug 325947.

Comment 22

11 years ago
That's this bug.  I think sicking meant "See bug 328566."
No longer depends on: 328566
Depends on: 334977
Group: security
You need to log in before you can comment on or make changes to this bug.