OS: Windows XP → All
Hardware: PC → All
Assignee: nobody → mats.palmgren
Status: UNCONFIRMED → ASSIGNED
Component: General → Event Handling
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → events
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.
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.
Attachment #338339 - Flags: review?(Olli.Pettay)
Attachment #338339 - Flags: review?(Olli.Pettay) → review+
Attachment #338339 - Flags: superreview?(bzbarsky)
Comment on attachment 338339 [details] [diff] [review] Patch rev. 1 sr=bzbarsky. Looks good.
Attachment #338339 - Flags: superreview?(bzbarsky) → superreview+
Mats, next time, can you please add a meaningful, self-contained commit message? The log should be rudimentarily readable without bugzilla available. Thanks.
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.
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.
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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)?
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.
Status: RESOLVED → VERIFIED
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.
Attachment #338339 - Flags: approval126.96.36.199?
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.
Whiteboard: [sg:critical?] → [sg:needinfo]
Comment on attachment 338339 [details] [diff] [review] Patch rev. 1 Approved for 188.8.131.52, a=dveditz
Attachment #338339 - Flags: approval184.108.40.206? → approval220.127.116.11+
(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.
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.
Fixed on CVS trunk for 18.104.22.168: 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
Verified for 22.214.171.124 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:126.96.36.199pre) Gecko/2008102304 GranParadiso/3.0.4pre.
crash test added http://hg.mozilla.org/mozilla-central/rev/51e405d991ce
Flags: in-testsuite? → in-testsuite+
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.