CVE-2025-1932

High CVSS 8.1 firefox OOB 🔧 Commit mapped

Overview

High

Severity

8.1

CVSS

Exploited ITW

Fixed

Fix Status

Impacthigh

DescriptionAn inconsistent comparator in xslt/txNodeSorter could have resulted in potentially exploitable out-of-bounds access. Only affected version 122 and later.

ComponentDOM

Bug ClassOOB

Tracker1944313

Fix commitd674323f853b (firefox) +60/-92

CWECWE-125 (Out-of-bounds read)

CVSS vectorCVSS:3.1/AV:N/AC:L/PR:N/UI:R/S:U/C:H/I:N/A:H

CISA KEVNot listed

CreditedIvan Fratric of Google Project Zero

Disclosed2025-03-04

Background

XSLT `xsl:sort`: XSLT lets a stylesheet sort a node set by one or more keys. Gecko implements this in txNodeSorter, which builds per-node sort values and orders node indexes with a comparison callback.
`txNodeSorter::compareNodes` / `calcSortValue`: compareNodes is the comparator handed to the sort; it lazily computes each node’s sortable value via calcSortValue, then calls the key’s txXPathResultComparator::compareValues.
Strict weak ordering: A sort comparator must be self-consistent (antisymmetric, transitive, irreflexive). A comparator that returns contradictory answers for the same pair can drive the sort algorithm to read/write outside the array bounds.
`txResultStringComparator` / collator: The string comparator holds a locale Collator (mCollator) built in init(). If init() fails mCollator is null and comparisons cannot be performed.

Root Cause Analysis

The bug lives in dom/xslt/xslt/txNodeSorter.cpp. In the old compareNodes, the first statement was NS_ENSURE_SUCCESS(aSortData.mRv, -1); and sort values were computed inline with failure baked into the return value: if (!sortValuesA[i] && !calcSortValue(sortValuesA[i], key, &aSortData, aIndexA)) return -1; for operand A, and the symmetric block return 1; for operand B. calcSortValue set aSortData.mRv and returned false whenever createSortableValue failed (for example when the string comparator’s mCollator was null, or evaluation failed). The result is an inconsistent comparator: once any value computation fails, mRv is non-OK, so every subsequent compareNodes call returns -1 unconditionally (via the leading NS_ENSURE_SUCCESS), and during the failing comparison itself the A-path returns -1 while the B-path returns +1 for otherwise-equal situations. A comparator that reports compare(x,y) < 0 and also compare(y,x) < 0 violates strict weak ordering, and the sort routine then indexes past the ends of the indexes/sortValues arrays — the out-of-bounds access the advisory describes. The advisory’s “inconsistent comparator … out-of-bounds access” matches the diff precisely.

The patch makes the comparator total and consistent. calcSortValue is rewritten to always return a UniquePtr<txObject> (via the new createSortableValue signature returning std::pair<UniquePtr<txObject>, nsresult>), recording only the first failure in aSortData.mRv and never altering the comparison’s return value. The leading NS_ENSURE_SUCCESS early-out in compareNodes is removed, so comparisons remain internally consistent for the whole sort regardless of errors; the recorded mRv is checked only after sortNodeSet finishes. The comparator’s collator initialization is also hardened: txResultStringComparator’s constructor no longer silently swallows a failed init(); instead txNodeSorter::addSortElement constructs the comparator and calls init(lang) with NS_ENSURE_SUCCESS, so a broken collator aborts setup rather than yielding a half-built comparator. Secondary memory-management cleanup moves sortValues ownership to UniquePtr<txObject> so values are freed automatically instead of via a manual delete loop.

Key insight

Letting per-value computation failures leak into the sort comparator’s return value (and short-circuiting every later comparison to -1) produced a comparator that violated strict weak ordering, so the sort read out of bounds; the fix decouples failure recording from comparison results so the ordering stays consistent even when values can’t be built.

Attack Path

