538308 - (CVE-2010-0176) nsTreeContentView Dangling Pointer Vulnerability (ZDI-CAN-633)

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: XUL testcase

ZDI-CAN-633: Mozilla Firefox nsTreeContentView Dangling Pointer Vulnerability

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

TippingPoint has identified a vulnerability affecting the following 
products:

    Mozilla Firefox 3.5.x

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

This vulnerability allows remote attackers to execute arbitrary code on
vulnerable installations of Mozilla Firefox. User interaction is
required in that the victim must visit a malicious website or be coerced
into opening a malicious document.

The specific flaw exists within the way that Mozilla's Firefox parses
.XUL files. While appending a particular tag to a treechildren
container, the application will create more than one reference to a
particular element without increasing its reference count. Upon removal
of one of the elements, the refcount will be decreased causing the
application to free the memory associated with the object. Due to the
rogue reference occurring, the next time the application attempts to
reference that container, the application will access memory that has
been freed which can lead to code execution under the context of the
application.

Version(s)  tested: Mozilla Firefox 3.5.6
Platform(s) tested: Windows XP SP3

The vulnerability occurs due to the application incorrectly creating
more than one "option" element inside an "optgroup" element within a
"treechildren" container.

The XML used to describe this vulnerability is shown below.

    <tree id="tr" flex="1">
        <treecols>
            <treecol/>
        </treecols>
        <treechildren>
            <html:optgroup id="optgroup">
                <html:option id="disappear" />
            </html:optgroup>
        </treechildren>
    </tree>

To reproduce the issue, one will need to manipulate this document via
javascript.

optgroup = document.getElementById("optgroup");
treechildren = document.createElement("treechildren");
optgroup.appendChild(tc);

v = document.getElementById("disappear");
v.parentNode.removeChild(v);
v = null;

tree = document.getElementById("tr");
col = tree.columns[0];
alert(tree.view.getItemAtIndex(1, col)); 

When appending an element to an XUL tree, the application will call the
following code.

