Closed Bug 334289 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: XBL, defect)

1.8 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta1

People

(Reporter: timeless, Assigned: Gavin)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, fixed1.8.1, Whiteboard: [sg:nse])

Attachments

(1 file)

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.
Should probably init to false or something...
Keywords: helpwanted
Attached patch init to falseSplinter Review
Assignee: general → gavin.sharp
Status: NEW → ASSIGNED
Attachment #218635 - Flags: superreview?(bzbarsky)
Attachment #218635 - Flags: review?(bzbarsky)
Keywords: helpwanted
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Attachment #218635 - Flags: superreview?(bzbarsky)
Attachment #218635 - Flags: superreview+
Attachment #218635 - Flags: review?(bzbarsky)
Attachment #218635 - Flags: review+
mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp 	1.102
No longer blocks: 260194
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #218635 - Flags: approval-branch-1.8.1?(bzbarsky)
Attachment #218635 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
Whiteboard: [checkin needed (1.8 branch)]
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]
Target Milestone: mozilla1.9alpha → mozilla1.8.1beta1
Version: Trunk → 1.8 Branch
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
Whiteboard: [sg:nse?]
Flags: blocking1.8.0.8?
Group: security
Flags: blocking1.8.0.8? → blocking1.8.0.8-
Whiteboard: [sg:nse?] → [sg:nse]
You need to log in before you can comment on or make changes to this bug.