Bug 554255 (CVE-2010-1199)

XSLT Sort Remote Code Execution Vulnerability (ZDI-CAN-747)

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
XSLT
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dveditz, Assigned: peterv)

Tracking

({fixed1.9.0.20, verified1.9.1, verified1.9.2})

Trunk
mozilla1.9.3a5
fixed1.9.0.20, verified1.9.1, verified1.9.2
Points:
---

Firefox Tracking Flags

(blocking2.0 beta1+, blocking1.9.2 .4+, status1.9.2 .4-fixed, blocking1.9.1 .10+, status1.9.1 .10-fixed)

Details

(Whiteboard: [sg:critical])

Attachments

(1 attachment)

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
Component: XML → XSLT
QA Contact: xml → xslt
Hardware: x86 → All
bp-8ffcc1be-56d2-4242-9d0e-c3bab2100322 from the PoC.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.1: --- → wanted
status1.9.2: --- → wanted
(Assignee)

Comment 2

7 years ago
Created attachment 434190 [details] [diff] [review]
v1

I think this catches all overflows, please double-check.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attachment #434190 - Flags: superreview?(jst)
Attachment #434190 - Flags: review?(jonas)
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.
blocking1.9.1: ? → .10+
blocking1.9.2: ? → .3+

Updated

7 years ago
Whiteboard: [sg:critical]

Updated

7 years ago
blocking2.0: ? → beta1+
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.
Attachment #434190 - Flags: review?(jonas) → review+
friendly reminder that code freeze is scheduled for monday, april 12th! :)

Updated

7 years ago
Attachment #434190 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 6

7 years ago
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
(Assignee)

Comment 7

7 years ago
Comment on attachment 434190 [details] [diff] [review]
v1

Check for an overflowing size that's then used to malloc a chunk of memory.
Attachment #434190 - Flags: approval1.9.2.4?

Comment 8

7 years ago
a=LegNeato for 1.9.2.4 and 1.9.1.10

Updated

7 years ago
Attachment #434190 - Flags: approval1.9.2.4? → approval1.9.2.4+

Comment 9

7 years ago
Actually, approval was only asked for 1.9.2.4, so approving for that...
(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.

Updated

7 years ago
Attachment #434190 - Flags: approval1.9.1.10+

Comment 11

7 years ago
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.

Updated

7 years ago
Whiteboard: [sg:critical] → [sg:critical], checkin-needed
Keywords: checkin-needed
Whiteboard: [sg:critical], checkin-needed → [sg:critical]
Fixed for 1.9.2

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a0fabd9ef95d
status1.9.2: wanted → .4-fixed
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a14044440a86
status1.9.1: wanted → .10-fixed
Keywords: checkin-needed
(Assignee)

Comment 14

7 years ago
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.
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).
Keywords: verified1.9.1, verified1.9.2
Alias: CVE-2010-1199
Group: core-security
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.
Attachment #434190 - Flags: approval1.9.0.next?
Comment on attachment 434190 [details] [diff] [review]
v1

Approved for 1.9.0.20, a=dveditz
Attachment #434190 - Flags: approval1.9.0.next? → approval1.9.0.next+
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
Keywords: fixed1.9.0.20
You need to log in before you can comment on or make changes to this bug.