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)
Core
XSLT
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: dveditz, Assigned: peterv)
Details
(Keywords: fixed1.9.0.20, verified1.9.1, verified1.9.2, Whiteboard: [sg:critical])
Attachments
(1 file)
1.39 KB,
patch
|
sicking
:
review+
jst
:
superreview+
christian
:
approval1.9.2.4+
christian
:
approval1.9.1.10+
dveditz
:
approval1.9.0.next+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•15 years ago
|
Component: XML → XSLT
QA Contact: xml → xslt
Updated•15 years ago
|
Hardware: x86 → All
Reporter | ||
Comment 1•15 years ago
|
||
bp-8ffcc1be-56d2-4242-9d0e-c3bab2100322 from the PoC.
Reporter | ||
Updated•15 years ago
|
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.1:
--- → wanted
status1.9.2:
--- → wanted
Assignee | ||
Comment 2•15 years ago
|
||
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)
Reporter | ||
Comment 3•15 years ago
|
||
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.
Reporter | ||
Updated•15 years ago
|
blocking1.9.1: ? → .10+
blocking1.9.2: ? → .3+
Updated•15 years ago
|
Whiteboard: [sg:critical]
Updated•15 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+
Comment 5•15 years ago
|
||
friendly reminder that code freeze is scheduled for monday, april 12th! :)
Updated•15 years ago
|
Attachment #434190 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 6•15 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
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Assignee | ||
Comment 7•15 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?
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...
Comment 10•15 years ago
|
||
(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+
Comment 11•15 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•15 years ago
|
Keywords: checkin-needed
Whiteboard: [sg:critical], checkin-needed → [sg:critical]
Comment 12•15 years ago
|
||
Fixed for 1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a0fabd9ef95d
Comment 13•15 years ago
|
||
Keywords: checkin-needed
Assignee | ||
Comment 14•15 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.
Comment 15•15 years ago
|
||
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
Reporter | ||
Updated•15 years ago
|
Alias: CVE-2010-1199
Reporter | ||
Updated•15 years ago
|
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?
Reporter | ||
Comment 17•15 years ago
|
||
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.
Description
•