1952465 - (CVE-2025-4087) Firefox: txMozillaXPathTreeWalker doesn't check for nullptr, causing bounds check to be optimized away

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Ivan Fratric]

Attachment: nullptr.py

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/134.0.0.0 Safari/537.36

Steps to reproduce:

In various places, txMozillaXPathTreeWalker calls Element::GetAttrNameAt with a txXPathNode index as a param. One example of this is txXPathNodeUtils::getNodeName:

void txXPathNodeUtils::getNodeName(const txXPathNode& aNode, nsAString& aName) {

...

  aNode.Content()
      ->AsElement()
      ->GetAttrNameAt(aNode.mIndex)
      ->GetQualifiedName(aName);
}

Element::GetAttrNameAt calls AttrArray::GetSafeAttrNameAt which checks if the index is within bounds and if not, returns nullptr. Since txXPathNodeUtils::getNodeName (and other similar functions in txMozillaXPathTreeWalker.cpp) don't check for nullptr, and null deref is an undefined behavior in C++, the compiler ends up removing the bounds check when inlining GetAttrNameAt into various xpath functions.

As evidence of this, here is the decompiled code from txXPathNodeUtils::getNodeName in linux64 release build of Firefox 136.0:

   0x7fa1f06fe980 <getNodeName>:	mov    eax,esi
   0x7fa1f06fe982 <getNodeName+2>:	and    eax,0xfffffffe
   0x7fa1f06fe985 <getNodeName+5>:	cmp    eax,0x7ffffffe
   0x7fa1f06fe98a <getNodeName+10>:	je     0x7fa1f06fe998 <getNodeName+24>
   0x7fa1f06fe98c <getNodeName+12>:	neg    eax
   0x7fa1f06fe98e <getNodeName+14>:	jno    0x7fa1f06fe9b5 <getNodeName+53>
   0x7fa1f06fe990 <getNodeName+16>:	mov    rdi,rdx
   0x7fa1f06fe993 <getNodeName+19>:	jmp    0x7fa1ecf94b60 <_ZN12nsTSubstringIDsE8TruncateEv>
   0x7fa1f06fe998 <getNodeName+24>:	test   BYTE PTR [rdi+0x1c],0x10
   0x7fa1f06fe99c <getNodeName+28>:	mov    rsi,QWORD PTR [rdi+0x28]
   0x7fa1f06fe9a0 <getNodeName+32>:	jne    0x7fa1f06fe9a9 <getNodeName+41>
   0x7fa1f06fe9a2 <getNodeName+34>:	cmp    WORD PTR [rsi+0x24],0x7
   0x7fa1f06fe9a7 <getNodeName+39>:	jne    0x7fa1f06fe990 <getNodeName+16>
   0x7fa1f06fe9a9 <getNodeName+41>:	add    rsi,0x58
   0x7fa1f06fe9ad <getNodeName+45>:	mov    rdi,rdx
   0x7fa1f06fe9b0 <getNodeName+48>:	jmp    0x7fa1eca27fe0 <_ZN12nsTSubstringIDsE6AssignERKS0_>
   0x7fa1f06fe9b5 <getNodeName+53>:	shr    esi,1
   0x7fa1f06fe9b7 <getNodeName+55>:	mov    rax,QWORD PTR [rdi+0x78]
   0x7fa1f06fe9bb <getNodeName+59>:	shl    rsi,0x4
   0x7fa1f06fe9bf <getNodeName+63>:	mov    rsi,QWORD PTR [rax+rsi*1+0x10]
   0x7fa1f06fe9c4 <getNodeName+68>:	test   sil,0x1
   0x7fa1f06fe9c8 <getNodeName+72>:	jne    0x7fa1f06fe9d5 <getNodeName+85>
   0x7fa1f06fe9ca <getNodeName+74>:	mov    rdi,rsi
   0x7fa1f06fe9cd <getNodeName+77>:	mov    rsi,rdx
   0x7fa1f06fe9d0 <getNodeName+80>:	jmp    0x7fa1ed260ae0 <_ZNK6nsAtom8ToStringER12nsTSubstringIDsE>
   0x7fa1f06fe9d5 <getNodeName+85>:	and    rsi,0xfffffffffffffffe
   0x7fa1f06fe9d9 <getNodeName+89>:	add    rsi,0x48
   0x7fa1f06fe9dd <getNodeName+93>:	jmp    0x7fa1f06fe9ad <getNodeName+45>

