39693 - Setting menulist.selectedItem to an integer crashes

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

[Simon Fraser [no longer active]]

This JS crashes in nsXULMenuListElement::SetSelectedItem(), because I'm 
mistakenly setting the selected item to an integer, not a DOM element.

    var menuPopup = document.getElementById("ParagraphPopup");
    var menuItems = menuPopup.childNodes;
    for (i=0; i < menuItems.length; i++)
    {
      var menuItem = menuItems.item(i);
      if (menuItem.data == state)
      {
        menuList.selectedItem = i;			// BOOM!
        break;
      }
    }
  }

Comment 1

[Chris Waterson]

Wow. The DOM glue lets integers through as pointers? That seems bad. brendan, 
jst: comments?

Comment 2

[Chris Waterson]

cc'ing mstoltz as this would be a pretty significant security hole...

Comment 3

[Chris Waterson]

kicking to jst, who owns the DOM glue. Mark nsbeta2 and note security hole in 
status. add vidur to cc list.

Comment 4

[Johnny Stenback (:jst)]

The problem is in IDLC, the code it generates looks something like this:

          nsIDOMElement* prop;
          if (PR_FALSE == nsJSUtils::nsConvertJSValToObject(...)) {
            rv = NS_ERROR_DOM_NOT_OBJECT_ERR;
          }
      
          rv = a->SetSelectedItem(prop);
          NS_IF_RELEASE(prop);

It does realize that the value it gets is not an object, and it sets rv to
NS_ERROR_DOM_NOT_OBJECT_ERR, but it still gladly contiunes on and calls
a->SetSelectedIndex()! Fantastic!

I have a patch that fixes the immediate problems found here and also some other
similar cases that were obvious from looking at the IDLC source. The patch adds
a "break;" after setting the return value, I haven't done any serious tests
with the patch but from looking at the generated code it does the right thing.

Comment 5

[Johnny Stenback (:jst)]

Attachment: Propesed fix

Comment 6

[Johnny Stenback (:jst)]

If someone would care to review the attached patch, and if this bug would be
marked nsbeta2+ I could regenerate all the glue code in mozilla and check that
in along with this (after some more testing). (or should I regenerate all the
gluecode and attach a diff for all the gluecode for review?)

Comment 7

[Chris Waterson]

I would've put the other code in an "else" block rather than using a "break", 
but otherwise, this seems like the right thing to do.

Comment 8

[leger]

Adding crash keyword

Comment 9

[rickg]

This is needed for beta2, given the crash and security issue; giving PDT 
approval.

Comment 10

[Johnny Stenback (:jst)]

This is now fixed, IDLC is updated and all the gluecode is regenerated and
checked in.

Marking FIXED.

Comment 11

[John Morrison]

verified fixed mac/linux/win32 20000531nn comm. builds -- dom glue now bails 
out when passed an int for an object.