From fbb006199321f4266ac43a30706b674846566433 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 15 Nov 2024 21:38:27 +0100 Subject: [PATCH 1/2] Fix GH-16808: Segmentation fault in RecursiveIteratorIterator->current() with a xml element input When the current data is invalid, NULL must be returned. At least that's how the check in SPL works and how other extensions do this as well. If we don't do this, an UNDEF value gets propagated to a return value (misprinted as null); leading to issues. Closes GH-16825. --- NEWS | 4 ++++ ext/simplexml/simplexml.c | 6 +++++- ext/simplexml/tests/gh16808.phpt | 12 ++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 ext/simplexml/tests/gh16808.phpt diff --git a/NEWS b/NEWS index 62c1e3eeeeb..ee2e4949920 100644 --- a/NEWS +++ b/NEWS @@ -37,6 +37,10 @@ PHP NEWS . Fixed bug GH-16695 (phar:// tar parser and zero-length file header blocks). (nielsdos, Hans Krentel) +- SimpleXML: + . Fixed bug GH-16808 (Segmentation fault in RecursiveIteratorIterator + ->current() with a xml element input). (nielsdos) + 21 Nov 2024, PHP 8.2.26 - Cli: diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index 21cd5cdb4c7..7e1dcf9eb2a 100644 --- a/ext/simplexml/simplexml.c +++ b/ext/simplexml/simplexml.c @@ -2539,7 +2539,11 @@ static zval *php_sxe_iterator_current_data(zend_object_iterator *iter) /* {{{ */ { php_sxe_iterator *iterator = (php_sxe_iterator *)iter; - return &iterator->sxe->iter.data; + zval *data = &iterator->sxe->iter.data; + if (Z_ISUNDEF_P(data)) { + return NULL; + } + return data; } /* }}} */ diff --git a/ext/simplexml/tests/gh16808.phpt b/ext/simplexml/tests/gh16808.phpt new file mode 100644 index 00000000000..be0bc59fb65 --- /dev/null +++ b/ext/simplexml/tests/gh16808.phpt @@ -0,0 +1,12 @@ +--TEST-- +GH-16808 (Segmentation fault in RecursiveIteratorIterator->current() with a xml element input) +--EXTENSIONS-- +simplexml +--FILE-- +"); +$test = new RecursiveIteratorIterator($sxe); +var_dump($test->current()); +?> +--EXPECT-- +NULL From 18b18f0ee02653a6a17aed1424e50489af657910 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 15 Nov 2024 21:15:36 +0100 Subject: [PATCH 2/2] Fix GH-16777: Calling the constructor again on a DOM object after it is in a document causes UAF Closes GH-16824. --- NEWS | 4 ++++ ext/dom/node.c | 3 +++ ext/dom/tests/gh16777_1.phpt | 24 ++++++++++++++++++++++++ ext/dom/tests/gh16777_2.phpt | 27 +++++++++++++++++++++++++++ 4 files changed, 58 insertions(+) create mode 100644 ext/dom/tests/gh16777_1.phpt create mode 100644 ext/dom/tests/gh16777_2.phpt diff --git a/NEWS b/NEWS index 8f80b7372f0..781c9387a63 100644 --- a/NEWS +++ b/NEWS @@ -15,6 +15,10 @@ PHP NEWS - Curl: . Fixed bug GH-16802 (open_basedir bypass using curl extension). (nielsdos) +- DOM: + . Fixed bug GH-16777 (Calling the constructor again on a DOM object after it + is in a document causes UAF). (nielsdos) + - FPM: . Fixed GH-16432 (PHP-FPM 8.2 SIGSEGV in fpm_get_status). (Jakub Zelenka) diff --git a/ext/dom/node.c b/ext/dom/node.c index cc693cd8a36..07c6859d730 100644 --- a/ext/dom/node.c +++ b/ext/dom/node.c @@ -1024,6 +1024,7 @@ PHP_METHOD(DOMNode, insertBefore) } if (child->doc == NULL && parentp->doc != NULL) { + xmlSetTreeDoc(child, parentp->doc); dom_set_document_ref_pointers(child, intern->document); } @@ -1188,6 +1189,7 @@ PHP_METHOD(DOMNode, replaceChild) } if (newchild->doc == NULL && nodep->doc != NULL) { + xmlSetTreeDoc(newchild, nodep->doc); dom_set_document_ref_pointers(newchild, intern->document); } @@ -1291,6 +1293,7 @@ PHP_METHOD(DOMNode, appendChild) } if (child->doc == NULL && nodep->doc != NULL) { + xmlSetTreeDoc(child, nodep->doc); dom_set_document_ref_pointers(child, intern->document); } diff --git a/ext/dom/tests/gh16777_1.phpt b/ext/dom/tests/gh16777_1.phpt new file mode 100644 index 00000000000..d821842ace4 --- /dev/null +++ b/ext/dom/tests/gh16777_1.phpt @@ -0,0 +1,24 @@ +--TEST-- +GH-16777 (Calling the constructor again on a DOM object after it is in a document causes UAF) +--EXTENSIONS-- +dom +--FILE-- +appendChild($text); +$text->__construct('my new value'); +$doc->appendChild($text); +echo $doc->saveXML(); +$dom2 = new DOMDocument(); +try { + $dom2->appendChild($text); +} catch (DOMException $e) { + echo $e->getMessage(), "\n"; +} +?> +--EXPECT-- + +my value +my new value +Wrong Document Error diff --git a/ext/dom/tests/gh16777_2.phpt b/ext/dom/tests/gh16777_2.phpt new file mode 100644 index 00000000000..a6f3a59621b --- /dev/null +++ b/ext/dom/tests/gh16777_2.phpt @@ -0,0 +1,27 @@ +--TEST-- +GH-16777 (Calling the constructor again on a DOM object after it is in a document causes UAF) +--EXTENSIONS-- +dom +--FILE-- +append($child = new DOMElement('child')); +$doc = new DOMDocument(); +$doc->appendChild($el); +$el->__construct('newname'); +$doc->appendChild($el); +echo $doc->saveXML(); +$dom2 = new DOMDocument(); +try { + $dom2->appendChild($el); +} catch (DOMException $e) { + echo $e->getMessage(), "\n"; +} +var_dump($child->ownerDocument === $doc); +?> +--EXPECT-- + + + +Wrong Document Error +bool(true)