454820 - crash when keypress UIEvent is triggered using dispatchEvent

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Brian Huisman]

User-Agent:       Opera/9.52 (Windows NT 5.1; U; en)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

Firefox crashes when attempting to dispatch a keypress event created using the UIEvents type.  The following code triggers the crash:

<p>PASS if Firefox does not crash.</p>
<script type="text/javascript">
var evObj = document.createEvent('UIEvents');
evObj.initUIEvent( 'keypress', true, true, window, 1 );
document.getElementsByTagName('p')[0].dispatchEvent(evObj);
</script>

Reproducible: Always

Steps to Reproduce:
1. Create a DOM event using the UIEvents category
2. Define the event as a keypress event
3. Dispatch the event
4. Hilarity ensues
Actual Results:  
Firefox crashes

Expected Results:  
Firefox should not crash

Thanks to TarquinWJ for distilling the HTML testcase.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Reporter's testcase

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

So nsContentUtils::GetAccelKeyCandidates doesn't check that the event is really
a keyevent. That is the security problem. Regression from bug 359638.

XBL doesn't null check, that may lead to crashes too.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Null-check the result of the QI to nsIDOMKeyEvent.

Since nsContentUtils::GetAccelKeyCandidates() and some protected methods
in nsXBLWindowKeyHandler is supposed to be called with nsIDOMKeyEvent only
I changed their signatures to reflect that.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 338339 [details] [diff] [review]
Patch rev. 1

sr=bzbarsky.  Looks good.

Comment 5

[Ben Bucksch (:BenB)]

Mats, next time, can you please add a meaningful, self-contained commit message? The log should be rudimentarily readable without bugzilla available. Thanks.

Comment 6

[Ben Bucksch (:BenB)]

http://hg.mozilla.org/mozilla-central/rev/5088957eb541

Comment 7

[Mats Palmgren (inactive)]

Ben, I intentionally left out details about the bug since it's marked
security sensitive.  A also did not land the crash test for the same
reason (I intend to land it when the bug is made public).  I think this
is "standard procedure" for SS bugs, please correct me if I'm wrong.

Comment 8

[Ben Bucksch (:BenB)]

Sorry, I didn't notice it's a security bug.
It's a pity to have the log messed up, it would be particularly important for security bugs. But yes, standard procedure so far.

Comment 9

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/5088957eb541

I'll land the crash test when the bug is public.

I can't reproduce the crash in 1.8 branch builds on Linux.

-> FIXED

Comment 11

[Daniel Veditz [:dveditz]]

The reporter of dupe bug 457543 has posted it to milw0rm
http://www.milw0rm.com/exploits/6614

Does this patch work for the 1.9.0 branch (cvs head)?

Comment 13

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080929033431 Minefield/3.1b1pre.   attached testcase does not crash.

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 338339 [details] [diff] [review]
Patch rev. 1

(In reply to comment #11)
> Does this patch work for the 1.9.0 branch (cvs head)?

Yes, this patch works and is complete also for 1.9.0 branch.

Comment 15

[Daniel Veditz [:dveditz]]

Is this really potentially exploitable? We were blindly casting the wrong type of event to nsKeyEvent, but there are no virtual function calls to launch you into space and the code in question only reads incorrect data and doesn't write.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 338339 [details] [diff] [review]
Patch rev. 1

Approved for 1.9.0.4, a=dveditz

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #15)
> Is this really potentially exploitable? We were blindly casting the wrong type
> of event to nsKeyEvent, but there are no virtual function calls
Ah, indeed. No virtual calls, only trying to use wrong nsXXXEvent struct.

Comment 18

[Mats Palmgren (inactive)]

I concur with your analysis.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsContentUtils.cpp&rev=1.309&mark=4064-4066,4143,4145#4053
In case there is no native event we will do a QI and null-deref
crash on line 4145.
If there is a native event we access it with:

nativeKeyEvent->charCode
nativeKeyEvent->isShift
nativeKeyEvent->alternativeCharCodes.Length()
nativeKeyEvent->alternativeCharCodes[i].mUnshiftedCharCode
nativeKeyEvent->alternativeCharCodes[i].mShiftedCharCode

where 'alternativeCharCodes' is a nsTArray<nsAlternativeCharCode>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/public/nsGUIEvent.h&rev=3.162&root=/cvsroot&mark=737,712#712
and nsTArray does not have a vtable so the accesses above are "just"
out-of-bounds memory reads.

Comment 19

[Mats Palmgren (inactive)]

Fixed on CVS trunk for 1.9.0.4:
mozilla/content/base/public/nsContentUtils.h 	1.178
mozilla/content/base/src/nsContentUtils.cpp 	1.310
mozilla/content/xbl/src/nsXBLEventHandler.cpp 	1.75
mozilla/content/xbl/src/nsXBLWindowKeyHandler.cpp 	1.48
mozilla/content/xbl/src/nsXBLWindowKeyHandler.h 	1.16

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.4 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4pre) Gecko/2008102304 GranParadiso/3.0.4pre.

Comment 22

[Bob Clary [:bc] (inactive)]

crash test added
http://hg.mozilla.org/mozilla-central/rev/51e405d991ce