Medium firefox OOB 🔧 Commit mapped

Overview

Medium
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
Impactmoderate
DescriptionA vulnerability was identified in Firefox where XPath parsing could trigger undefined behavior due to missing null checks during attribute access. This could lead to out-of-bounds read access and potentially, memory corruption.
ComponentDOM
Bug ClassOOB
Tracker1952465
Fix commitd656ba879fc0 (firefox) +37/-11
CISA KEVNot listed
CreditedIvan Fratric
Disclosed2025-04-29

Files Changed

  • dom/base/AttrArray.cpp
  • dom/base/AttrArray.h
  • dom/base/Element.h
  • dom/base/nsDOMAttributeMap.cpp
  • dom/base/nsINode.cpp
  • dom/base/nsTreeSanitizer.cpp
  • dom/xslt/xpath/txMozillaXPathTreeWalker.cpp
diff --git a/dom/base/AttrArray.cpp b/dom/base/AttrArray.cpp
index 929ec8da8c3..517ec90de57 100644
--- a/dom/base/AttrArray.cpp
+++ b/dom/base/AttrArray.cpp
@@ -185,11 +185,21 @@ const nsAttrName* AttrArray::AttrNameAt(uint32_t aPos) const {
   return &mImpl->mBuffer[aPos].mName;
 }
 
