[php-src] PHP-8.5: Fix GH-23331: UAF when an attribute child keeps a live wrapper

Author: Ilia Alshanetsky (iliaal)
Date: 2026-08-18T14:25:05-04:00

Commit: https://github.com/php/php-src/commit/d19f248f8efccb58aabbcc35fe5ce921abb89ae6
Raw diff: https://github.com/php/php-src/commit/d19f248f8efccb58aabbcc35fe5ce921abb89ae6.diff

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

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 GH-23331
Closes GH-23337

Changed paths:
  A  ext/dom/tests/gh23331.phpt
  A  ext/dom/tests/gh23331_2.phpt
  A  ext/dom/tests/gh23331_3.phpt
  M  NEWS
  M  ext/dom/php_dom.c


Diff:

diff --git a/NEWS b/NEWS
index f65c07cf4a7e..182c80517b04 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,8 @@ PHP                                                                        NEWS
 - DOM:
   . Fixed a use-after-free when cloning a DOMNameSpaceNode after
     DOMDocument::xinclude(). (iliaal)
+  . Fixed bug GH-23331 (UAF when node_list_unlink() skips attribute children
+    that still have a live wrapper). (iliaal)
 
 - Intl:
   . Fixed a double-free when IntlGregorianCalendar construction fails after
diff --git a/ext/dom/php_dom.c b/ext/dom/php_dom.c
index b19c3327419d..7bc99e68794c 100644
--- a/ext/dom/php_dom.c
+++ b/ext/dom/php_dom.c
@@ -1437,14 +1437,13 @@ void node_list_unlink(xmlNodePtr node)
 	dom_object *wrapper;
 
 	while (node != NULL) {
+		xmlNodePtr next = node->next;
 
 		wrapper = php_dom_object_get_data(node);
 
 		if (wrapper != NULL ) {
 			xmlUnlinkNode(node);
-		} else {
-			if (node->type == XML_ENTITY_REF_NODE)
-				break;
+		} else if (node->type != XML_ENTITY_REF_NODE) {
 			node_list_unlink(node->children);
 
 			switch (node->type) {
@@ -1461,7 +1460,7 @@ void node_list_unlink(xmlNodePtr node)
 
 		}
 
-		node = node->next;
+		node = next;
 	}
 }
 /* }}} end node_list_unlink */
diff --git a/ext/dom/tests/gh23331.phpt b/ext/dom/tests/gh23331.phpt
new file mode 100644
index 000000000000..8c193b845115
--- /dev/null
+++ b/ext/dom/tests/gh23331.phpt
@@ -0,0 +1,37 @@
+--TEST--
+GH-23331 (Use-after-free when an attribute child past an entity reference keeps a live wrapper)
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+
+$doc = new DOMDocument();
+$doc->loadXML('<!DOCTYPE root [<!ENTITY e "X">]><root attr="a&e;b"/>');
+$attr = $doc->documentElement->getAttributeNode('attr');
+$first = $attr->firstChild;
+$entity = $attr->childNodes[1];
+$last = $attr->lastChild;
+
+$doc->documentElement->setAttribute('attr', 'updated');
+
+echo "text before the entity reference: ";
+var_dump($first->textContent);
+echo "entity reference name: ";
+var_dump($entity->nodeName);
+echo "entity reference detached: ";
+var_dump($entity->parentNode === null);
+echo "text after the entity reference: ";
+var_dump($last->textContent);
+echo "detached from the attribute: ";
+var_dump($last->parentNode === null);
+echo "new attribute value: ";
+var_dump($doc->documentElement->getAttribute('attr'));
+
+?>
+--EXPECT--
+text before the entity reference: string(1) "a"
+entity reference name: string(1) "e"
+entity reference detached: bool(true)
+text after the entity reference: string(1) "b"
+detached from the attribute: bool(true)
+new attribute value: string(7) "updated"
diff --git a/ext/dom/tests/gh23331_2.phpt b/ext/dom/tests/gh23331_2.phpt
new file mode 100644
index 000000000000..1e0bb92e8113
--- /dev/null
+++ b/ext/dom/tests/gh23331_2.phpt
@@ -0,0 +1,34 @@
+--TEST--
+GH-23331 (Use-after-free when an attribute child past an entity reference keeps a live wrapper) - setAttributeNS() and removeAttribute()
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+
+$doc = new DOMDocument();
+$doc->loadXML('<!DOCTYPE root [<!ENTITY e "X">]><root xmlns:p="urn:x" p:attr="a&e;b"/>');
+$attr = $doc->documentElement->getAttributeNodeNS('urn:x', 'attr');
+$last = $attr->lastChild;
+$doc->documentElement->setAttributeNS('urn:x', 'p:attr', 'updated');
+echo "setAttributeNS, detached: ";
+var_dump($last->parentNode === null);
+echo "setAttributeNS, text: ";
+var_dump($last->textContent);
+
+$doc = new DOMDocument();
+$doc->loadXML('<!DOCTYPE root [<!ENTITY e "X">]><root attr="a&e;b"/>');
+$attr = $doc->documentElement->getAttributeNode('attr');
+$last = $attr->lastChild;
+unset($attr);
+$doc->documentElement->removeAttribute('attr');
+echo "removeAttribute, no wrapper on the attribute, detached: ";
+var_dump($last->parentNode === null);
+echo "removeAttribute, no wrapper on the attribute, text: ";
+var_dump($last->textContent);
+
+?>
+--EXPECT--
+setAttributeNS, detached: bool(true)
+setAttributeNS, text: string(1) "b"
+removeAttribute, no wrapper on the attribute, detached: bool(true)
+removeAttribute, no wrapper on the attribute, text: string(1) "b"
diff --git a/ext/dom/tests/gh23331_3.phpt b/ext/dom/tests/gh23331_3.phpt
new file mode 100644
index 000000000000..42125615ae12
--- /dev/null
+++ b/ext/dom/tests/gh23331_3.phpt
@@ -0,0 +1,41 @@
+--TEST--
+GH-23331 (Use-after-free when an attribute child past an entity reference keeps a live wrapper) - Dom\XMLDocument
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+
+$xml = '<!DOCTYPE root [<!ENTITY e "X">]><root xmlns:p="urn:x" attr="a&e;b" p:nsattr="c&e;d"/>';
+
+$doc = Dom\XMLDocument::createFromString($xml);
+$el = $doc->documentElement;
+$attr = $el->getAttributeNode('attr');
+$first = $attr->firstChild;
+$last = $attr->lastChild;
+unset($attr);
+$el->removeAttribute('attr');
+echo "removeAttribute, first: ";
+var_dump($first->textContent);
+echo "removeAttribute, detached: ";
+var_dump($last->parentNode === null);
+echo "removeAttribute, text: ";
+var_dump($last->textContent);
+
+$doc = Dom\XMLDocument::createFromString($xml);
+$el = $doc->documentElement;
+$attr = $el->getAttributeNodeNS('urn:x', 'nsattr');
+$last = $attr->lastChild;
+unset($attr);
+$el->removeAttributeNS('urn:x', 'nsattr');
+echo "removeAttributeNS, detached: ";
+var_dump($last->parentNode === null);
+echo "removeAttributeNS, text: ";
+var_dump($last->textContent);
+
+?>
+--EXPECT--
+removeAttribute, first: string(1) "a"
+removeAttribute, detached: bool(true)
+removeAttribute, text: string(1) "b"
+removeAttributeNS, detached: bool(true)
+removeAttributeNS, text: string(1) "d"