Last Comment Bug 554255 - (CVE-2010-1199) XSLT Sort Remote Code Execution Vulnerability (ZDI-CAN-747)
(CVE-2010-1199)
: XSLT Sort Remote Code Execution Vulnerability (ZDI-CAN-747)
Status: RESOLVED FIXED
[sg:critical]
: fixed1.9.0.20, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a5
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-22 23:32 PDT by Daniel Veditz [:dveditz]
Modified: 2010-07-24 14:20 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1+
.4+
.4-fixed
.10+
.10-fixed


Attachments
v1 (1.39 KB, patch)
2010-03-23 01:58 PDT, Peter Van der Beken [:peterv]
jonas: review+
jst: superreview+
christian: approval1.9.2.4+
christian: approval1.9.1.10+
dveditz: approval1.9.0.next+
Details | Diff | Review

Description Daniel Veditz [:dveditz] 2010-03-22 23:32:11 PDT
Created attachment 434177 [details]
zdi's PoC

Reported to the security alias:

ZDI-CAN-747: Mozilla Firefox XSLT Sort 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 target must visit a
malicious page or render a malicious file.

The specific flaw exists within a particular XSLT transformation when
applied to an XML document. If a large number of elements have this
transformation applied to them, the application will misallocate a
buffer. Upon usage of this buffer, the application will copy more data
than allocated thus causing an overflow. This can lead to code execution
under the context of the application.

This bug is triggered via the XSLT sort function and is implemented in
the following code.

