Closed Bug 241924 Opened 16 years ago Closed 16 years ago

Mozilla can upload files without user confirmation

Categories

(Core :: Security, defect)

x86
Windows 2000
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: sicking)

Details

(Keywords: fixed1.7, Whiteboard: [sg:fix] vuln 1.7a/b, fixed 1.7rc1 doesnotaffect1.4)

Attachments

(3 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; cs-CZ; rv:1.7) Gecko/20040425
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; cs-CZ; rv:1.7) Gecko/20040425

Description:
In the last Mozilla versions (the M1.7b and later - detail list see bellow) can
malicious JavaScript code upload files from the user computer without user
confirmation.

This is happening via classical <input type="file"> tag. The value of this tag
should be read only. If you give it some value="" in the HTML file, it should be
ignored. But unfortunately if you create input type="file" via document.write(),
its value is not cleared!!! e.g.: "document.write('<input name=\'u\' type="file"
value="c:\\autoexec.bat">');" can (after some onload="document.f.submit()")
upload "c:\autoexec.bat" to the server.

List of tested Mozilla versions (I have tested only Windows versions, sorry):
M1.6 not affected
Firefox 0.8 not affected
M1.7a not affected

M1.7b AFFECTED!
M1.7 last build AFFECTED! (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.7) Gecko/20040427)

See attachment for example (look at the value in input box).

If you want to see malicious code in action, look at the
http://www.penguin.cz/~met/pub/bug/form.html , but beware! During onload it will
automatically send your c:\autoexec.bat (sorry for Linux users 8-) to the server
and display it as the result in iframe bellow (yes, autoexec.bat is in the
Win2000/XP completely blank, so if you want to really test it, write some REM
comments in it).

(Ehm, sorry, but there is some problem on the penguin.cz server. Sometimes it
loads main www.penguin.cz page instead of my URL. So if you see blank page with
penguin logo, be patient and reload 8-)


Reproducible: Always
Steps to Reproduce:
Because this is seciruy bug and is not listed by default, adding some Mozilla staff.
Attached file Tescase
Simple testcase - loog at the input tag - Mozilla does not ignore its value.
Flags: blocking1.7?
I was sure we had a check for this... is the document.write necessary, or does
just setting the value attribute in HTML also work?
Flags: blocking1.7? → blocking1.7+
(In reply to comment #3)
> is the document.write necessary, or does
> just setting the value attribute in HTML also work?

Yes, it is. (I am demonstrating it on the the metioned page with "malicious code
in action").

In summary:

INPUT tag in HTML - OK
INPUT tag created by DOM createElement - OK
INPUT tag created by document.write() - VULNERABLE!
I'll have a look at this
Attached file Testcase2 - innerHTML
And finaly:

INPUT tag created by innerHTML has the same problem as document.write() - see
testcace 2
This regressed between 2004-03-03-08 and 2004-03-04-08.  Bug 232706 seems very
very suspect.
Though the diff to nsHTMLInputElement looks ok....
OK, if I change that SetValue() call in nsHTMLInputElement::ParseAttribute to
SetValueInternal() (which is what it should be to start with), then this bug
goes away, as far as I can tell.  And the bug is not present if the attribute
order is reversed -- that's handled by the implementation of Reset().

So clearly the problem is that when there is script on the callstack the call to
SetValue fails (which makes sense).  The part I don't get is why it worked before!
i just arrived at the same conclusion. The reason it worked before is that we
called SetValue *before* chaning mType. What i can't figure out is why there's a
difference using document.write or using normal markup :)
> What i can't figure out is why there's a difference using document.write or
> using normal markup :)

Easy.  In one case, there's JS on the stack, in the other there is not.  Then
the security manager does its security check, decides we were called from JS, and...

This exact issue came up in discussion recently, in fact -- that our DOM apis
are not reliable for internal use because they often perform security checks and
have to guess at who the "real caller" is.
aHA! Now i figured that part out too. And this is scary stuff.

The reason that just using markup doesn't work is that then when we call
SetValue we'll actually be allowed to set the value, even though it's a
file-input!! The reason is that we don't have any javascript contexts on the
stack so UniversalFileRead is *enabled*. Which is such a scary design, but as i
recall it it's really hard to change now.

I wonder if it's somehow possible to trigger this in older versions of mozilla too.

But the right fix is to call SetValueInternal, patch comming in a second.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #147273 - Flags: superreview?(bzbarsky)
Attachment #147273 - Flags: review?(bzbarsky)
Comment on attachment 147273 [details] [diff] [review]
this should do it

The word you've entered isn't in the dictionary. Click on a spelling suggestion
below or try again using the search box to the right.

Suggestions for cought:

	 1. caught
Comment on attachment 147273 [details] [diff] [review]
this should do it

s/cought/caught/ and s/accidently/accidentally/, and r+sr=bzbarsky
Attachment #147273 - Flags: superreview?(bzbarsky)
Attachment #147273 - Flags: superreview+
Attachment #147273 - Flags: review?(bzbarsky)
Attachment #147273 - Flags: review+
Comment on attachment 147273 [details] [diff] [review]
this should do it

This patch should be very safe since all it does is to bypass some
security-checks that we absolutly don't want to get cought in.
Attachment #147273 - Flags: approval1.7?
Comment on attachment 147273 [details] [diff] [review]
this should do it

checked in on trunk
I was thinking if there was any way to trigger the same bug in the way the code
was previously written, i.e. if we need to backport this patch to the 1.0 or 1.4
branches. However afaict the old way should be safe as well.
-> sicking, his patch and check-in
Assignee: security-bugs → bugmail
Comment on attachment 147273 [details] [diff] [review]
this should do it

a=dveditz for 1.7 branch. Please add the fixed1.7 keyword when checked in
Attachment #147273 - Flags: approval1.7? → approval1.7+
checked in
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: fixed1.7
Resolution: --- → FIXED
Tested with "Mozilla/5.0 (Windows; U; Windows NT 5.0; cs-CZ; rv:1.7)
Gecko/20040501 (from nightly/latest-1.7/)".

Both testcases document.write() and innerHTML seem OK.
Adding Jon Granrose to CC list to help round up QA resources for verification
Whiteboard: [sg:fix] vuln 1.7a/b, fixed 1.7rc1
Whiteboard: [sg:fix] vuln 1.7a/b, fixed 1.7rc1 → [sg:fix] vuln 1.7a/b, fixed 1.7rc1 doesnotaffect1.4
Removing security-sensitive flag for bugs on the known-vulnerabilities list
Group: security
Note: The Common Vulnerabilities and Exposures project (cve.mitre.org) has
assigned the name CAN-2004-0759 to this issue.
You need to log in before you can comment on or make changes to this bug.