CVE-2025-4087
Overview
Background
- `AttrNameAt(uint32_t)`
- The raw accessor; its own comment says it is “not out-of-bounds safe” — it returns
&mImpl->mBuffer[aPos].mNamewith no bounds check. - `GetSafeAttrNameAt(uint32_t)` / `Element::GetAttrNameAt(uint32_t)`
- The “safe” accessor used by loop callers. Pre-patch it returned a pointer or
nullptrwhenaPos >= AttrCount(), and callers walked attributes by incrementing an index until they gotnullptr. - XPath attribute walk
txXPathTreeWalker::moveToNamedAttributeiterates an element’s attributes with such an index loop to resolve an XPath attribute test; the attribute count/index is influenced by document content.
Root Cause Analysis
The attribute-index API relied on a nullptr sentinel to signal out-of-bounds, and that contract was fragile: callers that did not (or could not reliably) null-check the returned pointer before dereferencing it would read past the valid attribute range. The patch replaces the ambiguous single accessor with a two-form, type-enforced API. AttrArray::GetSafeAttrNameAt is split into a [[nodiscard]] bool GetSafeAttrNameAt(uint32_t aPos, const nsAttrName** aResult) that writes the result and returns true only when aPos < AttrCount(), plus a pointer-returning overload that now MOZ_CRASH("aPos out of bounds") instead of silently returning nullptr. Element gains a matching [[nodiscard]] bool GetAttrNameAt(uint32_t, const nsAttrName**) overload, and Element::GetAttrNameAt(uint32_t) is documented to crash on an invalid index.
Every index-loop caller is rewritten to the fallible form so the bounds result is impossible to ignore: nsINode::CompareDocumentPosition, nsTreeSanitizer::RemoveAllAttributes, nsDOMAttributeMap::IndexedGetter, and crucially the XPath walker txMozillaXPathTreeWalker.cpp’s moveToNamedAttribute all change from for (i = 0; (name = GetAttrNameAt(i)); ++i) to for (i = 0; GetAttrNameAt(i, &name); ++i). The [[nodiscard]] attribute makes any future caller that drops the success flag a compile-time error, closing the class of missing-null-check bugs the advisory describes. The advisory frames this as an out-of-bounds read reachable through XPath parsing; the diff substantiates that by hardening the exact attribute-indexing accessor the XPath walker uses, though the precise pre-patch sink that dereferenced an out-of-range mBuffer[aPos] is not shown line-for-line in this diff (inferred to be an unchecked use of the pointer-returning accessor / the unsafe AttrNameAt).
nullptr return value that a caller could forget to check; the fix makes bounds-safety unavoidable — a [[nodiscard]] bool fallible form plus a crash-on-OOB pointer form — so an out-of-range index can no longer be silently dereferenced.Attack Path
- Attacker
Serves a document (HTML/XML) with elements whose attributes are exercised by an XPath expression, or script that runs
document.evaluate(...)with an attribute node test. (web-reachable, inferred from the XPath walker change) - XPath evaluation
txXPathTreeWalker::moveToNamedAttributewalks the element’s attributes by incrementing indexi. - OOB index
The index loop advances to or past
AttrCount(); the pre-patch sentinel API allowed the returned pointer to be used without a reliable bounds check. - OOB read
mImpl->mBuffer[aPos].mNameis read for an out-of-rangeaPos, dereferencing attacker-adjacent heap memory as annsAttrName. - Primitive Out-of-bounds read of heap memory interpreted as attribute-name structure, potentially usable for information disclosure or as a step toward further memory corruption (escalation inferred, not shown by the diff).
Impact Assessment
MOZ_CRASH) converts a formerly silent OOB into a controlled abort for any path that still hits the infallible accessor.Files Changed
dom/base/AttrArray.cppdom/base/AttrArray.hdom/base/Element.hdom/base/nsDOMAttributeMap.cppdom/base/nsINode.cppdom/base/nsTreeSanitizer.cppdom/xslt/xpath/txMozillaXPathTreeWalker.cpp
Audit Directions
- Remaining users of unsafe `AttrNameAt`
AttrNameAt(uint32_t)is explicitly not bounds-checked; audit all its callers to ensure the index is provably in range. - Other sentinel-null index APIsLook for other DOM/collection accessors that signal OOB by returning
nullptrand whose callers dereference without checking; candidates for the same[[nodiscard]] booltreatment. - XPath/XSLT index walksReview other
txXPathTreeWalker/XSLT iterators that drive indices from document content for similar missing bounds handling.
Patch
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;