Upon evaluating of an XML statement, the application will execute the
following code. This code will count the number of sort keys, and then
add each key to a txNodeSorter element.
content/xslt/src/xslt/txInstructions.cpp:646
nsresult
txPushNewContext::execute(txExecutionState& aEs)
{
    nsRefPtr<txAExprResult> exprRes;
    nsresult rv = mSelect->evaluate(aEs.getEvalContext(),
                                    getter_AddRefs(exprRes));   // XXX
    NS_ENSURE_SUCCESS(rv, rv);

    if (exprRes->getResultType() != txAExprResult::NODESET) {
        // XXX ErrorReport: nodeset expected
        return NS_ERROR_XSLT_NODESET_EXPECTED;
    }

    txNodeSet* nodes = static_cast<txNodeSet*>
                                  (static_cast<txAExprResult*>
                                              (exprRes));   // XXX:
nodes to sort

    if (nodes->isEmpty()) {
        aEs.gotoInstruction(mBailTarget);

        return NS_OK;
    }

    txNodeSorter sorter;
    PRUint32 i, count = mSortKeys.Length();
    for (i = 0; i < count; ++i) {
        SortKey& sort = mSortKeys[i];
        rv = sorter.addSortElement(sort.mSelectExpr, sort.mLangExpr,    
   // XXX: number of sort keys
                                   sort.mDataTypeExpr, sort.mOrderExpr,
                                   sort.mCaseOrderExpr,
                                   aEs.getEvalContext());
        NS_ENSURE_SUCCESS(rv, rv);
    }
    nsRefPtr<txNodeSet> sortedNodes;
    rv = sorter.sortNodeSet(nodes, &aEs, getter_AddRefs(sortedNodes)); 
// XXX
    NS_ENSURE_SUCCESS(rv, rv);

This code will then add the specified key to the list of nodes to be
sorted.
content/xslt/src/xslt/txNodeSorter.cpp:68
nsresult
txNodeSorter::addSortElement(Expr* aSelectExpr, Expr* aLangExpr,
                             Expr* aDataTypeExpr, Expr* aOrderExpr,
                             Expr* aCaseOrderExpr, txIEvalContext*
aContext)
{
...
    // mSortKeys owns key now. 
    rv = mSortKeys.add(key);
    NS_ENSURE_SUCCESS(rv, rv);

    key.forget();
    mNKeys++;   // XXX

    return NS_OK;
}

After counting and copying the number of keys, the application will
proceed to sort the elements in this buffer. It does this by first
allocating via the following code.
content/xslt/src/xslt/txNodeSorter.cpp:157
nsresult
txNodeSorter::sortNodeSet(txNodeSet* aNodes, txExecutionState* aEs,
                          txNodeSet** aResult)
{
    if (mNKeys == 0 || aNodes->isEmpty()) {
        NS_ADDREF(*aResult = aNodes);

        return NS_OK;
    }

    *aResult = nsnull;

    nsRefPtr<txNodeSet> sortedNodes;
    nsresult rv =
aEs->recycler()->getNodeSet(getter_AddRefs(sortedNodes));
    NS_ENSURE_SUCCESS(rv, rv);

    txNodeSetContext* evalContext = new txNodeSetContext(aNodes, aEs);
    NS_ENSURE_TRUE(evalContext, NS_ERROR_OUT_OF_MEMORY);

    rv = aEs->pushEvalContext(evalContext);
    NS_ENSURE_SUCCESS(rv, rv);

    // Create and set up memoryblock for sort-values and indexarray
    PRUint32 len = static_cast<PRUint32>(aNodes->size());   // XXX
    void* mem = PR_Malloc(len * (sizeof(PRUint32) + mNKeys *
sizeof(TxObject*)));   // XXX
    NS_ENSURE_TRUE(mem, NS_ERROR_OUT_OF_MEMORY);

    PRUint32* indexes = static_cast<PRUint32*>(mem);    // XXX
    TxObject** sortValues = reinterpret_cast<TxObject**>(indexes +
len);

    PRUint32 i;
    for (i = 0; i < len; ++i) {
        indexes[i] = i;
    }
    memset(sortValues, 0, len * mNKeys * sizeof(TxObject*));

The misallocation is located in the following line of code. In this
case, the attacker controls more than 32 of the bits within the
allocation which will allow one to cause an integer overflow.

void* mem = PR_Malloc(len * (sizeof(PRUint32) + mNKeys *
sizeof(TxObject*)));   

Upon causing of the integer overflow, a copy exists which initializes
that underallocated buffer.

    PRUint32 i;
    for (i = 0; i < len; ++i) {
        indexes[i] = i;
    }

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



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

This vulnerability was discovered by:
    * Martin Barbella
Comment 1 Daniel Veditz [:dveditz] 2010-03-22 23:57:08 PDT
bp-8ffcc1be-56d2-4242-9d0e-c3bab2100322 from the PoC.
Comment 2 Peter Van der Beken [:peterv] 2010-03-23 01:58:33 PDT
Created attachment 434190 [details] [diff] [review]
v1

I think this catches all overflows, please double-check.
Comment 3 Daniel Veditz [:dveditz] 2010-03-23 10:40:00 PDT
Comment on attachment 434190 [details] [diff] [review]
v1

>+    if (mNKeys > (PR_UINT32_MAX - sizeof(PRUint32)) / sizeof(TxObject*) ||
>+        len >= PR_UINT32_MAX / itemSize) {

Does everything have to allow maximum sizes? Why not chop it off way short of an overflow (Too many--no soup for you!)? And a check like mNKeys > (0x08000000 / sizeof(TxObject*)) is a less complex check. I doubt we need to support tens of millions of these. (Still have to check len too, I'm simplifying here).

>+    // Don't overflow when calculating the length of the sort buffer.

If you do that you could turn this into a less obvious comment, like "Limit resource use to something sane". And then use that in the check-in comment which might then look like a boring DoS fix rather than the suspicious I'm-saying-nothing "fix bug x" check-in comment.
Comment 4 Jonas Sicking (:sicking) 2010-04-07 09:21:54 PDT
Comment on attachment 434190 [details] [diff] [review]
v1

Ugh, it's going to suck to whack-a-mole all of these.

We should transition to using size_t in more places, and take size_t as argument to various allocators.
Comment 5 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-07 13:12:33 PDT
friendly reminder that code freeze is scheduled for monday, april 12th! :)
Comment 6 Peter Van der Beken [:peterv] 2010-04-09 13:15:47 PDT
(In reply to comment #3)
> Does everything have to allow maximum sizes? Why not chop it off way short of
> an overflow (Too many--no soup for you!)? And a check like mNKeys > (0x08000000
> / sizeof(TxObject*)) is a less complex check. I doubt we need to support tens
> of millions of these. (Still have to check len too, I'm simplifying here).

Could do that. Personally, I like it more if the conditions we check for are explicitly the same as the conditions that cause the overflow. IMO it also makes it more likely that someone changing the calculation will make the same change to the conditions.

http://hg.mozilla.org/mozilla-central/rev/647dd06b64fe
Comment 7 Peter Van der Beken [:peterv] 2010-04-09 13:16:47 PDT
Comment on attachment 434190 [details] [diff] [review]
v1

Check for an overflowing size that's then used to malloc a chunk of memory.
Comment 8 christian 2010-04-12 11:22:24 PDT
a=LegNeato for 1.9.2.4 and 1.9.1.10
Comment 9 christian 2010-04-12 11:23:21 PDT
Actually, approval was only asked for 1.9.2.4, so approving for that...
Comment 10 Reed Loden [:reed] (use needinfo?) 2010-04-12 12:24:29 PDT
(In reply to comment #9)
> Actually, approval was only asked for 1.9.2.4, so approving for that...

Is 1.9.1 affected? If so, does the patch apply to 1.9.1? If it does, please request approval.
Comment 11 christian 2010-04-12 15:43:08 PDT
Yeah, I guess my comment was fairly dumb. If this applies to 1.9.1 we'll want it there as well. a=LegNeato for 1.9.1.10 as well to speed this along.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2010-04-12 17:51:36 PDT
Fixed for 1.9.2

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a0fabd9ef95d
Comment 13 Reed Loden [:reed] (use needinfo?) 2010-04-12 19:54:48 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a14044440a86
Comment 14 Peter Van der Beken [:peterv] 2010-04-13 00:45:23 PDT
Thanks for landing. Just to be clear, this was needed on 1.9.1 too. I must have cleared the flag by accident before submitting or something.
Comment 15 Al Billings [:abillings] 2010-04-13 12:30:04 PDT
Verified fixed in 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.10pre) Gecko/20100413 Shiretoko/3.5.10pre (.NET CLR 3.5.30729). The PoC no longer crashes Firefox.

Verified for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4pre) Gecko/20100413 Namoroka/3.6.4pre (.NET CLR 3.5.30729).
Comment 16 Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-07-18 22:00:03 PDT
Comment on attachment 434190 [details] [diff] [review]
v1

Requesting approval1.9.0.next on this patch so that we can take it in upcoming Camino 2.0.x security and stability updates.  If approved, I'll handle the checkins, unless the patch author requests otherwise.
Comment 17 Daniel Veditz [:dveditz] 2010-07-22 19:18:49 PDT
Comment on attachment 434190 [details] [diff] [review]
v1

Approved for 1.9.0.20, a=dveditz
Comment 18 Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-07-24 14:20:50 PDT
Checking in content/xslt/src/xslt/txNodeSorter.cpp;
/cvsroot/mozilla/content/xslt/src/xslt/txNodeSorter.cpp,v  <--  txNodeSorter.cpp
new revision: 1.24; previous revision: 1.23
done

Note You need to log in before you can comment on or make changes to this bug.