Attacker page/document Serves XML plus an XSLT stylesheet containing an xsl:sort over a node set (XSLT is web-reachable via XSLTProcessor/stylesheet PIs).
Force value-build failure Crafts the sort key / locale so that createSortableValue fails for some nodes (e.g. collator unavailable or expression evaluation failure for particular nodes), so calcSortValue fails mid-sort (inferred: exact trigger expression not shown by the diff).
Inconsistent ordering The failing comparisons return contradictory results, breaking the sort’s strict-weak-ordering assumption.
Out-of-bounds access The sort algorithm indexes the indexes/sortValues arrays past their bounds, reading (and dereferencing txObject* entries from) out-of-range memory.
Primitive A potentially exploitable out-of-bounds read / wild txObject* dereference during XSLT sorting.

Impact Assessment

This is a content-process (renderer) issue reachable by any page that can run XSLT, with no special privilege and only the user interaction of visiting/processing the document (CVSS vector AV:N, UI:R, C:H, A:H). The substantiated primitive is an out-of-bounds read driven by a non-conforming comparator, with dereference of out-of-range txObject* slots — strong enough for information disclosure or a controllable crash; full escalation beyond that is not demonstrated by the diff (inferred). Only Firefox 122 and later are affected, consistent with the comparator-change regression window the advisory cites.

Changed Functions

Function	Change	Notes
`for` dom/xslt/xslt/txNodeSorter.cpp	modified
`if` dom/xslt/xslt/txNodeSorter.cpp	modified
`if` dom/xslt/xslt/txXPathResultComparator.cpp	modified
`txResultNumberComparator` dom/xslt/xslt/txXPathResultComparator.cpp	modified
`txResultStringComparator` dom/xslt/xslt/txXPathResultComparator.h	modified
`StringValue` dom/xslt/xslt/txXPathResultComparator.h	modified
`NumberValue` dom/xslt/xslt/txXPathResultComparator.h	modified

Files Changed

dom/xslt/xslt/txNodeSorter.cpp
dom/xslt/xslt/txNodeSorter.h
dom/xslt/xslt/txXPathResultComparator.cpp
dom/xslt/xslt/txXPathResultComparator.h

Audit Directions

Comparators derived from fallible computation
Search other sorts (XSLT, DOM, intl) where a comparison callback can return an error-driven value; any that let failure change the ordering risk the same OOB.
`NS_ENSURE_SUCCESS` inside comparators
Flag comparator functions that early-return a fixed sign on recorded error state; these break strict weak ordering.
Lazy sort-value caching
Review code that lazily fills a sort-value array during comparison and relies on side effects for both value and error state.
Collator-null paths
Verify every txXPathResultComparator path tolerates a null/failed collator without producing inconsistent comparisons.

Patch

view commit

diff --git a/dom/xslt/xslt/txNodeSorter.cpp b/dom/xslt/xslt/txNodeSorter.cpp
index cc7ac3e3540..2da7871cc04 100644
--- a/dom/xslt/xslt/txNodeSorter.cpp
+++ b/dom/xslt/xslt/txNodeSorter.cpp
@@ -92,8 +92,12 @@ nsresult txNodeSorter::addSortElement(Expr* aSelectExpr, Expr* aLangExpr,
       }
     }
 
