552110 - (CVE-2010-1209) Use of deleted object by NodeIterator using NodeFilter which called detach (ZDI-CAN-712)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brandon Sterne (:bsterne)]

Attachment: Proof of concept

ZDI-CAN-712: Mozilla Firefox NodeIterator Remote Code Execution Vulnerability

-- 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 victim must visit a malicious page or open a malicious file.

The specific flaw exists within the application's implementation of the NodeIterator interface for traversal of the Document Object Model. Due to the implementation requiring a javascript callback, an attacker can utilize the callback in order to manipulate the contents of the page. By doing so in an unexpected manner, an attacker can cause the process to corrupt memory. Successful exploitation will lead to code execution under the context of the application.

The particular issue is located within Mozilla Firefox's implemenation of Document Traversal and involves both the NodeFilter and NodeIterator interfaces. The NodeIterator interface allows one to specify a NodeFilter in order to allow a finer granularity of control over which nodes will be traversed.

A NodeIterator's interface as specified at (http://www.w3.org/TR/DOM-Level-2-Traversal-Range/traversal.html#Traversal-NodeIterator) is defined as:

// Introduced in DOM Level 2:
interface NodeIterator {
  readonly attribute Node             root;
  readonly attribute unsigned long    whatToShow;
  readonly attribute NodeFilter       filter;
  readonly attribute boolean          expandEntityReferences;
  Node               nextNode()
                                        raises(DOMException);
  Node               previousNode()
                                        raises(DOMException);
  void               detach();
};

This interface contains two methods which are used to trigger this vulnerability. The first one is the 'filter' property. This is a readonly attribute that's set on creation of a NodeIterator. The next one is detach() which will detach a node from the tree. The implementation of this primarily resides within content/base/src/nsNodeIterator.cpp.

