Setting menulist.selectedItem to an integer crashes

VERIFIED FIXED

Status

()

P3
critical
VERIFIED FIXED
19 years ago
10 years ago

People

(Reporter: sfraser_bugs, Assigned: jst)

Tracking

({crash})

Trunk
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: security hole [nsbeta2+][HAVE FIX])

Attachments

(1 attachment)

(Reporter)

Description

19 years ago
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

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

Comment 2

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

Comment 3

19 years ago
kicking to jst, who owns the DOM glue. Mark nsbeta2 and note security hole in 
status. add vidur to cc list.
Assignee: waterson → jst
Keywords: nsbeta2
Whiteboard: security hole

Updated

19 years ago
Severity: normal → critical
(Assignee)

Comment 4

19 years ago
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.
Status: NEW → ASSIGNED
OS: Mac System 8.5 → All
(Assignee)

Comment 5

19 years ago
Created attachment 8878 [details] [diff] [review]
Propesed fix
(Assignee)

Comment 6

19 years ago
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

19 years ago
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

19 years ago
Adding crash keyword
Keywords: crash

Comment 9

19 years ago
This is needed for beta2, given the crash and security issue; giving PDT 
approval.
Whiteboard: security hole → security hole [nsbeta2+]
(Assignee)

Updated

19 years ago
Whiteboard: security hole [nsbeta2+] → security hole [nsbeta2+][HAVE FIX]
(Assignee)

Comment 10

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

Marking FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED

Comment 11

19 years ago
verified fixed mac/linux/win32 20000531nn comm. builds -- dom glue now bails 
out when passed an int for an object.
Status: RESOLVED → VERIFIED

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.