-    key->mComparator =
-        new txResultStringComparator(ascending, upperFirst, lang);
+    UniquePtr<txResultStringComparator> comparator =
+        MakeUnique<txResultStringComparator>(ascending, upperFirst);
+    rv = comparator->init(lang);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    key->mComparator = comparator.release();
   } else if (TX_StringEqualsAtom(dataType, nsGkAtoms::number)) {
     // Number comparator
     key->mComparator = new txResultNumberComparator(ascending);
@@ -134,7 +138,7 @@ nsresult txNodeSorter::sortNodeSet(txNodeSet* aNodes, txExecutionState* aEs,
 
   nsTArray<uint32_t> indexes(len.value());
   indexes.SetLengthAndRetainStorage(len.value());
-  nsTArray<txObject*> sortValues(numSortValues.value());
+  nsTArray<UniquePtr<txObject>> sortValues(numSortValues.value());
   sortValues.SetLengthAndRetainStorage(numSortValues.value());
   // txObject* has no null initializing constructor, so we init manually.
   memset(sortValues.Elements(), 0, sortValuesSize.value());
@@ -147,11 +151,7 @@ nsresult txNodeSorter::sortNodeSet(txNodeSet* aNodes, txExecutionState* aEs,
   auto nodeSetContext = MakeUnique<txNodeSetContext>(aNodes, aEs);
 
   // Sort the indexarray
-  SortData sortData{};
-  sortData.mNodeSorter = this;
-  sortData.mContext = nodeSetContext.get();
-  sortData.mSortValues = sortValues.Elements();
-  sortData.mRv = NS_OK;
+  SortData sortData{this, nodeSetContext.get(), sortValues.Elements(), NS_OK};
 
   aEs->pushEvalContext(nodeSetContext.release());
 
@@ -159,12 +159,6 @@ nsresult txNodeSorter::sortNodeSet(txNodeSet* aNodes, txExecutionState* aEs,
     return compareNodes(left, right, sortData);
   });
 
-  // Delete these here so we don't have to deal with them at every possible
-  // failurepoint
-  for (i = 0; i < numSortValues.value(); ++i) {
-    delete sortValues[i];
-  }
-
   if (NS_FAILED(sortData.mRv)) {
     // The txExecutionState owns the evalcontext so no need to handle it
     return sortData.mRv;
@@ -188,12 +182,10 @@ nsresult txNodeSorter::sortNodeSet(txNodeSet* aNodes, txExecutionState* aEs,
 
 int txNodeSorter::compareNodes(uint32_t aIndexA, uint32_t aIndexB,
                                SortData& aSortData) {
-  NS_ENSURE_SUCCESS(aSortData.mRv, -1);
-
   txListIterator iter(&aSortData.mNodeSorter->mSortKeys);
-  txObject** sortValuesA =
+  UniquePtr<txObject>* sortValuesA =
       aSortData.mSortValues + aIndexA * aSortData.mNodeSorter->mNKeys;
-  txObject** sortValuesB =
+  UniquePtr<txObject>* sortValuesB =
       aSortData.mSortValues + aIndexB * aSortData.mNodeSorter->mNKeys;
 
   unsigned int i;
@@ -201,19 +193,19 @@ int txNodeSorter::compareNodes(uint32_t aIndexA, uint32_t aIndexB,
   for (i = 0; i < aSortData.mNodeSorter->mNKeys; ++i) {
     SortKey* key = (SortKey*)iter.next();
     // Lazy create sort values
-    if (!sortValuesA[i] &&
-        !calcSortValue(sortValuesA[i], key, &aSortData, aIndexA)) {
-      return -1;
+    if (!sortValuesA[i]) {
+      sortValuesA[i] = calcSortValue(key, &aSortData, aIndexA);
     }
-    if (!sortValuesB[i] &&
-        !calcSortValue(sortValuesB[i], key, &aSortData, aIndexB)) {
-      return 1;
+    if (!sortValuesB[i]) {
+      sortValuesB[i] = calcSortValue(key, &aSortData, aIndexB);
     }
 
     // Compare node values
-    int compRes =
-        key->mComparator->compareValues(sortValuesA[i], sortValuesB[i]);
-    if (compRes != 0) return compRes;
+    int compRes = key->mComparator->compareValues(sortValuesA[i].get(),
+                                                  sortValuesB[i].get());
+    if (compRes != 0) {
+      return compRes;
+    }
   }
 
   // All keys have the same value for these nodes.
@@ -221,16 +213,18 @@ int txNodeSorter::compareNodes(uint32_t aIndexA, uint32_t aIndexB,
 }
 
 // static
-bool txNodeSorter::calcSortValue(txObject*& aSortValue, SortKey* aKey,
-                                 SortData* aSortData, uint32_t aNodeIndex) {
+UniquePtr<txObject> txNodeSorter::calcSortValue(SortKey* aKey,
+                                                SortData* aSortData,
+                                                uint32_t aNodeIndex) {
   aSortData->mContext->setPosition(aNodeIndex + 1);  // position is 1-based
 
-  nsresult rv = aKey->mComparator->createSortableValue(
-      aKey->mExpr, aSortData->mContext, aSortValue);
-  if (NS_FAILED(rv)) {
+  UniquePtr<txObject> sortValue;
+  nsresult rv;
+  std::tie(sortValue, rv) =
+      aKey->mComparator->createSortableValue(aKey->mExpr, aSortData->mContext);
+  if (NS_FAILED(rv) && NS_SUCCEEDED(aSortData->mRv)) {
+    // Record the first failure.
     aSortData->mRv = rv;
-    return false;
   }
-
-  return true;
+  return sortValue;
 }
diff --git a/dom/xslt/xslt/txNodeSorter.h b/dom/xslt/xslt/txNodeSorter.h
index b6c883b6ea5..3ab74763815 100644
--- a/dom/xslt/xslt/txNodeSorter.h
+++ b/dom/xslt/xslt/txNodeSorter.h
@@ -36,7 +36,7 @@ class txNodeSorter {
   struct SortData {
     txNodeSorter* mNodeSorter;
     txNodeSetContext* mContext;
-    txObject** mSortValues;
+    mozilla::UniquePtr<txObject>* mSortValues;
     nsresult mRv;
   };
   struct SortKey {
@@ -46,8 +46,9 @@ class txNodeSorter {
 
   static int compareNodes(uint32_t aIndexA, uint32_t aIndexB,
                           SortData& aSortData);
-  static bool calcSortValue(txObject*& aSortValue, SortKey* aKey,
-                            SortData* aSortData, uint32_t aNodeIndex);
+  static mozilla::UniquePtr<txObject> calcSortValue(SortKey* aKey,
+                                                    SortData* aSortData,
+                                                    uint32_t aNodeIndex);
   txList mSortKeys;
   unsigned int mNKeys;
 };
diff --git a/dom/xslt/xslt/txXPathResultComparator.cpp b/dom/xslt/xslt/txXPathResultComparator.cpp
index 470a55dc503..0331dac2913 100644
--- a/dom/xslt/xslt/txXPathResultComparator.cpp
+++ b/dom/xslt/xslt/txXPathResultComparator.cpp
@@ -19,13 +19,10 @@ using Collator = mozilla::intl::Collator;
 #define kUpperFirst (1 << 1)
 
 txResultStringComparator::txResultStringComparator(bool aAscending,
-                                                   bool aUpperFirst,
-                                                   const nsString& aLanguage) {
+                                                   bool aUpperFirst) {
   mSorting = 0;
   if (aAscending) mSorting |= kAscending;
   if (aUpperFirst) mSorting |= kUpperFirst;
-  nsresult rv = init(aLanguage);
-  if (NS_FAILED(rv)) NS_ERROR("Failed to initialize txResultStringComparator");
 }
 
 nsresult txResultStringComparator::init(const nsString& aLanguage) {
@@ -49,61 +46,35 @@ nsresult txResultStringComparator::init(const nsString& aLanguage) {
   return NS_OK;
 }
 
-nsresult txResultStringComparator::createSortableValue(Expr* aExpr,
-                                                       txIEvalContext* aContext,
-                                                       txObject*& aResult) {
-  UniquePtr<StringValue> val(new StringValue);
-
-  if (!mCollator) {
-    return NS_ERROR_FAILURE;
-  }
-
-  val->mString = MakeUnique<nsString>();
-  nsString& string = *val->mString;
-  nsresult rv = aExpr->evaluateToString(aContext, string);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  aResult = val.release();
-
-  return NS_OK;
+std::pair<UniquePtr<txObject>, nsresult>
+txResultStringComparator::createSortableValue(Expr* aExpr,
+                                              txIEvalContext* aContext) {
+  UniquePtr<nsString> string = MakeUnique<nsString>();
+  nsresult rv = aExpr->evaluateToString(aContext, *string);
+  return std::make_pair(MakeUnique<StringValue>(std::move(string)), rv);
 }
 
 int txResultStringComparator::compareValues(txObject* aVal1, txObject* aVal2) {
   nsString& dval1 = *((StringValue*)aVal1)->mString;
   nsString& dval2 = *((StringValue*)aVal2)->mString;
 
-  if (!mCollator) {
-    MOZ_ASSERT_UNREACHABLE("No mCollator");
-    return -1;
-  }
-
   int32_t result = mCollator->CompareStrings(dval1, dval2);
 
   return (mSorting & kAscending) ? result : -result;
 }
 
-txResultStringComparator::StringValue::StringValue() = default;
-
-txResultStringComparator::StringValue::~StringValue() = default;
-
 txResultNumberComparator::txResultNumberComparator(bool aAscending) {
   mAscending = aAscending ? 1 : -1;
 }
 
-nsresult txResultNumberComparator::createSortableValue(Expr* aExpr,
-                                                       txIEvalContext* aContext,
-                                                       txObject*& aResult) {
-  UniquePtr<NumberValue> numval(new NumberValue);
-
+std::pair<UniquePtr<txObject>, nsresult>
+txResultNumberComparator::createSortableValue(Expr* aExpr,
+                                              txIEvalContext* aContext) {
   RefPtr<txAExprResult> exprRes;
   nsresult rv = aExpr->evaluate(aContext, getter_AddRefs(exprRes));
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  numval->mVal = exprRes->numberValue();
-
-  aResult = numval.release();
-
-  return NS_OK;
+  return std::make_pair(
+      MakeUnique<NumberValue>(NS_SUCCEEDED(rv) ? exprRes->numberValue() : 0),
+      rv);
 }
 
 int txResultNumberComparator::compareValues(txObject* aVal1, txObject* aVal2) {
diff --git a/dom/xslt/xslt/txXPathResultComparator.h b/dom/xslt/xslt/txXPathResultComparator.h
index 165cc5f2edd..3ca547cbb4d 100644
--- a/dom/xslt/xslt/txXPathResultComparator.h
+++ b/dom/xslt/xslt/txXPathResultComparator.h
@@ -31,10 +31,11 @@ class txXPathResultComparator {
   virtual int compareValues(txObject* val1, txObject* val2) = 0;
 
   /*
-   * Create a sortable value.
+   * Create a sortable value. This always needs to return a value, but can
+   * indicate failure in the nsresult.
    */
-  virtual nsresult createSortableValue(Expr* aExpr, txIEvalContext* aContext,
-                                       txObject*& aResult) = 0;
+  virtual std::pair<mozilla::UniquePtr<txObject>, nsresult> createSortableValue(
+      Expr* aExpr, txIEvalContext* aContext) = 0;
 };
 
 /*
@@ -42,22 +43,21 @@ class txXPathResultComparator {
  */
 class txResultStringComparator : public txXPathResultComparator {
  public:
-  txResultStringComparator(bool aAscending, bool aUpperFirst,
-                           const nsString& aLanguage);
+  txResultStringComparator(bool aAscending, bool aUpperFirst);
+  nsresult init(const nsString& aLanguage);
 
   int compareValues(txObject* aVal1, txObject* aVal2) override;
-  nsresult createSortableValue(Expr* aExpr, txIEvalContext* aContext,
-                               txObject*& aResult) override;
+  std::pair<mozilla::UniquePtr<txObject>, nsresult> createSortableValue(
+      Expr* aExpr, txIEvalContext* aContext) override;
 
  private:
   mozilla::UniquePtr<const mozilla::intl::Collator> mCollator;
-  nsresult init(const nsString& aLanguage);
   int mSorting;
 
   class StringValue : public txObject {
    public:
-    StringValue();
-    ~StringValue();
+    explicit StringValue(mozilla::UniquePtr<nsString> aString)
+        : mString(std::move(aString)) {}
 
     mozilla::UniquePtr<nsString> mString;
   };
@@ -71,14 +71,16 @@ class txResultNumberComparator : public txXPathResultComparator {
   explicit txResultNumberComparator(bool aAscending);
 
   int compareValues(txObject* aVal1, txObject* aVal2) override;
-  nsresult createSortableValue(Expr* aExpr, txIEvalContext* aContext,
-                               txObject*& aResult) override;
+  std::pair<mozilla::UniquePtr<txObject>, nsresult> createSortableValue(
+      Expr* aExpr, txIEvalContext* aContext) override;
 
  private:
   int mAscending;
 
   class NumberValue : public txObject {
    public:
+    explicit NumberValue(double aVal) : mVal(aVal) {}
+
     double mVal;
   };
 };

Loading diff…

References

Vendor advisory Fix commit Bug tracker NVD

On This Page