576070 - (CVE-2010-3167) nsTreeContentView Dangling Pointer Remote Code Execution Vulnerability (ZDI-CAN-804)

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Reed Loden [:reed]]

Attachment: 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]]

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)]

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]]

I don't crash using the stated 3.6.3 either, but I tested Mac and not Windows.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(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][[e-mail]]]

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

[[e-mail]]

(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)]

Olli, I hope you can take this

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Sorry for the delay. I'll look into this later today or tomorrow.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase, run locally

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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

[[e-mail]]

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][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/a5abfcbf9fd3

Comment 13

[LegNeato]

Comment on attachment 461171 [details] [diff] [review]
patch

a=LegNeato for 1.9.2.9 and 1.9.1.12.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4c98dfa26307
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f738ede600d2

Comment 15

[Al Billings [:abillings - ex-MoCo]]

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.