Creation of a NodeIterator first starts within the following snippet.
One of the parameters is the specified filter.
content/base/src/nsDocument.cpp:4767
NS_IMETHODIMP
nsDocument::CreateNodeIterator(nsIDOMNode *aRoot,
                               PRUint32 aWhatToShow,
                               nsIDOMNodeFilter *aFilter,     // XXX
                               PRBool aEntityReferenceExpansion,
                               nsIDOMNodeIterator **_retval)
{
  *_retval = nsnull;

  if (!aRoot)
    return NS_ERROR_DOM_NOT_SUPPORTED_ERR;

  nsresult rv = nsContentUtils::CheckSameOrigin(this, aRoot);
  NS_ENSURE_SUCCESS(rv, rv);

  NS_ENSURE_ARG_POINTER(_retval);

  nsCOMPtr<nsINode> root = do_QueryInterface(aRoot);
  NS_ENSURE_TRUE(root, NS_ERROR_DOM_NOT_SUPPORTED_ERR);

  nsNodeIterator *iterator = new nsNodeIterator(root,
                                                aWhatToShow,
                                                aFilter,  // XXX
                                               
aEntityReferenceExpansion);

This will then instantiated a new NodeIterator which will then create an nsTraversal object which will utilize the specified filter and store it as one of it's properties.
content/base/src/nsNodeIterator.cpp:183
nsNodeIterator::nsNodeIterator(nsINode *aRoot,
                               PRUint32 aWhatToShow,
                               nsIDOMNodeFilter *aFilter,
                               PRBool aExpandEntityReferences) :
    nsTraversal(aRoot, aWhatToShow, aFilter, aExpandEntityReferences), 
// XXX
    mDetached(PR_FALSE),
    mPointer(mRoot, PR_TRUE)
{
    aRoot->AddMutationObserver(this);
}

Iteration of a NodeIterator is then performed via the nextNode() and prevNode() methods. The implementation of the nextNode() method is located in the following file. If there is a filter, this code will ask the filter function if the node is valid, and if so will return it. To accomplish this, the application will use the TestNode method.

content/base/src/nsNodeIterator.cpp:264
/* nsIDOMNode nextNode ()  raises (DOMException); */
NS_IMETHODIMP nsNodeIterator::NextNode(nsIDOMNode **_retval)
{
    nsresult rv;
    PRInt16 filtered;

    *_retval = nsnull;

    if (mDetached)
        return NS_ERROR_DOM_INVALID_STATE_ERR;

    mWorkingPointer = mPointer;

    while (mWorkingPointer.MoveToNext(mRoot)) {
        nsCOMPtr<nsINode> testNode = mWorkingPointer.mNode;
        rv = TestNode(testNode, &filtered);       // XXX
        NS_ENSURE_SUCCESS(rv, rv);

        if (filtered == nsIDOMNodeFilter::FILTER_ACCEPT) {
            mPointer = mWorkingPointer;
            mWorkingPointer.Clear();
            return CallQueryInterface(testNode, _retval);
        }
    }

    mWorkingPointer.Clear();
    return NS_OK;
}

nsTraversal::TestNode will then call the filtering function and return the filtervalue of the particular node that the iterator currently has. This will then call the AcceptNode method of the NodeFilter interface.

content/base/src/nsTraversal.cpp:67
/*
 * Tests if and how a node should be filtered. Uses mWhatToShow and
 * mFilter to test the node.
 * @param aNode     Node to test
 * @param _filtered Returned filtervalue. See nsIDOMNodeFilter.idl
 * @returns         Errorcode
 */
nsresult nsTraversal::TestNode(nsINode* aNode, PRInt16* _filtered)
{
    nsresult rv;

    *_filtered = nsIDOMNodeFilter::FILTER_SKIP;

    PRUint16 nodeType = 0;
    // Check the most common cases
    if (aNode->IsNodeOfType(nsINode::eELEMENT)) {
        nodeType = nsIDOMNode::ELEMENT_NODE;
    }
    else if (aNode->IsNodeOfType(nsINode::eCONTENT)) {
        nsIAtom* tag = static_cast<nsIContent*>(aNode)->Tag();
        if (tag == nsGkAtoms::textTagName) {
            nodeType = nsIDOMNode::TEXT_NODE;
        }
        else if (tag == nsGkAtoms::cdataTagName) {
            nodeType = nsIDOMNode::CDATA_SECTION_NODE;
        }
        else if (tag == nsGkAtoms::commentTagName) {
            nodeType = nsIDOMNode::COMMENT_NODE;
        }
        else if (tag == nsGkAtoms::processingInstructionTagName) {
            nodeType = nsIDOMNode::PROCESSING_INSTRUCTION_NODE;
        }
    }

    nsCOMPtr<nsIDOMNode> domNode;
    if (!nodeType) {
        domNode = do_QueryInterface(aNode);
        rv = domNode->GetNodeType(&nodeType);
        NS_ENSURE_SUCCESS(rv, rv);
    }

    if (nodeType <= 12 && !((1 << (nodeType-1)) & mWhatToShow)) {
        return NS_OK;
    }

    if (mFilter) {          // XXX
        if (!domNode) {
            domNode = do_QueryInterface(aNode);
        }

        return mFilter->AcceptNode(domNode, _filtered);    // XXX
    }

    *_filtered = nsIDOMNodeFilter::FILTER_ACCEPT;
    return NS_OK;
}

A NodeFilter interface is located at (http://www.w3.org/TR/DOM-Level-2-Traversal-Range/traversal.html#Traversal-NodeFilter) and can be defined as.

interface NodeFilter {

  // Constants returned by acceptNode
  const short               FILTER_ACCEPT                  = 1;
  const short               FILTER_REJECT                  = 2;
  const short               FILTER_SKIP                    = 3;


  // Constants for whatToShow
  const unsigned long       SHOW_ALL                       =
0xFFFFFFFF;
  const unsigned long       SHOW_ELEMENT                   =
0x00000001;
  const unsigned long       SHOW_ATTRIBUTE                 =
0x00000002;
  const unsigned long       SHOW_TEXT                      =
0x00000004;
  const unsigned long       SHOW_CDATA_SECTION             =
0x00000008;
  const unsigned long       SHOW_ENTITY_REFERENCE          =
0x00000010;
  const unsigned long       SHOW_ENTITY                    =
0x00000020;
  const unsigned long       SHOW_PROCESSING_INSTRUCTION    =
0x00000040;
  const unsigned long       SHOW_COMMENT                   =
0x00000080;
  const unsigned long       SHOW_DOCUMENT                  =
0x00000100;
  const unsigned long       SHOW_DOCUMENT_TYPE             =
0x00000200;
  const unsigned long       SHOW_DOCUMENT_FRAGMENT         =
0x00000400;
  const unsigned long       SHOW_NOTATION                  =
0x00000800;

  short              acceptNode(in Node n);
};

If a malicious javascript filter that removes the elements the NodeIterator is iterating over, the application will access memory that's freed while the node is being iterated.

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

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

This vulnerability was discovered by:
    * regenrecht

Comment 1

[Boris Zbarsky [:bzbarsky]]

sicking, you know this code, right?

Comment 2

[Johnny Stenback (:jst)]

Over to Ben Newman for investigation...

Comment 3

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #0)
> If a malicious javascript filter that removes the elements the NodeIterator is
> iterating over, the application will access memory that's freed while the node
> is being iterated.

I'm missing something here.  What evidence indicates that freed memory has been accessed and/or that memory has been corrupted?  When/where was that memory freed, and when/where is it accessed, specifically?  The proof of concept is a bit hand-wavy, to say the least.

I'm not questioning the legitimacy of the vulnerability.  I just want to be sure we're all talking about the same memory.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think I can see a way where we could crash here. Though the reasons for the crash is quite different from the description of the original reporter.

So I think that if you call detach() from within the filter, we'll stop observing the document. However we'll still be traversing the document until we find a useful node.

So steps to attack would be something like:

1. Create a nodeiterator with a nodefilter which always returns FILTER_ACCEPT
2. Iterate to a node, X, that is a *grand*child of the root
3. Change the nodefilter to upon call do the following
  i) call detach() on the nodeiterator
  ii) remove X parent from its parent
  iii) drop any references to X or its parent
  iv) do enough stuff that we GC (for example perform a sync XHR)
  v) return FILTER_SKIP
