Last Comment Bug 576070 - (CVE-2010-3167) nsTreeContentView Dangling Pointer Remote Code Execution Vulnerability (ZDI-CAN-804)
(CVE-2010-3167)
: nsTreeContentView Dangling Pointer Remote Code Execution Vulnerability (ZDI-C...
Status: RESOLVED FIXED
[sg:critical][critsmash:investigating...
: verified1.9.2
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: All All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
: Neil Deakin
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-30 13:13 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2010-09-27 18:31 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
wanted
.9+
.9-fixed
.12+
.12-fixed


Attachments
PoC (789 bytes, application/vnd.mozilla.xul+xml)
2010-06-30 13:13 PDT, Reed Loden [:reed] (use needinfo?)
no flags Details
testcase, run locally (1.04 KB, application/vnd.mozilla.xul+xml)
2010-07-28 03:25 PDT, Olli Pettay [:smaug]
no flags Details
patch (7.87 KB, patch)
2010-07-29 03:17 PDT, Olli Pettay [:smaug]
neil: review+
christian: approval1.9.2.9+
christian: approval1.9.1.12+
Details | Diff | Splinter Review

Description Reed Loden [:reed] (use needinfo?) 2010-06-30 13:13:55 PDT
Created attachment 455232 [details]
PoC

ZDI-CAN-804: Mozilla Firefox nsTreeContentView Dangling Pointer Remote Code Execution Vulnerability

-- CVSS ----------------------------------------------------------------
10, (AV:N/AC:L/Au:N/C:C/I:C/A:C)

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

TippingPoint has identified a vulnerability affecting the following 
products:

    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 a particular
element within the XUL namespace. Due to a method for the element having
the side effect of executing javascript, an attacker can provide their
own javascript code which can be used to remove an object out from
underneath the element's child hierarchy. This can force the application
to make an invalid reference when traversing it's internal objects, thus
using an illegitimate pointer. This can be leveraged by an attacker to
execute arbitrary code under the context of the application.

Version(s)  tested: 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3)
Gecko/20100401 Firefox/3.6.3

One of the arguments to nsTreeContentView::GetCell is an nsITreeColumn
interface. This gives the function the side effect of being able to
execute a method of this object that is provided by the site. In this
code, the first point of the variable that can be controlled is located
within ChildIterator::Init.

layout/xul/base/src/tree/src/nsTreeContentView.cpp:1531
nsIContent*
nsTreeContentView::GetCell(nsIContent* aContainer, nsITreeColumn* aCol)
{
  nsCOMPtr<nsIAtom> colAtom;
  PRInt32 colIndex;
  aCol->GetAtom(getter_AddRefs(colAtom));
  aCol->GetIndex(&colIndex);    // XXX

  // Traverse through cells, try to find the cell by "ref" attribute or
by cell
  // index in a row. "ref" attribute has higher priority.
  nsIContent* result = nsnull;
  PRInt32 j = 0;
  ChildIterator iter, last;
  for (ChildIterator::Init(aContainer, &iter, &last); iter != last;
++iter) {
    nsIContent* cell = *iter;

    if (cell->Tag() == nsGkAtoms::treecell) {
      if (colAtom && cell->AttrValueIs(kNameSpaceID_None,
nsGkAtoms::ref,
                                       colAtom, eCaseMatters)) {
        result = cell;
        break;
      }
      else if (j == colIndex) {
        result = cell;
      }
      j++;
    }
  }

  return result;
}

An example of a method that can be used to exploit the side effect in
the previously function can be located within ::GetCellValue. If an
attacker destroys the row when GetCell is called, then
ChildIterator::Init will access it as the unsafe type.

layout/xul/base/src/tree/src/nsTreeContentView.cpp:471
NS_IMETHODIMP
nsTreeContentView::GetCellValue(PRInt32 aRow, nsITreeColumn* aCol,
nsAString& _retval)
{
  _retval.Truncate();
  NS_ENSURE_ARG_POINTER(aCol);
  NS_PRECONDITION(aRow >= 0 && aRow < PRInt32(mRows.Length()), "bad
row");
  if (aRow < 0 || aRow >= PRInt32(mRows.Length()))
    return NS_ERROR_INVALID_ARG;   

  Row* row = mRows[aRow];

  nsIContent* realRow =
    nsTreeUtils::GetImmediateChild(row->mContent, nsGkAtoms::treerow);
  if (realRow) {
    nsIContent* cell = GetCell(realRow, aCol);  // XXX
    if (cell)
      cell->GetAttr(kNameSpaceID_None, nsGkAtoms::value, _retval);
  }

  return NS_OK;
}

At this point, aContent has been destroyed.

layout/base/nsChildIterator.cpp:48
nsresult
ChildIterator::Init(nsIContent*    aContent,
                    ChildIterator* aFirst,
                    ChildIterator* aLast)
{
  // Initialize out parameters to be equal, in case of failure.
  aFirst->mContent = aLast->mContent = nsnull;
  aFirst->mIndex   = aLast->mIndex   = 0;
  
  NS_PRECONDITION(aContent != nsnull, "no content");
  if (! aContent)
    return NS_ERROR_NULL_POINTER;

  nsIDocument* doc = aContent->GetOwnerDoc();
  ...


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

This vulnerability was discovered by:
    * regenrecht
Comment 1 Reed Loden [:reed] (use needinfo?) 2010-06-30 14:04:40 PDT
Can't get the PoC to crash on either latest trunk or 1.9.2 nightlies...
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-30 16:50:23 PDT
We need to prevent content scripts (or maybe all scripts) from being able to provide their own implementation of nsITreeColumn. jst, can you remind me how to do that?
Comment 3 Daniel Veditz [:dveditz] 2010-07-01 16:44:11 PDT
I don't crash using the stated 3.6.3 either, but I tested Mac and not Windows.
Comment 4 Olli Pettay [:smaug] 2010-07-13 04:06:17 PDT
(In reply to comment #2)
> We need to prevent content scripts (or maybe all scripts) from being able to
> provide their own implementation of nsITreeColumn. 
Why?
Looks like nsTreeContentView::GetCellValue needs to just use the normal
com-rules and keep the parameters it gives to GetCell alive.

(I haven't managed to reproduce this yet.)
Comment 5 Olli Pettay [:smaug] 2010-07-13 04:09:16 PDT
Though, I agree, allowing only binary treecolumns would be better and safer.

We could add non-scriptable nsPITreeColumn interface and required
treecolumns to implement that.
Comment 6 neil@parkwaycc.co.uk 2010-07-13 04:20:50 PDT
(In reply to comment #5)
> We could add non-scriptable nsPITreeColumn interface and required
> treecolumns to implement that.
We already have this, via GetColumnImpl. (Which is also a perf win.)
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-07-13 13:15:47 PDT
Olli, I hope you can take this
Comment 8 Olli Pettay [:smaug] 2010-07-27 08:57:49 PDT
Sorry for the delay. I'll look into this later today or tomorrow.
Comment 9 Olli Pettay [:smaug] 2010-07-28 03:25:55 PDT
Created attachment 460826 [details]
testcase, run locally
Comment 10 Olli Pettay [:smaug] 2010-07-29 03:17:54 PDT
Created attachment 461171 [details] [diff] [review]
patch

I decided to keep the method in nsTreeBodyFrame since that is where it is 
really used. nsTreeContentView uses it just for nativeness check.
Comment 11 neil@parkwaycc.co.uk 2010-07-29 06:51:22 PDT
Comment on attachment 461171 [details] [diff] [review]
patch

>+public:
>+  static
...
>+protected:
File a bug on moving this? (Or replacing it with do_QueryObject?)

>+#define NS_ENSURE_NATIVE_COLUMN(_col)                                \
>+  nsRefPtr<nsTreeColumn> col = nsTreeBodyFrame::GetColumnImpl(_col); \
>+  if (!col) {                                                        \
>+    return NS_ERROR_INVALID_ARG;                                     \
>+  }
It would be neat if you could actually use the nsTreeColumn* object (it's not clear that this macro creates a local variable), since we can then in theory call the non-xpcom methods such as GetAtom() [in a followup bug of course].

> NS_IMETHODIMP
> nsTreeContentView::IsSelectable(PRInt32 aRow, nsITreeColumn* aCol, PRBool* _retval)
> {
>+  NS_ENSURE_NATIVE_COLUMN(aCol);
Whoa, how have we managed to go this long without a content-facing null check?
Comment 12 Olli Pettay [:smaug] 2010-07-31 12:13:05 PDT
http://hg.mozilla.org/mozilla-central/rev/a5abfcbf9fd3
Comment 13 christian 2010-08-03 10:36:27 PDT
Comment on attachment 461171 [details] [diff] [review]
patch

a=LegNeato for 1.9.2.9 and 1.9.1.12.
Comment 15 Al Billings [:abillings] 2010-08-17 15:50:07 PDT
Verified for 1.9.2 using the attached testcase and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9pre) Gecko/20100817 Namoroka/3.6.9pre ( .NET CLR 3.5.30729). In 1.9.2.8, the testcase causes a crash.

For 1.9.1.11, I cannot get a crash with the attached testcase. Are we sure that this is exploitable for 1.9.1? Neither the testcase or original PoC seem to work there.

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