Fix #80927: Removing documentElement after creating attribute node: possible use-after-free

Closes phpGH-11892.

Author: ndossche

NEWS

@@ -8,6 +8,8 @@ PHP                                                                        NEWS
 - DOM:
   . adoptNode now respects the strict error checking property. (nielsdos)
   . Align DOMChildNode parent checks with spec. (nielsdos)
+  . Fixed bug #80927 (Removing documentElement after creating attribute node:
+    possible use-after-free). (nielsdos)
 
 - Opcache:
   . Avoid resetting JIT counter handlers from multiple processes/threads.

UPGRADING.INTERNALS

@@ -148,6 +148,8 @@ PHP 8.3 INTERNALS UPGRADE NOTES
      dom_parent_node_before() now use an uint32_t argument for the number of nodes instead of int.
    - There is now a helper function php_dom_get_content_into_zval() to get the contents of a node.
      This avoids allocation if possible.
+   - The function dom_set_old_ns() has been moved into ext/libxml as php_libxml_set_old_ns() and
+     is now publicly exposed as an API.
 
  g. ext/libxml
    - Two new functions: php_libxml_invalidate_node_list_cache_from_doc() and

ext/dom/element.c

@@ -1497,7 +1497,7 @@ PHP_METHOD(DOMElement, toggleAttribute)
 			}
 
 			ns->next = NULL;