4. At this point we should be attempting to traverse the tree, but the nodes
   we're attempting to traverse are GCed and we should crash

Not sure what the best way to fix this is. Probably to not remove the mutation observer until after we've found a new node if detach() is called while we're inside NextNode or PreviousNode



3. Call nodeiterator.next()

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Alternatively, after every call to TestNode always check mDetached and bail if it's true.

Comment 6

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

Need to have rationale to block - why'd you nominate them? As far as we can tell, we don't know if the PoC is correct yet.

Comment 7

[Reed Loden [:reed]]

(In reply to comment #6)
> Need to have rationale to block - why'd you nominate them? As far as we can
> tell, we don't know if the PoC is correct yet.

Because most ZDIs we get (save maybe one or two) have been valid security bugs (mostly sg:critical, too). Based on that reputation, we should preemptively block to ensure we get this fixed in the next scheduled release.

Comment 8

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Preliminary/preemptive patch.

Jonas, this patch addresses the scenario you described.

Change summary:

* Consolidated code that was duplicated between NextNode and PreviousNode into NextOrPrevNode.

* Calling .detach() in the filter function now causes the current .nextNode() or .previousNode() call to return null (another option would be to throw the NS_ERROR_DOM_INVALID_STATE_ERR exception).

* The call to mRoot->RemoveMutationObserver(this) is now postponed until the nsNodeIterator destructor, whereas previously the observer was removed immediately in Detach.

* Any mutation events after the .detach() call are ignored.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 437940 [details] [diff] [review]
Preliminary/preemptive patch.

>+nsNodeIterator::NextOrPrevNode(PRBool (NodePointer::*aMove)(nsINode*),

Nit: Please use a typedef, the C function pointer syntax is so ugly that it's always worth hiding behind a typedef IMHO.

>         rv = TestNode(testNode, &filtered);
>         NS_ENSURE_SUCCESS(rv, rv);
> 
>+        if (mDetached)
>+            break;

The safest thing would probably be to simply return NS_ERROR_DOM_INVALID_STATE_ERR here. Especially if you add mWorkingPointer.Clear() to nsNodeIterator::Detach

> NS_IMETHODIMP nsNodeIterator::Detach(void)
> {
>     if (!mDetached) {
>-        mRoot->RemoveMutationObserver(this);
>-

Why remove this? Having extra mutation observers costs us perf-wise so it'd be nice to keep it. Seems like it should be safe to do so with your other changes. Then you can also remove the changes to nsNodeIterator::ContentInserted/ContentRemoved

Would love to see some tests here too. At least attached to this bug for now until it's safe to check in on trunk.

Minusing for now, feel free to rerequest if there's something you don't agree with.

Comment 10

[Ben Newman (:bnewman) (:benjamn)]

Attachment: v2.

(In reply to comment #9)
> Nit: Please use a typedef, the C function pointer syntax is so ugly that it's
> always worth hiding behind a typedef IMHO.

No problem.

> >         rv = TestNode(testNode, &filtered);
> >         NS_ENSURE_SUCCESS(rv, rv);
> > 
> >+        if (mDetached)
> >+            break;
> 
> The safest thing would probably be to simply return
> NS_ERROR_DOM_INVALID_STATE_ERR here.

I guess so.  Could there ever be any sane reason for calling .detach() from the filter function?

> Especially if you add mWorkingPointer.Clear() to nsNodeIterator::Detach

I'd rather call mWorkingPointer.Clear() in NextOrPrevNode, just before returning the error.  mWorkingPointer is supposed to be valid for the duration of NextOrPrevNode, so invalidating it in some other method seems sneaky.

I also noticed that we don't call mWorkingPointer.Clear() when the NS_ENSURE_SUCCESS fails, so I fixed that as well.

> > NS_IMETHODIMP nsNodeIterator::Detach(void)
> > {
> >     if (!mDetached) {
> >-        mRoot->RemoveMutationObserver(this);
> >-
> 
> Why remove this? Having extra mutation observers costs us perf-wise so it'd be
> nice to keep it. Seems like it should be safe to do so with your other changes.
> Then you can also remove the changes to
> nsNodeIterator::ContentInserted/ContentRemoved

Reverted.

> Would love to see some tests here too. At least attached to this bug for now
> until it's safe to check in on trunk.

I'd love to have some tests, too, but I have a feeling it's going to be tricky.  I'll look into that...

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 437992 [details] [diff] [review]
v2.

The AutoClear, while cool, seems a bit overly ambitious :) But sure.

r=me

Remember that you'll need an sr too given that this is a security bug.

Comment 12

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Patch with regression tests.

Added two kinds of tests, one just to ensure that the exception is properly thrown when .detach() is called inside the filter function, and the other implementing the scenario Jonas proposed in comment #4.

Comment 13

[Ben Newman (:bnewman) (:benjamn)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/e6498322182f

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, I just realized that this fix might not be good enough :(

Consider the following steps:

1. Create a nodeiterator with a nodefilter which always returns FILTER_ACCEPT
2. Iterate to a node, X, that is a *grand*child of the root
3. Change the nodefilter to upon call do the following
  i) call detach() on the nodeiterator
  ii) remove X parent from its parent
  iii) drop any references to X or its parent
  iv) cause a GC
  v) call nodeiterator.next()
4. At this point we attempt to traverse the tree, but the nodes
   we're attempting to traverse are GCed and we should crash

Note here that we're calling nodeiterator.next from *inside* the filter.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Err.. never mind. The first thing we do in the iteration functions is to see if we've detached and then just bail.

It's all good.

Comment 16

[Daniel Veditz [:dveditz]]

It looks like this patch applies directly to the older branches, any reason we can't land this ASAP? Please request approval on the patch here unless we need a different branch patch for some reason--in which case please make one :-)

Comment 17

[(no longer active)]

In the future, please try to not include security sensitive information in commit messages... ;-)

Comment 18

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #17)
> In the future, please try to not include security sensitive information in
> commit messages... ;-)

Yikes, you're right, sorry.

Landed on mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4dfce925fecd

Comment 19

[Reed Loden [:reed]]

Also, the tests should have been held off until after the release has been made. :/

Comment 20

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #19)
> Also, the tests should have been held off until after the release has been
> made. :/

I did run the tests, and they pass, for what it's worth.

Comment 21

[(no longer active)]

(In reply to comment #20)
> (In reply to comment #19)
> > Also, the tests should have been held off until after the release has been
> > made. :/
> 
> I did run the tests, and they pass, for what it's worth.

No, the issue here is that black hats can benefit from the tests in order to construct exploits...

Comment 22

[LegNeato]

Comment on attachment 437992 [details] [diff] [review]
v2.

a=LegNeato (I gave verbal approval before this landed)

Comment 23

[Ben Newman (:bnewman) (:benjamn)]

Landed on mozilla-1.9.2 as well:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ff200c22d5ee

My apologies for the ill-considered commit message.

Comment 24

[LegNeato]

Comment on attachment 437992 [details] [diff] [review]
v2.

...and 1.9.1.11

Comment 25

[Reed Loden [:reed]]

For the record, the test needs to land on 1.9.2 after the release is complete.

Comment 26

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

Verified on 1.9.1 using checked in tests.

Comment 27

[Daniel Veditz [:dveditz]]

Verified 1.9.2 on mac using checked-in test copied over (and verified it failed on earlier version 3.6.3).