Last Comment Bug 571106 - (CVE-2010-2753) nsTreeSelection Dangling Pointer Remote Code Execution Vulnerability (ZDI-CAN-755)
: nsTreeSelection Dangling Pointer Remote Code Execution Vulnerability (ZDI-CAN...
: fixed1.9.0.20, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: All All
-- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Neil Deakin
Depends on: CVE-2010-2760
  Show dependency treegraph
Reported: 2010-06-09 15:12 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2010-09-27 18:05 PDT (History)
15 users (show)
dveditz: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

simple version (953 bytes, patch)
2010-06-10 08:33 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
patch (982 bytes, patch)
2010-06-16 05:35 PDT, Olli Pettay [:smaug]
neil: review+
dveditz: approval1.9.2.7+
dveditz: approval1.9.1.11+
Details | Diff | Splinter Review

Description User image Reed Loden [:reed] (use needinfo?) 2010-06-09 15:12:54 PDT
Created attachment 450220 [details]
XUL testcase

ZDI-CAN-755: Mozilla Firefox nsTreeSelection Dangling Pointer Remote Code Execution Vulnerability

-- ABSTRACT ------------------------------------------------------------

TippingPoint has identified a vulnerability affecting the following 


    Mozilla Firefox 3.6.x

-- VULNERABILITY DETAILS -----------------------------------------------

This vulnerability allows remote attackers to execute arbitrary code on

vulnerable installations of Mozilla Firefox. User interaction is

required to exploit this vulnerability in that the target must visit a

malicious page or open a malicious file.

The specific flaw exists within the implementation of XUL <tree>

element's "selection" attribute. There is an integer overflow when

calculating the bounds of a new selection range. When calling

adjustSelection on this manged range both ranges are deleted leaving a

dangling reference. A remote attacker can exploit this vulnerability to

execute arbitrary code under the context of the browser.

View of XUL <tree> element exposes "selection" attribute. This in turn

allows user to precisely choose set of tree's rows to be shown as

selected. The way class nsTreeSelection

(layout/xul/base/src/tree/src/nsTreeSelection.cpp) is implemented is

quite interesting: with a linked list of nsTreeRange instances where

nsTreeRange represents single continuous range of selected rows (and

possibly a pointer to the next range). nsTreeSelection holds directly

only one pointer, to the first range, mFirstRange.

What is also important to note: whenever any nsTreeRange instance is

deleted, delete (inside destructor) is called recursively on the next

linked instances as well.

Imagine we have the following piece of Javascript code:

  [1]  sel = tree.treeBoxObject.view.selection;

  [2]  sel.rangedSelect(0, 0x7fffffff, true);

  [3]  sel.adjustSelection(1, 1);

At [1] we grab the reference to selection object. At this moment

mFirstRange is null, no row is selected, so there is no need for any

range to keep track of.

Then [2] we call method rangedSelect(). From



  PRInt32 start = aStartIndex < aEndIndex ? aStartIndex : aEndIndex;

  PRInt32 end = aStartIndex < aEndIndex ? aEndIndex : aStartIndex;


  nsTreeRange* range = new nsTreeRange(this, start, end);

  if (!range)



  if (aAugment && mFirstRange)



    mFirstRange = range;


That makes mFirstRange being set to range <0, 2G>, or more precisely

(yet in ad hoc pseudo code):

  mFirstRange = {

    min:  0,

    max:  2G,

    prev: null,

    next: null


And at [3] we hit integer overflow that causes a lot of trouble. From

nsTreeSelection::AdjustSelection(PRInt32 aIndex, PRInt32 aCount):


  nsTreeRange* newRange = nsnull;

  PRBool selChanged = PR_FALSE;

  nsTreeRange* curr = mFirstRange;

  while (curr) {

    if (aCount > 0) {

      // inserting

      if (aIndex > curr->mMax) {



      else if (aIndex <= curr->mMin) {



      else {

        // adjustment happen inside the range.

        // break apart the range and create two ranges

        ADD_NEW_RANGE(newRange, this, curr->mMin, aIndex - 1);

        ADD_NEW_RANGE(newRange, this, aIndex + aCount, curr->mMax +


        selChanged = PR_TRUE;



    else {



    curr = curr->mNext;


  delete mFirstRange;

  mFirstRange = newRange;


As we are calling AdjustSelection(1, 1), two ADD_NEW_RANGE() are


This means that something like <0, 0>.Insert(<2, -2G>) will be called.

Notice that because of integer overflow lower bound (2) is above higher

bound (-2G).

  void Insert(nsTreeRange* aRange) {

    if (mMin >= aRange->mMax)

      aRange->Connect(mPrev, this);

    else if (mNext)



      aRange->Connect(this, nsnull);


So, now <2, -2G>.Connect(mPrev = null, <0, 0>):

  void Connect(nsTreeRange* aPrev = nsnull, nsTreeRange* aNext = nsnull)


    if (aPrev)

      aPrev->mNext = this;


      mSelection->mFirstRange = this;

    if (aNext)

      aNext->mPrev = this;

    mPrev = aPrev;

    mNext = aNext;


Which results in:

  mFirstRange = {

    min:  2,

    max:  -2G,

    prev: null,

    next: {

      min:  0,

      max:  0,

      prev: mFirstRange,

      next: null



But we can not forget about the epilogue of AdjustSelection():

  delete mFirstRange;

  mFirstRange = newRange;

Final result: both ranges (<2, -2G>, <0, 0>) are deleted and mFirstRange

points at deleted range <0, 0>.

-- CREDIT --------------------------------------------------------------

This vulnerability was discovered by:

    * regenrecht
Comment 1 User image Reed Loden [:reed] (use needinfo?) 2010-06-09 15:16:37 PDT
Comment 2 User image Daniel Veditz [:dveditz] 2010-06-09 18:59:49 PDT
On Mac 3.5.11pre and 3.6.6pre it looks like a null-deref DoS
3.5: bp-2d7a4637-6ce5-47b0-867c-a41ca2100609
3.6: bp-c584be08-5186-402e-bee2-f6af42100609

I get a similar Mac trunk crash to Reed's Linux one above.

The code looks the same though, so I'd expect all versions to have the same issues as described in comment 0.
Comment 3 User image Olli Pettay [:smaug] 2010-06-10 08:33:02 PDT
Created attachment 450359 [details] [diff] [review]
simple version

Make sure nsTreeRange is created in a proper way.

Other cases when nsTreeRange is created manually without the macro are safe.
Comment 4 User image 2010-06-10 09:19:26 PDT
Comment on attachment 450359 [details] [diff] [review]
simple version

>+    PRInt32 start = macro_start; \
Not sure why you created a temporary for macro_start but still double-evaluated macro_end?

>+    PRInt32 end = start < macro_end ? macro_end : start; \
Nit: start > macro_end is the (hopefully unlikely) bug case; start = macro_end and start < macro_end are equally likely, so this hurts branch prediction.
Comment 5 User image Olli Pettay [:smaug] 2010-06-10 15:12:41 PDT
ok, I'll tweak the patch a bit.
Comment 6 User image Olli Pettay [:smaug] 2010-06-15 14:06:23 PDT
I'll update the patch tomorrow.
Comment 7 User image Olli Pettay [:smaug] 2010-06-16 05:35:01 PDT
Created attachment 451552 [details] [diff] [review]
Comment 8 User image Olli Pettay [:smaug] 2010-06-16 11:42:26 PDT
Comment 9 User image Daniel Veditz [:dveditz] 2010-06-23 10:43:51 PDT
Comment on attachment 451552 [details] [diff] [review]

Approved for and, a=dveditz for release-drivers
Comment 11 User image Al Billings [:abillings] 2010-07-01 17:16:32 PDT
Verified for using attached testcase and build 1 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20100701 Firefox/3.5.11 (.NET CLR 3.5.30729)). 

Verified for with its build 1 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20100701 Firefox/3.6.7 (.NET CLR 3.5.30729)).

Verified that and both crash with the testcase.
Comment 12 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-07-18 21:58:24 PDT
Comment on attachment 451552 [details] [diff] [review]

Requesting on this patch so that we can take it in upcoming Camino 2.0.x security and stability updates.  If approved, I'll handle the checkins, unless the patch author requests otherwise.
Comment 13 User image Daniel Veditz [:dveditz] 2010-07-22 19:18:14 PDT
Comment on attachment 451552 [details] [diff] [review]

Approved for, a=dveditz
Comment 14 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-07-24 15:29:29 PDT
Checking in layout/xul/base/src/tree/src/nsTreeSelection.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeSelection.cpp,v  <--  nsTreeSelection.cpp
new revision: 1.63; previous revision: 1.62
Comment 15 User image Daniel Veditz [:dveditz] 2010-08-09 02:08:53 PDT
We need an in-tree regression test for this.
Comment 16 User image Daniel Veditz [:dveditz] 2010-08-10 12:55:59 PDT
making private again while we fix bug 585815

Note You need to log in before you can comment on or make changes to this bug.