Skip to content

Fix GH-23331: UAF when an attribute child keeps a live wrapper - #23337

Open
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/gh-23331-dom-attr-child-uaf
Open

Fix GH-23331: UAF when an attribute child keeps a live wrapper#23337
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/gh-23331-dom-attr-child-uaf

Conversation

@iliaal

@iliaal iliaal commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

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.

dom
--FILE--
<?php

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a case for Dom\XMLDocument would be nice

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants