Bug 576070 (CVE-2010-3167)

nsTreeContentView Dangling Pointer Remote Code Execution Vulnerability (ZDI-CAN-804)

RESOLVED FIXED

Status

()

Core
XUL
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: reed, Assigned: smaug)

Tracking

({verified1.9.2})

unspecified
verified1.9.2
Points:
---

Firefox Tracking Flags

(blocking2.0 final+, status2.0 wanted, blocking1.9.2 .9+, status1.9.2 .9-fixed, blocking1.9.1 .12+, status1.9.1 .12-fixed)

Details

(Whiteboard: [sg:critical][critsmash:investigating] [qa-examined-191])

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.1: --- → ?
status1.9.2: --- → ?
status2.0: --- → ?
(Reporter)

Comment 1

7 years ago
Can't get the PoC to crash on either latest trunk or 1.9.2 nightlies...
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?
I don't crash using the stated 3.6.3 either, but I tested Mac and not Windows.
Whiteboard: [sg:critical?] → need to repro
(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.)
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

7 years ago
(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.)
Olli, I hope you can take this
Assignee: nobody → Olli.Pettay
Whiteboard: need to repro

Updated

7 years ago
Whiteboard: [sg:critical]
blocking1.9.1: ? → .12+
blocking1.9.2: ? → .8+
status1.9.1: ? → wanted
status1.9.2: ? → wanted
status2.0: ? → wanted

Updated

7 years ago
Whiteboard: [sg:critical] → [sg:critical][critsmash:investigating]
blocking2.0: ? → final+
Sorry for the delay. I'll look into this later today or tomorrow.
Created attachment 460826 [details]
testcase, run locally
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.
Attachment #461171 - Flags: review?(neil)
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?
Attachment #461171 - Flags: review?(neil) → review+
http://hg.mozilla.org/mozilla-central/rev/a5abfcbf9fd3
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Attachment #461171 - Flags: approval1.9.2.9?
Attachment #461171 - Flags: approval1.9.1.12?

Comment 13

7 years ago
Comment on attachment 461171 [details] [diff] [review]
patch

a=LegNeato for 1.9.2.9 and 1.9.1.12.
Attachment #461171 - Flags: approval1.9.2.9?
Attachment #461171 - Flags: approval1.9.2.9+
Attachment #461171 - Flags: approval1.9.1.12?
Attachment #461171 - Flags: approval1.9.1.12+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4c98dfa26307
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f738ede600d2
status1.9.1: wanted → .12-fixed
status1.9.2: wanted → .9-fixed
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.
Keywords: verified1.9.2
Whiteboard: [sg:critical][critsmash:investigating] → [sg:critical][critsmash:investigating] [qa-examined-191]
Alias: CVE-2010-3167
Group: core-security
You need to log in before you can comment on or make changes to this bug.