diff --git a/NEWS b/NEWS index 4ace7d3dc462..edad7d9d8a9c 100644 --- a/NEWS +++ b/NEWS @@ -37,6 +37,8 @@ PHP NEWS ownerDocument, parentNode, isConnected and baseURI now return correct values, and textContent returns NULL per the DOM specification. (jordikroon) + . Fixed bug GH-21952 (UAF in php_dom_object_get_data when DOMNotation + outlives owning DOCTYPE). (David Carlier) - Fileinfo: . Fixed bug GH-20679 (finfo_file() doesn't work on remote resources). diff --git a/UPGRADING b/UPGRADING index d271eb47f7d1..dcf64417828f 100644 --- a/UPGRADING +++ b/UPGRADING @@ -28,6 +28,10 @@ PHP 8.6 UPGRADE NOTES from global scope" instead of the prior readonly modification error. ReflectionProperty::isWritable() also reports these properties accurately. + . Dom\Notation and DOMNotation: $parentNode now returns null and + $isConnected returns false, matching the W3C DOM Level 3 Core + specification. The previous wiring caused a use-after-free when the + owning DocType was removed. - GD: . imagesetstyle(), imagefilter() and imagecrop() filter their diff --git a/ext/dom/dom_iterators.c b/ext/dom/dom_iterators.c index f97a9ec825d5..9350480d1b2a 100644 --- a/ext/dom/dom_iterators.c +++ b/ext/dom/dom_iterators.c @@ -63,7 +63,10 @@ xmlNodePtr create_notation(xmlDtdPtr parent_dtd, const xmlChar *name, const xmlC ret->ExternalID = xmlStrdup(ExternalID); ret->SystemID = xmlStrdup(SystemID); if (parent_dtd != NULL) { - ret->parent = parent_dtd; + /* Don't store parent_dtd: orphan notation can outlive the DTD when + * removeChild($doctype) frees it. parentNode/isConnected return spec- + * mandated null/false unconditionally; ownerDocument keeps working + * via ret->doc. */ ret->doc = parent_dtd->doc; } return (xmlNodePtr) ret; diff --git a/ext/dom/dom_properties.h b/ext/dom/dom_properties.h index 918bf0c62f78..85ee0260e4b4 100644 --- a/ext/dom/dom_properties.h +++ b/ext/dom/dom_properties.h @@ -147,6 +147,8 @@ zend_result dom_nodelist_length_read(dom_object *obj, zval *retval); /* notation properties */ zend_result dom_notation_public_id_read(dom_object *obj, zval *retval); zend_result dom_notation_system_id_read(dom_object *obj, zval *retval); +zend_result dom_notation_parent_node_read(dom_object *obj, zval *retval); +zend_result dom_notation_is_connected_read(dom_object *obj, zval *retval); /* processinginstruction properties */ zend_result dom_processinginstruction_target_read(dom_object *obj, zval *retval); diff --git a/ext/dom/notation.c b/ext/dom/notation.c index 73d0d6b083a5..01cf4be4b924 100644 --- a/ext/dom/notation.c +++ b/ext/dom/notation.c @@ -69,4 +69,17 @@ zend_result dom_notation_system_id_read(dom_object *obj, zval *retval) /* }}} */ +zend_result dom_notation_parent_node_read(dom_object *obj, zval *retval) +{ + DOM_PROP_NODE(xmlNodePtr, nodep, obj); + ZVAL_NULL(retval); + return SUCCESS; +} + +zend_result dom_notation_is_connected_read(dom_object *obj, zval *retval) +{ + DOM_PROP_NODE(xmlNodePtr, nodep, obj); + ZVAL_FALSE(retval); + return SUCCESS; +} #endif diff --git a/ext/dom/php_dom.c b/ext/dom/php_dom.c index eccbe4ed2984..4d731eac4043 100644 --- a/ext/dom/php_dom.c +++ b/ext/dom/php_dom.c @@ -1250,6 +1250,8 @@ PHP_MINIT_FUNCTION(dom) DOM_REGISTER_PROP_HANDLER(&dom_notation_prop_handlers, "publicId", dom_notation_public_id_read, NULL); DOM_REGISTER_PROP_HANDLER(&dom_notation_prop_handlers, "systemId", dom_notation_system_id_read, NULL); zend_hash_merge(&dom_notation_prop_handlers, &dom_node_prop_handlers, NULL, false); + DOM_OVERWRITE_PROP_HANDLER(&dom_notation_prop_handlers, "parentNode", dom_notation_parent_node_read, NULL); + DOM_OVERWRITE_PROP_HANDLER(&dom_notation_prop_handlers, "isConnected", dom_notation_is_connected_read, NULL); zend_hash_add_new_ptr(&classes, dom_notation_class_entry->name, &dom_notation_prop_handlers); dom_modern_notation_class_entry = register_class_Dom_Notation(dom_modern_node_class_entry); @@ -1260,7 +1262,9 @@ PHP_MINIT_FUNCTION(dom) DOM_REGISTER_PROP_HANDLER(&dom_modern_notation_prop_handlers, "publicId", dom_notation_public_id_read, NULL); DOM_REGISTER_PROP_HANDLER(&dom_modern_notation_prop_handlers, "systemId", dom_notation_system_id_read, NULL); zend_hash_merge(&dom_modern_notation_prop_handlers, &dom_modern_node_prop_handlers, NULL, false); - zend_hash_add_new_ptr(&classes, dom_modern_notation_class_entry->name, &dom_modern_node_prop_handlers); + DOM_OVERWRITE_PROP_HANDLER(&dom_modern_notation_prop_handlers, "parentNode", dom_notation_parent_node_read, NULL); + DOM_OVERWRITE_PROP_HANDLER(&dom_modern_notation_prop_handlers, "isConnected", dom_notation_is_connected_read, NULL); + zend_hash_add_new_ptr(&classes, dom_modern_notation_class_entry->name, &dom_modern_notation_prop_handlers); dom_entity_class_entry = register_class_DOMEntity(dom_node_class_entry); dom_entity_class_entry->create_object = dom_objects_new; diff --git a/ext/dom/tests/gh21952.phpt b/ext/dom/tests/gh21952.phpt new file mode 100644 index 000000000000..1869d5dee128 --- /dev/null +++ b/ext/dom/tests/gh21952.phpt @@ -0,0 +1,29 @@ +--TEST-- +GH-21952 (UAF in php_dom_object_get_data when DOMNotation outlives owning DOCTYPE) +--EXTENSIONS-- +dom +--FILE-- +loadXML(<<<'XML' + +]> + +XML); +$notation = $doc->doctype->notations[0]; +var_dump($notation->parentNode); +var_dump($notation->isConnected); +$doc->removeChild($doc->doctype); +var_dump($notation->nodeName); +var_dump($notation->systemId); +var_dump($notation->parentNode); +var_dump($notation->isConnected); +?> +--EXPECT-- +NULL +bool(false) +string(10) "myNotation" +string(8) "test.dtd" +NULL +bool(false) diff --git a/ext/dom/tests/modern/xml/DTDNamedNodeMap.phpt b/ext/dom/tests/modern/xml/DTDNamedNodeMap.phpt index 4ac15a029e37..59d6196f45bc 100644 --- a/ext/dom/tests/modern/xml/DTDNamedNodeMap.phpt +++ b/ext/dom/tests/modern/xml/DTDNamedNodeMap.phpt @@ -141,7 +141,11 @@ object(Dom\Entity)#3 (17) { ["textContent"]=> NULL } -object(Dom\Notation)#4 (14) { +object(Dom\Notation)#4 (16) { + ["publicId"]=> + string(0) "" + ["systemId"]=> + string(11) "viewgif.exe" ["nodeType"]=> int(12) ["nodeName"]=> @@ -149,11 +153,11 @@ object(Dom\Notation)#4 (14) { ["baseURI"]=> string(%d) "%s" ["isConnected"]=> - bool(true) + bool(false) ["ownerDocument"]=> string(22) "(object value omitted)" ["parentNode"]=> - string(22) "(object value omitted)" + NULL ["parentElement"]=> NULL ["childNodes"]=> diff --git a/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt b/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt index dd74a2737594..3beab4252f53 100644 --- a/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt +++ b/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt @@ -55,47 +55,47 @@ XML; string(3) "GIF" NULL NULL +bool(false) bool(true) -bool(true) -bool(true) +bool(false) NULL string(3) "GIF" NULL NULL +bool(false) bool(true) -bool(true) -bool(true) +bool(false) NULL bool(true) === JPEG === string(4) "JPEG" NULL NULL +bool(false) bool(true) -bool(true) -bool(true) +bool(false) NULL string(4) "JPEG" NULL NULL +bool(false) bool(true) -bool(true) -bool(true) +bool(false) NULL bool(true) === HTML === string(4) "HTML" NULL NULL +bool(false) bool(true) -bool(true) -bool(true) +bool(false) NULL string(4) "HTML" NULL NULL +bool(false) bool(true) -bool(true) -bool(true) +bool(false) NULL bool(true)