Closed Bug 576070 (CVE-2010-3167) Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: XUL, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
status2.0 --- wanted
blocking1.9.2 --- .9+
status1.9.2 --- .9-fixed
blocking1.9.1 --- .12+
status1.9.1 --- .12-fixed

People

(Reporter: reed, Assigned: smaug)

Details

(Keywords: verified1.9.2, Whiteboard: [sg:critical][critsmash:investigating] [qa-examined-191])

Attachments

(3 files)

Attached file 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
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.1: --- → ?
status1.9.2: --- → ?
status2.0: --- → ?
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.
(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
Whiteboard: [sg:critical]
blocking1.9.1: ? → .12+
blocking1.9.2: ? → .8+
Whiteboard: [sg:critical] → [sg:critical][critsmash:investigating]
blocking2.0: ? → final+
Sorry for the delay. I'll look into this later today or tomorrow.
Attached patch patchSplinter Review
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
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #461171 - Flags: approval1.9.2.9?
Attachment #461171 - Flags: approval1.9.1.12?
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+
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.