layout/xul/base/src/tree/src/nsTreeContentView.cpp:966
void
nsTreeContentView::ContentInserted(nsIDocument *aDocument,
                                   nsIContent* aContainer,
                                   nsIContent* aChild,
                                   PRInt32 aIndexInContainer)
{

Upon an optgroup being appended to a tree, the following code will add a
row for the optgroup.

layout/xul/base/src/tree/src/nsTreecontentView.cpp:1038
  else if (childTag == nsGkAtoms::optgroup) {
    InsertRowFor(aContainer, aChild);       // XXX
  }
  else if (childTag == nsGkAtoms::option) {
    PRInt32 parentIndex = FindContent(aContainer);
    PRInt32 index = 0;
    GetIndexInSubtree(aContainer, aChild, &index);
    PRInt32 count = InsertRow(parentIndex, index, aChild);  // XXX
    if (mBoxObject)
      mBoxObject->RowCountChanged(parentIndex + index + 1, count);
  }

Upon adding another treechildren element, the application will duplicate
the element and add a new row to the tree.

layout/xul/base/src/tree/src/nsTreeContentView.cpp:1007

  if (childTag == nsGkAtoms::treechildren) {
    PRInt32 index = FindContent(aContainer);
    if (index >= 0) {
      Row* row = (Row*)mRows[index];
      row->SetEmpty(PR_FALSE);
      if (mBoxObject)
        mBoxObject->InvalidateRow(index);
      if (row->IsContainer() && row->IsOpen()) {
        PRInt32 count = EnsureSubtree(index);   // XXX: index = 0, and
should be pointing to the optgroup
        if (mBoxObject)
          mBoxObject->RowCountChanged(index + 1, count);
      }
    }
  }

  nsCOMPtr<nsIContent> child;
  if (row->mContent->Tag() == nsGkAtoms::optgroup)
    child = row->mContent;
  else {
    nsTreeUtils::GetImmediateChild(row->mContent,
nsGkAtoms::treechildren, getter_AddRefs(child));
    if (! child) {
      return 0;
    }
  }

Calling EnsureSubtree will serialize the object at the specified row
index.

layout/xul/base/src/tree/src/nsTreeContentView.cpp:1297
PRInt32
nsTreeContentView::EnsureSubtree(PRInt32 aIndex)
{
  Row* row = (Row*)mRows[aIndex];

  nsCOMPtr<nsIContent> child;
  if (row->mContent->Tag() == nsGkAtoms::optgroup)
    child = row->mContent;
  else {
    nsTreeUtils::GetImmediateChild(row->mContent,
nsGkAtoms::treechildren, getter_AddRefs(child));
    if (! child) {
      return 0;
    }
  }
  nsAutoVoidArray rows;
  PRInt32 index = 0;
  Serialize(child, aIndex, &index, rows);   // XXX: serialize the
optgroup again due to both indexes referencing the same variable
  mRows.InsertElementsAt(rows, aIndex + 1);
  PRInt32 count = rows.Count();

Calling Serialize will serialize an element and add it to the row
specified by index. 

layout/xul/base/src/tree/src/nsTreeContentView.cpp:1139
// Recursively serialize content, starting with aContent.
void
nsTreeContentView::Serialize(nsIContent* aContent, PRInt32 aParentIndex,
PRInt32* aIndex, nsVoidArray& aRows)
{
  ChildIterator iter, last;
  for (ChildIterator::Init(aContent, &iter, &last); iter != last;
++iter) {
    nsCOMPtr<nsIContent> content = *iter;
    nsIAtom *tag = content->Tag();
    PRInt32 count = aRows.Count();

    if (content->IsNodeOfType(nsINode::eXUL)) {
      if (tag == nsGkAtoms::treeitem)
        SerializeItem(content, aParentIndex, aIndex, aRows);
      else if (tag == nsGkAtoms::treeseparator)
        SerializeSeparator(content, aParentIndex, aIndex, aRows);
    }
    else if (content->IsNodeOfType(nsINode::eHTML)) {
      if (tag == nsGkAtoms::option)
        SerializeOption(content, aParentIndex, aIndex, aRows);  // XXX
      else if (tag == nsGkAtoms::optgroup)
        SerializeOptGroup(content, aParentIndex, aIndex, aRows);
    }
    *aIndex += aRows.Count() - count;
  }
}

When serializing an optgroup, a new row is created. The optgroup will
then serialize it's child "option" element.

void
nsTreeContentView::SerializeOptGroup(nsIContent* aContent, PRInt32
aParentIndex,
                                     PRInt32* aIndex, nsVoidArray&
aRows)
{
  Row* row = Row::Create(mAllocator, aContent, aParentIndex);
  aRows.AppendElement(row);
  row->SetContainer(PR_TRUE);
  row->SetOpen(PR_TRUE);

  nsCOMPtr<nsIContent> child;
  nsTreeUtils::GetImmediateChild(aContent, nsGkAtoms::option,
getter_AddRefs(child));
  if (child) {
    // Now, recursively serialize our child.
    PRInt32 count = aRows.Count();
    PRInt32 index = 0;
    Serialize(aContent, aParentIndex + *aIndex + 1, &index, aRows);    
// XXX: child element
    row->mSubtreeSize += aRows.Count() - count;
  }
  else
    row->SetEmpty(PR_TRUE);
}

When an element is removed from the tree, the following code is called
to remove a row from the document. This will deallocate all the contents
of a particular row.

layout/xul/base/src/tree/src/nsTreeContentView.cpp:1051
void
nsTreeContentView::ContentRemoved(nsIDocument *aDocument,
                                     nsIContent* aContainer,
                                     nsIContent* aChild,
                                     PRInt32 aIndexInContainer)
{

layout/xul/base/src/tree/src/nsTreeContentView.cpp:1105
  else if (tag == nsGkAtoms::treeitem ||
           tag == nsGkAtoms::treeseparator ||
           tag == nsGkAtoms::option ||
           tag == nsGkAtoms::optgroup
          ) {
    PRInt32 index = FindContent(aChild);
    if (index >= 0) {
      PRInt32 count = RemoveRow(index);     // XXX: remove the option
element
      if (mBoxObject)
        mBoxObject->RowCountChanged(index, -count);
    }
  }

PRInt32
nsTreeContentView::RemoveRow(PRInt32 aIndex)
{
  Row* row = (Row*)mRows[aIndex];
  PRInt32 count = row->mSubtreeSize + 1;
  PRInt32 parentIndex = row->mParentIndex;

  Row::Destroy(mAllocator, row);
  for(PRInt32 i = 1; i < count; i++) {
    Row* nextRow = (Row*)mRows[aIndex + i];
    Row::Destroy(mAllocator, nextRow);          // XXX
  }
  mRows.RemoveElementsAt(aIndex, count);

  UpdateSubtreeSizes(parentIndex, -count);
  
  UpdateParentIndexes(aIndex, 0, -count);

  return count;
}

Upon the deallocation, the duplicate variable reference can be fetched
via the GetItemAtIndex method. Due to this reference being freed, the
contents of the option tag point to recently freed memory.

layout/xul/base/src/tree/src/nsTreeContentView.cpp:759
NS_IMETHODIMP
nsTreeContentView::GetItemAtIndex(PRInt32 aIndex, nsIDOMElement**
_retval)
{
  NS_PRECONDITION(aIndex >= 0 && aIndex < mRows.Count(), "bad index");
  if (aIndex < 0 || aIndex >= mRows.Count())
    return NS_ERROR_INVALID_ARG;   

  Row* row = (Row*)mRows[aIndex];
  row->mContent->QueryInterface(NS_GET_IID(nsIDOMElement),
(void**)_retval);

  return NS_OK;
}


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

This vulnerability was discovered by:
    * regenrecht

Comment 1

[Daniel Veditz [:dveditz]]

Shiretoko crash: bp-b848e49b-6227-485f-80cf-d9cd42100106

Comment 2

[Daniel Veditz [:dveditz]]

I also crash in Namoroka:
  bp-45c05fb1-be24-4726-ade7-35d822100106
  bp-b16e2c0b-e4ad-4116-8e2e-8d9352100106

That one didn't happen immediately, in fact I forgot I had run the testcase but crashed later when I was switching tabs. Then crashed again after restoring.

We fixed crashes in this file before:
https://bugzilla.mozilla.org/buglist.cgi?quicksearch=FIX+nsTreeContentView

Most recently we fixed security bug 479931 in 1.9.1.6 which may have inspired regenrecht to code-inspect this file. Funtimes.

Comment 3

[Timothy Nikkel (:tnikkel)]

In bug 366203 we tried one way of not allowing multiple treechildren, but it caused a regression. Maybe we can disallow multiple treechildren some other way.

Comment 4

[[e-mail]]

(In reply to comment #3)
> In bug 366203 we tried one way of not allowing multiple treechildren, but it
> caused a regression. Maybe we can disallow multiple treechildren some other
> way.
But that case was about multiple top-level treechildren, wasn't it?

Comment 5

[[e-mail]]

Attachment: Proposed patch

Unlike ContentInserted, I noticed that Serialise doesn't verify the type of the child against the type of the parent. Bringing the two back into sync seems to resolve this bug. Note that I don't check the tag of the parent because my belief is that the callers already ensure a suitable parent.

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to comment #4)
> But that case was about multiple top-level treechildren, wasn't it?

Ah, okay, I just skimmed the bug report and saw "multiple treechildren".

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 420596 [details] [diff] [review]
Proposed patch

This looks fine.... but why is there any HTML anything in here?  Is that a carryover from XBL form controls?  If so, can we kill it?

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 420596 [details] [diff] [review]
Proposed patch

I don't think this actually addresses the real issue here. This patch will refuse to construct a row for the optgroup at all (because it is HTML and its parent is XUL), thereby avoiding the problem, but regressing anyone using optgroup/options in a tree.

Comment 9

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

using optgroupt/option in a tree is/was for XBL based form controls, right?
All that code should be just removed.

Comment 10

[[e-mail]]

And it was already not supported for dynamic changes to a XUL tree anyway.

Comment 11

[Timothy Nikkel (:tnikkel)]

If we can drop optgroup/option inside trees (I can't speak about this), then a patch that clearly does that would be better than one that does that in an obfuscated way.

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: alternate patch

Don't allow XUL nodes under non-XUL nodes.

Comment 13

[[e-mail]]

Comment on attachment 424181 [details] [diff] [review]
alternate patch

>@@ -923,35 +923,35 @@ nsTreeContentView::AttributeChanged(nsID
>+      if (parent && parent->IsXUL()) {
>         PRInt32 index = FindContent(parent);
[Unnecessary?]

>@@ -1075,16 +1078,19 @@ nsTreeContentView::ContentRemoved(nsIDoc
>+    if (!aContainer->IsXUL())
[Unnecessary?]

>@@ -1267,17 +1276,17 @@ nsTreeContentView::GetIndexInSubtree(nsI
>+    if (content->IsXUL() && aContainer->IsXUL()) {
[You didn't cache containerIsXUL this time?]

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to comment #13)
> (From update of attachment 424181 [details] [diff] [review])
> >@@ -923,35 +923,35 @@ nsTreeContentView::AttributeChanged(nsID
> >+      if (parent && parent->IsXUL()) {
> >         PRInt32 index = FindContent(parent);
> [Unnecessary?]

Better safe than sorry I guess.

> >@@ -1075,16 +1078,19 @@ nsTreeContentView::ContentRemoved(nsIDoc
> >+    if (!aContainer->IsXUL())
> [Unnecessary?]

I think this is needed, if say a treechildren is removed from an optgroup parent.

> >@@ -1267,17 +1276,17 @@ nsTreeContentView::GetIndexInSubtree(nsI
> >+    if (content->IsXUL() && aContainer->IsXUL()) {
> [You didn't cache containerIsXUL this time?]

I overlooked that, I can cache it.

Comment 15

[[e-mail]]

I was thinking that if you can find the row to remove then the parent must be XUL otherwise you wouldn't have added the row. (And in the unlikely event that you somehow find a way of breaking that again then you would still want to remove the row so that you don't dangle for next time...)

Comment 16

[Boris Zbarsky [:bzbarsky]]

> If we can drop optgroup/option inside trees (I can't speak about this), then a
> patch that clearly does that would be better than one that does that in an
> obfuscated way.

Agreed, and I think we can in fact drop them.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 424181 [details] [diff] [review]
alternate patch

sr=me, but if we dropped the optgroup/whatever stuff, could we undo some or all of this?

Comment 18

[Timothy Nikkel (:tnikkel)]

(In reply to comment #15)
> I was thinking that if you can find the row to remove then the parent must be
> XUL otherwise you wouldn't have added the row. (And in the unlikely event that
> you somehow find a way of breaking that again then you would still want to
> remove the row so that you don't dangle for next time...)

Sure, if aChild has a row then we would want to remove it. The situation I know we can't let happen is if aChild is a treechildren and aContainer is an optgroup. Because then we execute this code

  if (tag == nsGkAtoms::treechildren) {
    PRInt32 index = FindContent(aContainer);
    if (index >= 0) {
      Row* row = mRows[index];
      row->SetEmpty(PR_TRUE);
      PRInt32 count = RemoveSubtree(index);

and now we have deleted the rows for any options/optgroups that were children of the container optgroup. Would you prefer if I just checked aContainer->IsXUL() inside the treechildren if?

Comment 19

[[e-mail]]

Actually I think it might be clearer if you move the aContainer->IsXUL() check earlier in AttributeChanged to match the other methods.

Comment 20

[Timothy Nikkel (:tnikkel)]

(In reply to comment #19)
> Actually I think it might be clearer if you move the aContainer->IsXUL() check
> earlier in AttributeChanged to match the other methods.

Ok.

Comment 21

[Timothy Nikkel (:tnikkel)]

> Would you prefer if I just checked
> aContainer->IsXUL() inside the treechildren if?

So is that a 'no' to this then?

Comment 22

[[e-mail]]

Correct, otherwise that would defeat the point of comment #19 ;-)

Comment 23

[Timothy Nikkel (:tnikkel)]

Attachment: alternate patch v2

For reference here is what I will land with the changes made.

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 420596 [details] [diff] [review]
Proposed patch

Clearing review request.

Comment 25

[Timothy Nikkel (:tnikkel)]

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

Comment 26

[Timothy Nikkel (:tnikkel)]

And after some bake time on trunk we'll want this on the branches, which is now just 1.9.2 and 1.9.1, correct?

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking on 1.9.1 and 1.9.2, does it affect 1.9.0?

Comment 28

[Timothy Nikkel (:tnikkel)]

It does affect 1.9.0, but I am under the impression that the freeze for the last ever 1.9.0.x release is long passed.

Comment 29

[Boris Zbarsky [:bzbarsky]]

The freeze for the last MoCo firefox release may be passed, but we generally land changes like this on branches that we may not be using but other products are, as needed.

Comment 30

[Timothy Nikkel (:tnikkel)]

Ok, so what flags do we use in that case? Just an approval flag on the patch?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Yep.

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Does this apply cleanly to 1.9.2 and 1.9.1? If so, please request approval on the patch.

Comment 33

[Timothy Nikkel (:tnikkel)]

Attachment: alternate patch v2 1.9.0

Merge was trivial.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 424371 [details] [diff] [review]
alternate patch v2

Approved for 1.9.2.2, 1.9.1.9, and 1.9.0.19, a=dveditz

Comment 35

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/976224d9dfb3

Comment 36

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/353af8416a4f

Comment 37

[:Gavin Sharp [email: [e-mail]]]

mozilla/layout/xul/base/src/tree/src/nsTreeContentView.cpp 	1.72

Comment 38

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

Verified fixed for 1.9.0 using testcase with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.19pre) Gecko/2010031106 GranParadiso/3.0.19pre (.NET CLR 3.5.30729).

Verified for 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.9pre) Gecko/20100311 Shiretoko/3.5.9pre (.NET CLR 3.5.30729)

Verified for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100311 Namoroka/3.6.2pre (.NET CLR 3.5.30729).

Comment 39

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8 patch

Comment 40

[Timothy Nikkel (:tnikkel)]

Where did the testcase in this bug go? I wanted to check it in as a crashtest now that this bug is public.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You should be able to see it now.

Comment 42

[Timothy Nikkel (:tnikkel)]

Added crashtest
http://hg.mozilla.org/mozilla-central/rev/9c4c5b4232b3