-			dom_set_old_ns(thisp->doc, ns);
+			php_libxml_set_old_ns(thisp->doc, ns);
 			dom_reconcile_ns(thisp->doc, thisp);
 		} else {
 			/* TODO: in the future when namespace bugs are fixed,

ext/dom/php_dom.c

@@ -1436,35 +1436,6 @@ void dom_normalize (xmlNodePtr nodep)
 }
 /* }}} end dom_normalize */
 
-
-/* {{{ void dom_set_old_ns(xmlDoc *doc, xmlNs *ns) */
-void dom_set_old_ns(xmlDoc *doc, xmlNs *ns) {
-	if (doc == NULL)
-		return;
-
-	ZEND_ASSERT(ns->next == NULL);
-
-	/* Note: we'll use a prepend strategy instead of append to
-	 * make sure we don't lose performance when the list is long.
-	 * As libxml2 could assume the xml node is the first one, we'll place our
-	 * new entries after the first one. */
-
-	if (doc->oldNs == NULL) {
-		doc->oldNs = (xmlNsPtr) xmlMalloc(sizeof(xmlNs));
-		if (doc->oldNs == NULL) {
-			return;
-		}
-		memset(doc->oldNs, 0, sizeof(xmlNs));
-		doc->oldNs->type = XML_LOCAL_NAMESPACE;
-		doc->oldNs->href = xmlStrdup(XML_XML_NAMESPACE);
-		doc->oldNs->prefix = xmlStrdup((const xmlChar *)"xml");
-	} else {
-		ns->next = doc->oldNs->next;
-	}
-	doc->oldNs->next = ns;
-}
-/* }}} end dom_set_old_ns */
-
 static void dom_reconcile_ns_internal(xmlDocPtr doc, xmlNodePtr nodep, xmlNodePtr search_parent)
 {
 	xmlNsPtr nsptr, nsdftptr, curns, prevns = NULL;
@@ -1486,7 +1457,7 @@ static void dom_reconcile_ns_internal(xmlDocPtr doc, xmlNodePtr nodep, xmlNodePt
 					/* Note: we can't get here if the ns is already on the oldNs list.
 					 * This is because in that case the definition won't be on the node, and
 					 * therefore won't be in the nodep->nsDef list. */
-					dom_set_old_ns(doc, curns);
+					php_libxml_set_old_ns(doc, curns);
 					curns = prevns;
 				}
 			}

ext/dom/php_dom.h

@@ -128,7 +128,6 @@ void php_dom_throw_error_with_message(int error_code, char *error_message, int s
 void node_list_unlink(xmlNodePtr node);
 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);
-void dom_set_old_ns(xmlDoc *doc, xmlNs *ns);
 void dom_reconcile_ns(xmlDocPtr doc, xmlNodePtr nodep);
 void dom_reconcile_ns_list(xmlDocPtr doc, xmlNodePtr nodep, xmlNodePtr last);
 xmlNsPtr dom_get_nsdecl(xmlNode *node, xmlChar *localName);

ext/dom/tests/bug80927.phpt

@@ -0,0 +1,89 @@
+--TEST--
+Bug #80927 (Removing documentElement after creating attribute node: possible use-after-free)
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+
+function test1() {
+    $dom = new DOMDocument();
+    $dom->appendChild($dom->createElement("html"));
+    $a = $dom->createAttributeNS("fake_ns", "test:test");
+    $dom->removeChild($dom->documentElement);
+
+    echo $dom->saveXML();
+
+    var_dump($a->namespaceURI);
+    var_dump($a->prefix);
+}
+
+enum Test2Variation {
+    case REMOVE_DOCUMENT;
+    case REMOVE_CHILD;
+}
+
+function test2(Test2Variation $variation) {
+    $dom = new DOMDocument();
+    $dom->appendChild($dom->createElement("html"));
+    $a = $dom->createAttributeNS("fake_ns", "test:test");
+
+    $foo = $dom->appendChild($dom->createElement('foo'));
+    $foo->appendChild($dom->documentElement);
+
+    unset($foo);
+
+    match ($variation) {
+        Test2Variation::REMOVE_DOCUMENT => $dom->documentElement->remove(),
+        Test2Variation::REMOVE_CHILD => $dom->documentElement->firstElementChild->remove(),
+    };
+
+    echo $dom->saveXML();
+
+    var_dump($a->namespaceURI);
+    var_dump($a->prefix);
+}
+
+function test3() {
+    $dom = new DOMDocument();
+    $dom->appendChild($dom->createElement('html'));
+    $foobar = $dom->documentElement->appendChild($dom->createElementNS('some:ns', 'foo:bar'));
+    $foobar2 = $foobar->appendChild($dom->createElementNS('some:ns', 'foo:bar2'));
+    $foobar->remove();
+    unset($foobar);
+    $dom->documentElement->appendChild($foobar2);
+
+    echo $dom->saveXML();
+
+    var_dump($foobar2->namespaceURI);
+    var_dump($foobar2->prefix);
+}
+
+echo "--- Remove namespace declarator for attribute, root ---\n";
+test1();
+echo "--- Remove namespace declarator for attribute, moved root ---\n";
+test2(Test2Variation::REMOVE_DOCUMENT);
+echo "--- Remove namespace declarator for attribute, moved root child ---\n";
+test2(Test2Variation::REMOVE_CHILD);
+echo "--- Remove namespace declarator for element ---\n";
+test3();
+
+?>
+--EXPECT--
+--- Remove namespace declarator for attribute, root ---
+<?xml version="1.0"?>
+string(7) "fake_ns"
+string(4) "test"
+--- Remove namespace declarator for attribute, moved root ---
+<?xml version="1.0"?>
+string(7) "fake_ns"
+string(4) "test"
+--- Remove namespace declarator for attribute, moved root child ---
+<?xml version="1.0"?>
+<foo/>
+string(7) "fake_ns"
+string(4) "test"
+--- Remove namespace declarator for element ---
+<?xml version="1.0"?>
+<html><foo:bar2 xmlns:foo="some:ns"/></html>
+string(7) "some:ns"
+string(3) "foo"

ext/libxml/libxml.c

@@ -103,6 +103,39 @@ zend_module_entry libxml_module_entry = {
 
 /* }}} */
 
+static void php_libxml_set_old_ns_list(xmlDocPtr doc, xmlNsPtr first, xmlNsPtr last)
+{
+	if (UNEXPECTED(doc == NULL)) {
+		return;
+	}
+
+	ZEND_ASSERT(last->next == NULL);
+
+	/* Note: we'll use a prepend strategy instead of append to
+	 * make sure we don't lose performance when the list is long.
+	 * As libxml2 could assume the xml node is the first one, we'll place our
+	 * new entries after the first one. */
+
+	if (UNEXPECTED(doc->oldNs == NULL)) {
+		doc->oldNs = (xmlNsPtr) xmlMalloc(sizeof(xmlNs));
+		if (doc->oldNs == NULL) {
+			return;
+		}
+		memset(doc->oldNs, 0, sizeof(xmlNs));
+		doc->oldNs->type = XML_LOCAL_NAMESPACE;
+		doc->oldNs->href = xmlStrdup(XML_XML_NAMESPACE);
+		doc->oldNs->prefix = xmlStrdup((const xmlChar *)"xml");
+	} else {
+		last->next = doc->oldNs->next;
+	}
+	doc->oldNs->next = first;
+}
+
+PHP_LIBXML_API void php_libxml_set_old_ns(xmlDocPtr doc, xmlNsPtr ns)
+{
+	php_libxml_set_old_ns_list(doc, ns, ns);
+}
+
 static void php_libxml_unlink_entity(void *data, void *table, const xmlChar *name)
 {
 	xmlEntityPtr entity = data;
@@ -211,8 +244,41 @@ static void php_libxml_node_free(xmlNodePtr node)
 					xmlHashScan(dtd->pentities, php_libxml_unlink_entity, dtd->pentities);
 					/* No unlinking of notations, see remark above at case XML_NOTATION_NODE. */
 				}
-				ZEND_FALLTHROUGH;
+				xmlFreeNode(node);
+				break;
 			}
+			case XML_ELEMENT_NODE:
+				if (node->nsDef && node->doc) {
+					/* Make the namespace declaration survive the destruction of the holding element.
+					 * This prevents a use-after-free on the namespace declaration.
+					 *
+					 * The main problem is that libxml2 doesn't have a reference count on the namespace declaration.
+					 * We don't actually need to save the namespace declaration if we know the subtree it belongs to
+					 * has no references from userland. However, we can't know that without traversing the whole subtree
+					 * (=> slow), or without adding some subtree metadata (=> also slow).
+					 * So we have to assume we need to save everything.
+					 *
+					 * However, namespace declarations are quite rare in comparison to other node types.
+					 * Most node types are either elements, text or attributes.
+					 * And you only need one namespace declaration per namespace (in principle).
+					 * So I expect the number of namespace declarations to be low for an average XML document.
+					 *
+					 * In the worst possible case we have to save all namespace declarations when we for example remove
+					 * the whole document. But given the above reasoning this likely won't be a lot of declarations even
+					 * in the worst case.
+					 * A single declaration only takes about 48 bytes of memory, and I don't expect the worst case to occur
+					 * very often (why would you remove the whole document?).
+					 */
+					xmlNsPtr ns = node->nsDef;
+					xmlNsPtr last = ns;
+					while (last->next) {
+						last = last->next;
+					}
+					php_libxml_set_old_ns_list(node->doc, ns, last);
+					node->nsDef = NULL;
+				}
+				xmlFreeNode(node);
+				break;
 			default:
 				xmlFreeNode(node);
 				break;

ext/libxml/php_libxml.h

@@ -134,6 +134,7 @@ PHP_LIBXML_API int php_libxml_xmlCheckUTF8(const unsigned char *s);
 PHP_LIBXML_API void php_libxml_switch_context(zval *context, zval *oldcontext);
 PHP_LIBXML_API void php_libxml_issue_error(int level, const char *msg);
 PHP_LIBXML_API bool php_libxml_disable_entity_loader(bool disable);
+PHP_LIBXML_API void php_libxml_set_old_ns(xmlDocPtr doc, xmlNsPtr ns);
 
 /* Init/shutdown functions*/
 PHP_LIBXML_API void php_libxml_initialize(void);