Array access happens at mov rsi,QWORD PTR [rax+rsi*1+0x10] and, most notably, there is no prior check that rsi is within bounds.

You might think that mIndex in those calls to GetAttrNameAt is always going to be within bounds. However, this changes with bug 1941002. It's still possible (even after the patches in bug 1941002), to interrupt xslt transofrmation and execute JavaScript on the same thread / context. Such JavaScript code can then modify and remove attributes from a node, causing mIndex to no longer be correct and potentially pointing out of bounds.

I am attaching a proof of concept for being able to modify attributes during xslt transformation. This PoC should cause a null pointer crash on the debug / asan builds (for me this includes a release asan builds), but not on the actual optimized release builds. The ASAN log can be found at the end of this report.

One thing that saves this bug from becoming exploitable in optimized builds is that, looking at the implementation of AttrArray, the mBuffer can never shrink in size. It can only grow, making mIndex always stay within bounds. However, if in the future some unrelated memory optimization is made that makes AttrArray buffer shrink during use (such as calling AttrArray::Compact from another place that attacker can reach), then this bug will become a potentially exploitable out-of-bounds read.

Besides adding an explicit nullptr check, this type of issues can be broadly mitigated by always compiling Firefox with -fno-delete-null-pointer-checks.

Because the bug is not immediately exploitable, I am reporting it as an FYI without applying any disclosure deadline.

null deref ASAN log:

=================================================================
==4097182==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fc50407e45f bp 0x7fffbd4e1cf0 sp 0x7fffbd4e1ce0 T0)
==4097182==The signal is caused by a READ memory access.
==4097182==Hint: address points to the zero page.
    #0 0x7fc50407e45f in IsAtom objdir-ff-asan/dist/include/nsAttrName.h:67:34
    #1 0x7fc50407e45f in GetQualifiedName objdir-ff-asan/dist/include/nsAttrName.h:131:9
    #2 0x7fc50407e45f in txXPathNodeUtils::getNodeName(txXPathNode const&, nsTSubstring<char16_t>&) dom/xslt/xpath/txMozillaXPathTreeWalker.cpp:349:9
    #3 0x7fc50407b607 in txCoreFunctionCall::evaluate(txIEvalContext*, txAExprResult**) dom/xslt/xpath/txCoreFunctionCall.cpp:182:13
    #4 0x7fc50407f3c3 in Expr::evaluateToString(txIEvalContext*, nsTString<char16_t>&) dom/xslt/xpath/txExpr.cpp:20:17
    #5 0x7fc5040790de in txCoreFunctionCall::evaluate(txIEvalContext*, txAExprResult**) dom/xslt/xpath/txCoreFunctionCall.cpp:209:26
    #6 0x7fc5040be64e in txLREAttribute::execute(txExecutionState&) dom/xslt/xslt/txInstructions.cpp:427:25
    #7 0x7fc5041183a2 in txXSLTProcessor::execute(txExecutionState&) dom/xslt/xslt/txXSLTProcessor.cpp:46:17
    #8 0x7fc5040e123d in txMozillaXSLTProcessor::TransformToFragment(nsINode&, bool, mozilla::dom::Document&, mozilla::ErrorResult&) dom/xslt/xslt/txMozillaXSLTProcessor.cpp:807:10
    #9 0x7fc4fefbc56c in TransformToFragment objdir-ff-asan/dist/include/mozilla/dom/txMozillaXSLTProcessor.h:99:12
    #10 0x7fc4fefbc56c in mozilla::dom::XSLTProcessor_Binding::transformToFragment(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) objdir-ff-asan/dom/bindings/./XSLTProcessorBinding.cpp:1055:83
    #11 0x7fc4ff91d8ca in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) dom/bindings/BindingUtils.cpp:3302:13
    #12 0x7fc506e0e4ff in CallJSNative js/src/vm/Interpreter.cpp:493:13
    #13 0x7fc506e0e4ff in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) js/src/vm/Interpreter.cpp:589:12
    #14 0x7fc507f4fe76 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:1705:10
    #15 0x378695523873  (<unknown module>)