-const nsAttrName* AttrArray::GetSafeAttrNameAt(uint32_t aPos) const {
+[[nodiscard]] bool AttrArray::GetSafeAttrNameAt(
+    uint32_t aPos, const nsAttrName** aResult) const {
   if (aPos >= AttrCount()) {
-    return nullptr;
+    return false;
   }
-  return &mImpl->mBuffer[aPos].mName;
+  *aResult = &mImpl->mBuffer[aPos].mName;
+  return true;
+}
+
+const nsAttrName* AttrArray::GetSafeAttrNameAt(uint32_t aPos) const {
+  const nsAttrName* name;
+  if (!GetSafeAttrNameAt(aPos, &name)) {
+    MOZ_CRASH("aPos out of bounds");
+  }
+  return name;
 }
 
 const nsAttrName* AttrArray::GetExistingAttrNameFromQName(
diff --git a/dom/base/AttrArray.h b/dom/base/AttrArray.h
index fbbd2e58257..8d0d27df202 100644
--- a/dom/base/AttrArray.h
+++ b/dom/base/AttrArray.h
@@ -83,7 +83,13 @@ class AttrArray {
   // Returns the attribute info at a given position, *not* out-of-bounds safe
   BorrowedAttrInfo AttrInfoAt(uint32_t aPos) const;
 
-  // Returns attribute name at given position or null if aPos is out-of-bounds
+  // If aPos is in bounds, set aResult to the attribute at the given position
+  // without AddRefing it and return true. Otherwise, return false.
+  [[nodiscard]] bool GetSafeAttrNameAt(uint32_t aPos,
+                                       const nsAttrName** aResult) const;
+
+  // If aPos is in bounds, return the attribute at the given position.
+  // Otherwise, crash.
   const nsAttrName* GetSafeAttrNameAt(uint32_t aPos) const;
 
   const nsAttrName* GetExistingAttrNameFromQName(const nsAString& aName) const;
diff --git a/dom/base/Element.h b/dom/base/Element.h
index 7a150ec4d42..a5b1219ba91 100644
--- a/dom/base/Element.h
+++ b/dom/base/Element.h
@@ -1070,12 +1070,12 @@ class Element : public FragmentOrElement {
    * Get the namespace / name / prefix of a given attribute.
    *
    * @param   aIndex the index of the attribute name
-   * @returns The name at the given index, or null if the index is
-   *          out-of-bounds.
+   * @returns The name at the given index.
    * @note    The document returned by NodeInfo()->GetDocument() (if one is
    *          present) is *not* necessarily the owner document of the element.
    * @note    The pointer returned by this function is only valid until the
    *          next call of either GetAttrNameAt or SetAttr on the element.
+   * @note    This will crash if the index is invalid.
    */
   const nsAttrName* GetAttrNameAt(uint32_t aIndex) const {
     return mAttrs.GetSafeAttrNameAt(aIndex);
@@ -1088,6 +1088,14 @@ class Element : public FragmentOrElement {
     return mAttrs.AttrNameAt(aIndex);
   }
 
+  /**
+   * A fallible overload of GetAttrNameAt.
+   */
+  [[nodiscard]] bool GetAttrNameAt(uint32_t aIndex,
+                                   const nsAttrName** aResult) const {
+    return mAttrs.GetSafeAttrNameAt(aIndex, aResult);
+  }
+
   /**
    * Gets the attribute info (name and value) for this element at a given index.
    */
diff --git a/dom/base/nsDOMAttributeMap.cpp b/dom/base/nsDOMAttributeMap.cpp
index f0d39c52e6c..7d1601e9641 100644
--- a/dom/base/nsDOMAttributeMap.cpp
+++ b/dom/base/nsDOMAttributeMap.cpp
@@ -314,8 +314,10 @@ Attr* nsDOMAttributeMap::IndexedGetter(uint32_t aIndex, bool& aFound) {
   aFound = false;
   NS_ENSURE_TRUE(mContent, nullptr);
 
-  const nsAttrName* name = mContent->GetAttrNameAt(aIndex);
-  NS_ENSURE_TRUE(name, nullptr);
+  const nsAttrName* name;
+  if (!mContent->GetAttrNameAt(aIndex, &name)) {
+    return nullptr;
+  }
 
   aFound = true;
   // Don't use the nodeinfo even if one exists since it can have the wrong
diff --git a/dom/base/nsINode.cpp b/dom/base/nsINode.cpp
index 4d30b5f97fb..7969cbd9f6b 100644
--- a/dom/base/nsINode.cpp
+++ b/dom/base/nsINode.cpp
@@ -1201,7 +1201,7 @@ uint16_t nsINode::CompareDocumentPosition(nsINode& aOtherNode,
 
       uint32_t i;
       const nsAttrName* attrName;
-      for (i = 0; (attrName = elem->GetAttrNameAt(i)); ++i) {
+      for (i = 0; elem->GetAttrNameAt(i, &attrName); ++i) {
         if (attrName->Equals(attr1->NodeInfo())) {
           NS_ASSERTION(!attrName->Equals(attr2->NodeInfo()),
                        "Different attrs at same position");
diff --git a/dom/base/nsTreeSanitizer.cpp b/dom/base/nsTreeSanitizer.cpp
index bdefc878816..e5d3dd402d1 100644
--- a/dom/base/nsTreeSanitizer.cpp
+++ b/dom/base/nsTreeSanitizer.cpp
@@ -1483,7 +1483,7 @@ void nsTreeSanitizer::SanitizeChildren(nsINode* aRoot) {
 
 void nsTreeSanitizer::RemoveAllAttributes(Element* aElement) {
   const nsAttrName* attrName;
-  while ((attrName = aElement->GetAttrNameAt(0))) {
+  while (aElement->GetAttrNameAt(0, &attrName)) {
     int32_t attrNs = attrName->NamespaceID();
     RefPtr<nsAtom> attrLocal = attrName->LocalName();
     aElement->UnsetAttr(attrNs, attrLocal, false);
diff --git a/dom/xslt/xpath/txMozillaXPathTreeWalker.cpp b/dom/xslt/xpath/txMozillaXPathTreeWalker.cpp
index 6fdd33f0553..0db2e5cb693 100644
--- a/dom/xslt/xpath/txMozillaXPathTreeWalker.cpp
+++ b/dom/xslt/xpath/txMozillaXPathTreeWalker.cpp
@@ -134,7 +134,7 @@ bool txXPathTreeWalker::moveToNamedAttribute(nsAtom* aLocalName,
 
   const nsAttrName* name;
   uint32_t i;
-  for (i = 0; (name = element->GetAttrNameAt(i)); ++i) {
+  for (i = 0; element->GetAttrNameAt(i, &name); ++i) {
     if (name->Equals(aLocalName, aNSID)) {
       mPosition.mIndex = i;
 
Loading diff…