Closed Bug 554255 (CVE-2010-1199) Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: XSLT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
blocking2.0 --- beta1+
blocking1.9.2 --- .4+
status1.9.2 --- .4-fixed
blocking1.9.1 --- .10+
status1.9.1 --- .10-fixed

People

(Reporter: dveditz, Assigned: peterv)

Details

(Keywords: fixed1.9.0.20, verified1.9.1, verified1.9.2, Whiteboard: [sg:critical])

Attachments

(1 file)

Attached file 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
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Attached patch v1Splinter Review
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+
Whiteboard: [sg:critical]
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! :)
Attachment #434190 - Flags: superreview?(jst) → superreview+
(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
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
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?
a=LegNeato for 1.9.2.4 and 1.9.1.10
Attachment #434190 - Flags: approval1.9.2.4? → approval1.9.2.4+
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.
Attachment #434190 - Flags: approval1.9.1.10+
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.
Whiteboard: [sg:critical] → [sg:critical], checkin-needed
Keywords: checkin-needed
Whiteboard: [sg:critical], checkin-needed → [sg:critical]
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).
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.

Attachment

General

Created:
Updated:
Size: