241924 - Mozilla can upload files without user confirmation

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Met - Martin Hassman]

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]

Because this is seciruy bug and is not listed by default, adding some Mozilla staff.

Comment 2

[Met - Martin Hassman]

Attachment: Tescase

Simple testcase - loog at the input tag - Mozilla does not ignore its value.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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]

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

I'll have a look at this

Comment 6

[Met - Martin Hassman]

Attachment: Testcase2 - innerHTML

And finaly:

INPUT tag created by innerHTML has the same problem as document.write() - see
testcace 2

Comment 7

[Boris Zbarsky [:bzbarsky]]

This regressed between 2004-03-03-08 and 2004-03-04-08.  Bug 232706 seems very
very suspect.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Though the diff to nsHTMLInputElement looks ok....

Comment 9

[Boris Zbarsky [:bzbarsky]]

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]

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

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

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]

Attachment: this should do it

Comment 14

[timeless]

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

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]

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]

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]

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

-> sicking, his patch and check-in

Comment 20

[Daniel Veditz [:dveditz]]

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]

checked in

Comment 22

[Met - Martin Hassman]

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

Adding Jon Granrose to CC list to help round up QA resources for verification

Comment 24

[Daniel Veditz [:dveditz]]

Removing security-sensitive flag for bugs on the known-vulnerabilities list

Comment 25

[Mark Cox]

Note: The Common Vulnerabilities and Exposures project (cve.mitre.org) has
assigned the name CAN-2004-0759 to this issue.