==4097182==Register values:
rax = 0x0000000000000000  rbx = 0x000050400005ee28  rcx = 0x0000000000000000  rdx = 0x0000000000000000  
rdi = 0x000051d0000a8c80  rsi = 0x0000000000000062  rbp = 0x00007fffbd4e1cf0  rsp = 0x00007fffbd4e1ce0  
 r8 = 0x0000000000000000   r9 = 0x00007fffffffff01  r10 = 0x00007fffffffff01  r11 = 0x8005e5739171b001  
r12 = 0x000050700004a460  r13 = 0x00005030000a9060  r14 = 0x00007fffbd4e20a0  r15 = 0x00005060000f9b00  
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV objdir-ff-asan/dist/include/nsAttrName.h:67:34 in IsAtom
==4097182==ABORTING

Comment 1

[Andrew McCreight [:mccr8]]

I read over comment 0 in more detail. I failed to notice that it says this isn't exploitable because AttrArray can't shrink, so indices can't become invalid, so I'll drop it to sec-moderate. That being said, I think this issue applies to every caller of GetAttrNameAt that doesn't do a null check, so there could be more serious problems. I checked a couple and none of them were doing checks. I think we need to switch to a safe-by-default API.

Comment 2

[Andrew McCreight [:mccr8]]

I talked to Peter and I can take this.

Comment 3

[Edgar Chen [:edgar]]

(S3 per sec-moderate, if you think this should be a S2, feel free to upgrade it)

Comment 4

[Andrew McCreight [:mccr8]]

I looked at the 30 call sites for Element::GetAttrNameAt (plus one separate direct call to AttrArray::GetSafeAttrNameAt). Only a few did a proper null check:

1. nsDOMAttributeMap::IndexedGetter
2. nsTreeSanitizer::RemoveAllAttributes
3. txXPathTreeWalker::moveToNamedAttribute
4. nsINode::CompareDocumentPosition

txMozillaXPathTreeWalker does this slightly dodgy thing where it tucks away the indices into a field txXPathNode::mIndex, with magic values for documents and elements that are UINT32_MAX and UINT32_MAX - 1 (side note: what is the maximum number of attrs...), so if somebody forgets to check things properly that could go wrong.

Anyways, the rest of the callers just iterate over all of the attributes, so they should be fine thanks to the property mentioned above that we never shrink the attr array. We do remove things from it, but this is done by calling the destructor on the last element of the array so that's probably fine.

My proposal to fix this is to make the current function crash if the index is out of bounds. For the 4 places where we want it to be fallible, I'll add a new method that has a [[nodiscard]] bool return value and passes the result pointer to an argument. I think this even makes 3 of the places less awkward.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1952465 - Improve GetAttrNameAt API.

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to Andrew McCreight [:mccr8] from comment #4)

(side note: what is the maximum number of attrs...), so if somebody forgets to check things properly that could go wrong.

The size in bytes for the attr array is capped below UINT32_MAX (see https://searchfox.org/mozilla-central/rev/1a7d78fdc729359ea43c65d647879c32bcdfb852/dom/base/AttrArray.cpp#316-317,322), given that attributes have both a name and a value and both contain a uintpt_t, the maximum number of attribrutes should be much lower.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/554b26083dae
Improve GetAttrNameAt API. r=smaug

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/554b26083dae

Comment 9

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:mccr8, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox137 to wontfix.

For more information, please visit BugBot documentation.

Comment 10

[Andrew McCreight [:mccr8]]

I'm concerned about performance regressions here so I want to wait at least a few days. It isn't a huge deal if this misses beta.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Let's just let it bake another cycle then if we're not in a rush. Leaving the NI for an eventual ESR128 uplift request, though.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: Bug 1952465 - Improve GetAttrNameAt API.

Original Revision: https://phabricator.services.mozilla.com/D241895

Comment 13

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: potential security problems
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: none
• Risk associated with taking this patch: low
• Explanation of risk level: this only makes us crash in a bad situation; it has been on Nightly already for a week or so
• String changes made/needed: none
• Is Android affected?: yes

Comment 14

[Andrew McCreight [:mccr8]]

Looks like there were no performance or stability regressions filed, so I've nominated this for uplift.

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/d8034a31653e

Comment 16

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt