Last Comment Bug 241924 - Mozilla can upload files without user confirmation
: Mozilla can upload files without user confirmation
Status: RESOLVED FIXED
[sg:fix] vuln 1.7a/b, fixed 1.7rc1 ...
: fixed1.7
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-04-28 00:48 PDT by Met - Martin Hassman
Modified: 2004-08-03 00:46 PDT (History)
13 users (show)
chofmann: blocking1.7+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tescase (258 bytes, text/html)
2004-04-28 00:51 PDT, Met - Martin Hassman
no flags Details
Testcase2 - innerHTML (397 bytes, text/html)
2004-04-28 12:45 PDT, Met - Martin Hassman
no flags Details
this should do it (1.03 KB, patch)
2004-04-28 16:35 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.7+
Details | Diff | Splinter Review

Description Met - Martin Hassman 2004-04-28 00:48:19 PDT
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:
Comment 1 Met - Martin Hassman 2004-04-28 00:49:47 PDT
Because this is seciruy bug and is not listed by default, adding some Mozilla staff.
Comment 2 Met - Martin Hassman 2004-04-28 00:51:51 PDT
Created attachment 147219 [details]
Tescase

Simple testcase - loog at the input tag - Mozilla does not ignore its value.
Comment 3 Boris Zbarsky [:bz] 2004-04-28 07:15:03 PDT
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?
Comment 4 Met - Martin Hassman 2004-04-28 08:53:08 PDT
(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!
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-04-28 09:56:25 PDT
I'll have a look at this
Comment 6 Met - Martin Hassman 2004-04-28 12:45:47 PDT
Created attachment 147261 [details]
Testcase2 - innerHTML

And finaly:

INPUT tag created by innerHTML has the same problem as document.write() - see
testcace 2
Comment 7 Boris Zbarsky [:bz] 2004-04-28 13:51:00 PDT
This regressed between 2004-03-03-08 and 2004-03-04-08.  Bug 232706 seems very
very suspect.
Comment 8 Boris Zbarsky [:bz] 2004-04-28 13:58:56 PDT
Though the diff to nsHTMLInputElement looks ok....
Comment 9 Boris Zbarsky [:bz] 2004-04-28 15:58:15 PDT
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!
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-04-28 16:13:05 PDT
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 :)
Comment 11 Boris Zbarsky [:bz] 2004-04-28 16:19:44 PDT
> 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.
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-04-28 16:21:32 PDT
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.
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-04-28 16:35:56 PDT
Created attachment 147273 [details] [diff] [review]
this should do it
Comment 14 timeless 2004-04-28 16:56:18 PDT
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 15 Boris Zbarsky [:bz] 2004-04-28 17:35:54 PDT
Comment on attachment 147273 [details] [diff] [review]
this should do it

s/cought/caught/ and s/accidently/accidentally/, and r+sr=bzbarsky
Comment 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-04-28 17:56:58 PDT
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.
Comment 17 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-04-28 17:57:57 PDT
Comment on attachment 147273 [details] [diff] [review]
this should do it

checked in on trunk
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-04-28 17:59:16 PDT
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.
Comment 19 Daniel Veditz [:dveditz] 2004-04-29 13:03:27 PDT
-> sicking, his patch and check-in
Comment 20 Daniel Veditz [:dveditz] 2004-04-29 13:31:22 PDT
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
Comment 21 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-04-29 14:00:50 PDT
checked in
Comment 22 Met - Martin Hassman 2004-05-03 02:46:20 PDT
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.
Comment 23 Daniel Veditz [:dveditz] 2004-06-17 13:37:16 PDT
Adding Jon Granrose to CC list to help round up QA resources for verification
Comment 24 Daniel Veditz [:dveditz] 2004-07-22 02:34:37 PDT
Removing security-sensitive flag for bugs on the known-vulnerabilities list
Comment 25 Mark Cox 2004-08-03 00:46:47 PDT
Note: The Common Vulnerabilities and Exposures project (cve.mitre.org) has
assigned the name CAN-2004-0759 to this issue.

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