diff --git a/NEWS b/NEWS index 338409dd70c5..9493cf8fc769 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) - Opcache: . Fixed opcache.protect_memory race under ZTS. (realFlowControl) 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-- +loadXML(']>'); +$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-- +loadXML(']>'); +$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(']>'); +$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-- +]>'; + +$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"