334289 - nsXBLPrototypeHandler::ExecuteHandler uses preventDefault unitinitialized if aEvent fails to qi to nsIDOMNSUIEvent

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[timeless]

i'm filing this is as a security bug because i'm not sure if content code could coerce us to take this path.

i don't think anything bad will happen if we do happen to use it uninitialized, but i don't want to think about it either.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Should probably init to false or something...

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: init to false

Comment 3

[:Gavin Sharp [email: [e-mail]]]

mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp 	1.102

Comment 4

[Daniel Veditz [:dveditz]]

I don't see how this is a security problem. Please correct me if I'm wrong, but the worst case is we'll think preventDefault is set on a non-UI event that doesn't support it, bailing early and ignoring an event we should have processed. I'm much less worried about that than the other way, acting on events we should have ignored.

The code would have been clearer in the first place, and avoided this error, if it had been written

   if (isXBLCommand && !isReceiverCommandElement) {
     // See if preventDefault has been set.  If so, don't execute.
-    PRBool preventDefault;
     nsCOMPtr<nsIDOMNSUIEvent> nsUIEvent(do_QueryInterface(aEvent));
-    if (nsUIEvent)
+    if (nsUIEvent) {
+      PRBool preventDefault = PR_FALSE;
       nsUIEvent->GetPreventDefault(&preventDefault);
- 
       if (preventDefault)
         return NS_OK;
+    }
 
Please remove the '?' after 'nse' in the whiteboard (and the security group check) if you concur