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

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)
 
 - Opcache:
   . Fixed opcache.protect_memory race under ZTS. (realFlowControl)

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 */

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"

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"

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"