Fix GH-23331: UAF when an attribute child keeps a live wrapper - #23337
Open
iliaal wants to merge 1 commit into
Open
Fix GH-23331: UAF when an attribute child keeps a live wrapper#23337iliaal wants to merge 1 commit into
iliaal wants to merge 1 commit into
Conversation
devnexen
reviewed
Aug 17, 2026
| dom | ||
| --FILE-- | ||
| <?php | ||
|
|
Member
There was a problem hiding this comment.
Adding a case for Dom\XMLDocument would be nice
Contributor
Author
There was a problem hiding this comment.
Added gh23331_3.phpt: removeAttribute() and removeAttributeNS() on Dom\XMLDocument, both segfault pre-patch. Modern setAttribute() goes via dom_remove_all_children() so it was never affected. Modern setAttributeNS() never reaches node_list_unlink() at all, separate defect, own PR.
node_list_unlink() detaches attribute children that still have a PHP wrapper so libxml2 does not free them underneath it, but two exits abandoned the rest of the list: xmlUnlinkNode() clears node->next, and the XML_ENTITY_REF_NODE case broke out of the loop instead of only skipping the borrowed entity children. Anything past either exit was freed by xmlSetProp(), xmlSetNsProp() or xmlFreeProp() with a live wrapper still pointing at it. Fixes phpGH-23331
iliaal
force-pushed
the
fix/gh-23331-dom-attr-child-uaf
branch
from
August 17, 2026 23:43
ec8556a to
4b871e3
Compare
devnexen
approved these changes
Aug 18, 2026
devnexen
approved these changes
Aug 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
node_list_unlink() abandoned an attribute's remaining children at two exits: xmlUnlinkNode() clears node->next, so the loop stopped at the first child that had a wrapper, and the XML_ENTITY_REF_NODE case broke out of the loop instead of only skipping the borrowed entity children. xmlSetProp(), xmlSetNsProp() and xmlFreeProp() then freed everything past either exit while a live wrapper still pointed at it.
The #37456 recursion guard is kept: entity-reference children are still never walked. Every caller passes an attribute's or an element's child list rather than a DTD list, so continuing the sibling walk stays inside a finite content list.
On the legacy classes setAttribute(), setAttributeNS(), removeAttribute(), removeAttributeNode() and removeAttributeNS() all reach it. On the modern classes only the three removal methods do: Dom\Element::setAttribute() replaces the value through dom_remove_all_children(), which is unaffected because php_libxml_node_free_list() already caches the sibling before unlinking, and Dom\Element::setAttributeNS() hands the attribute straight to xmlSetNsProp() without unlinking anything, so it frees a wrapped child unconditionally. That last one is an independent defect and gets its own PR.
The removal methods reach it only when the attribute itself carries no wrapper, where dom_remove_attribute() falls through to xmlFreeProp(); that case segfaults rather than reading freed memory. The wrapper positions decide which exit fires rather than the caller, so gh23331.phpt pins the first and gh23331_2.phpt the second; gh23331_3.phpt covers the modern classes.