Fix UAF in reflected attribute write paths

NEWS

@@ -67,6 +67,8 @@ PHP                                                                        NEWS
     NULL deref on first use after a failed open). (Girgias)
 
 - DOM:
+  . Fixed a use-after-free when reflected attribute setters replace an
+    attribute whose child still has a live wrapper. (Jakub Zelenka)
   . Fixed bug GH-23352 (UAF reading an attribute value node retained across
     DOMDocument::adoptNode()). (David Carlier)
   . Fixed bug GH-24260 (querySelectorAll() returns duplicate elements for

ext/dom/element.c

@@ -156,7 +156,7 @@ static xmlAttrPtr dom_element_reflected_attribute_write(dom_object *obj, zval *n
 
 	/* Typed property, so it is a string already */
 	ZEND_ASSERT(Z_TYPE_P(newval) == IS_STRING);
-	return xmlSetNsProp(nodep, NULL, (const xmlChar *) name, (const xmlChar *) Z_STRVAL_P(newval));
+	return dom_set_attribute_value(nodep, NULL, (const xmlChar *) name, (const xmlChar *) Z_STRVAL_P(newval));
 }
 
 /* {{{ className	string
@@ -1030,11 +1030,7 @@ static void dom_set_attribute_ns_modern(dom_object *intern, xmlNodePtr elemp, ze
 	if (errorcode == 0) {
 		php_dom_libxml_ns_mapper *ns_mapper = php_dom_get_ns_mapper(intern);
 		xmlNsPtr ns = php_dom_libxml_ns_mapper_get_ns_raw_prefix_string(ns_mapper, prefix, xmlStrlen(prefix), uri);
-		xmlNodePtr existing = (xmlNodePtr) xmlHasNsProp(elemp, localname, ns == NULL ? NULL : ns->href);
-		if (existing != NULL && existing->type != XML_ATTRIBUTE_DECL) {
-			node_list_unlink(existing->children);
-		}
-		xmlAttrPtr attr = xmlSetNsProp(elemp, ns, localname, BAD_CAST value);
+		xmlAttrPtr attr = dom_set_attribute_value(elemp, ns, localname, BAD_CAST value);
 		if (UNEXPECTED(attr == NULL)) {
 			php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
 		} else {

ext/dom/php_dom.c

@@ -2408,6 +2408,15 @@ void dom_remove_all_children(xmlNodePtr nodep)
 	}
 }
 
+xmlAttrPtr dom_set_attribute_value(xmlNodePtr element, xmlNsPtr ns, const xmlChar *name, const xmlChar *value)
+{
+	xmlAttrPtr existing = xmlHasNsProp(element, name, ns == NULL ? NULL : ns->href);
+	if (existing != NULL && existing->type != XML_ATTRIBUTE_DECL) {
+		node_list_unlink(existing->children);
+	}
+	return xmlSetNsProp(element, ns, name, value);
+}
+
 void php_dom_get_content_into_zval(const xmlNode *nodep, zval *return_value, bool null_on_failure)
 {
 	ZEND_ASSERT(nodep != NULL);

ext/dom/php_dom.h

@@ -140,6 +140,7 @@ zend_object *dom_xpath_objects_new(zend_class_entry *class_type);
 #endif
 bool dom_get_strict_error(php_libxml_ref_obj *document);
 void node_list_unlink(xmlNodePtr node);
+xmlAttrPtr dom_set_attribute_value(xmlNodePtr element, xmlNsPtr ns, const xmlChar *name, const xmlChar *value);
 int dom_check_qname(char *qname, char **localname, char **prefix, int uri_len, int name_len);
 xmlNsPtr dom_get_ns(xmlNodePtr node, char *uri, int *errorcode, char *prefix);
 xmlNsPtr dom_get_ns_unchecked(xmlNodePtr nodep, char *uri, char *prefix);

ext/dom/tests/reflected_attribute_write_live_child.phpt

@@ -0,0 +1,63 @@
+--TEST--
+Reflected attribute writes keep attribute children with live wrappers
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+
+function check(string $label, $doc, Closure $write): void {
+    $element = $doc->documentElement;
+    $text = $element->getAttributeNode('class')->firstChild;
+
+    $write($element);
+
+    echo $label, ": detached: ";
+    var_dump($text->parentNode === null);
+    echo $label, ": text: ";
+    var_dump($text->textContent);
+    echo $label, ": new value: ";
+    var_dump($element->getAttribute('class'));
+}
+
+$doc = new DOMDocument();
+$doc->loadXML('<root class="old"/>');
+check('legacy className', $doc, static fn ($element) => $element->className = 'new');
+
+$doc = Dom\XMLDocument::createFromString('<root class="old"/>');
+check('modern className', $doc, static fn ($element) => $element->className = 'new');
+
+$doc = Dom\XMLDocument::createFromString('<root class="old"/>');
+check('classList value', $doc, static fn ($element) => $element->classList->value = 'new');
+
+$doc = Dom\XMLDocument::createFromString('<root class="old"/>');
+check('classList update', $doc, static fn ($element) => $element->classList->add('new'));
+
+$doc = Dom\XMLDocument::createFromString('<root class="old"/>');
+$element = $doc->documentElement;
+$element->id = 'old';
+$text = $element->getAttributeNode('id')->firstChild;
+$element->id = 'new';
+echo "id: detached: ";
+var_dump($text->parentNode === null);
+echo "id: text: ";
+var_dump($text->textContent);
+echo "id: new value: ";
+var_dump($element->id);
+
+?>
+--EXPECT--
+legacy className: detached: bool(true)
+legacy className: text: string(3) "old"
+legacy className: new value: string(3) "new"
+modern className: detached: bool(true)
+modern className: text: string(3) "old"
+modern className: new value: string(3) "new"
+classList value: detached: bool(true)
+classList value: text: string(3) "old"
+classList value: new value: string(3) "new"
+classList update: detached: bool(true)
+classList update: text: string(3) "old"
+classList update: new value: string(7) "old new"
+id: detached: bool(true)
+id: text: string(3) "old"
+id: new value: string(3) "new"

ext/dom/token_list.c

@@ -143,7 +143,7 @@ static void dom_token_list_update(dom_token_list_object *intern)
 	/* 2. Set an attribute value for the associated element using associated attribute’s local name and the result of
 	 *    running the ordered set serializer for token set. */
 	char *value = dom_ordered_set_serializer(token_set);
-	xmlSetNsProp(dom_token_list_get_element(intern), NULL, BAD_CAST "class", BAD_CAST value);
+	dom_set_attribute_value(dom_token_list_get_element(intern), NULL, BAD_CAST "class", BAD_CAST value);
 	efree(intern->cached_string);
 	intern->cached_string = value;
 }
@@ -382,7 +382,7 @@ zend_result dom_token_list_value_write(dom_object *obj, zval *newval)
 		zend_value_error("Value must not contain any null bytes");
 		return FAILURE;
 	}
-	xmlSetNsProp(dom_token_list_get_element(intern), NULL, BAD_CAST "class", BAD_CAST Z_STRVAL_P(newval));
+	dom_set_attribute_value(dom_token_list_get_element(intern), NULL, BAD_CAST "class", BAD_CAST Z_STRVAL_P(newval));
 	/* Note: we don't update the set here, the set is always lazily updated for performance reasons. */
 	return SUCCESS;
 }