From a56ff4fec7163b94108acd2dc9630774fed0f886 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 10 Oct 2024 21:28:45 +0200 Subject: [PATCH 1/2] Fix GH-16337: Use-after-free in SplHeap We introduce a new flag to indicate when a heap or priority queue is write-locked. In principle we could've used SPL_HEAP_CORRUPTED too, but that won't be descriptive to users (and it's a lie too). Closes GH-16346. --- NEWS | 3 +++ ext/spl/spl_heap.c | 55 +++++++++++++++++++++++++------------- ext/spl/tests/gh16337.phpt | 49 +++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 18 deletions(-) create mode 100644 ext/spl/tests/gh16337.phpt diff --git a/NEWS b/NEWS index 8281a0865fc..0c20c4dab52 100644 --- a/NEWS +++ b/NEWS @@ -32,6 +32,9 @@ PHP NEWS . Fixed bug GH-16385 (Unexpected null returned by session_set_cookie_params). (nielsdos) +- SPL: + . Fixed bug GH-16337 (Use-after-free in SplHeap). (nielsdos) + - XMLReader: . Fixed bug GH-16292 (Segmentation fault in ext/xmlreader/php_xmlreader.c). (nielsdos) diff --git a/ext/spl/spl_heap.c b/ext/spl/spl_heap.c index 736f151da58..df67bcb9daf 100644 --- a/ext/spl/spl_heap.c +++ b/ext/spl/spl_heap.c @@ -32,6 +32,7 @@ #define PTR_HEAP_BLOCK_SIZE 64 #define SPL_HEAP_CORRUPTED 0x00000001 +#define SPL_HEAP_WRITE_LOCKED 0x00000002 zend_object_handlers spl_handler_SplHeap; zend_object_handlers spl_handler_SplPriorityQueue; @@ -278,12 +279,16 @@ static void spl_ptr_heap_insert(spl_ptr_heap *heap, void *elem, void *cmp_userda heap->max_size *= 2; } + heap->flags |= SPL_HEAP_WRITE_LOCKED; + /* sifting up */ for (i = heap->count; i > 0 && heap->cmp(spl_heap_elem(heap, (i-1)/2), elem, cmp_userdata) < 0; i = (i-1)/2) { spl_heap_elem_copy(heap, spl_heap_elem(heap, i), spl_heap_elem(heap, (i-1)/2)); } heap->count++; + heap->flags &= ~SPL_HEAP_WRITE_LOCKED; + if (EG(exception)) { /* exception thrown during comparison */ heap->flags |= SPL_HEAP_CORRUPTED; @@ -311,6 +316,8 @@ static int spl_ptr_heap_delete_top(spl_ptr_heap *heap, void *elem, void *cmp_use return FAILURE; } + heap->flags |= SPL_HEAP_WRITE_LOCKED; + if (elem) { spl_heap_elem_copy(heap, elem, spl_heap_elem(heap, 0)); } else { @@ -334,6 +341,8 @@ static int spl_ptr_heap_delete_top(spl_ptr_heap *heap, void *elem, void *cmp_use } } + heap->flags &= ~SPL_HEAP_WRITE_LOCKED; + if (EG(exception)) { /* exception thrown during comparison */ heap->flags |= SPL_HEAP_CORRUPTED; @@ -374,10 +383,14 @@ static spl_ptr_heap *spl_ptr_heap_clone(spl_ptr_heap *from) { /* {{{ */ static void spl_ptr_heap_destroy(spl_ptr_heap *heap) { /* {{{ */ int i; + heap->flags |= SPL_HEAP_WRITE_LOCKED; + for (i = 0; i < heap->count; ++i) { heap->dtor(spl_heap_elem(heap, i)); } + heap->flags &= ~SPL_HEAP_WRITE_LOCKED; + efree(heap->elements); efree(heap); } @@ -597,6 +610,21 @@ PHP_METHOD(SplHeap, isEmpty) } /* }}} */ +static zend_result spl_heap_consistency_validations(const spl_heap_object *intern, bool write) +{ + if (intern->heap->flags & SPL_HEAP_CORRUPTED) { + zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + return FAILURE; + } + + if (write && (intern->heap->flags & SPL_HEAP_WRITE_LOCKED)) { + zend_throw_exception(spl_ce_RuntimeException, "Heap cannot be changed when it is already being modified.", 0); + return FAILURE; + } + + return SUCCESS; +} + /* {{{ Push $value on the heap */ PHP_METHOD(SplHeap, insert) { @@ -609,8 +637,7 @@ PHP_METHOD(SplHeap, insert) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, true) != SUCCESS)) { RETURN_THROWS(); } @@ -632,8 +659,7 @@ PHP_METHOD(SplHeap, extract) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, true) != SUCCESS)) { RETURN_THROWS(); } @@ -658,8 +684,7 @@ PHP_METHOD(SplPriorityQueue, insert) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, true) != SUCCESS)) { RETURN_THROWS(); } @@ -699,8 +724,7 @@ PHP_METHOD(SplPriorityQueue, extract) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, true) != SUCCESS)) { RETURN_THROWS(); } @@ -726,8 +750,7 @@ PHP_METHOD(SplPriorityQueue, top) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, false) != SUCCESS)) { RETURN_THROWS(); } @@ -837,8 +860,7 @@ PHP_METHOD(SplHeap, top) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, false) != SUCCESS)) { RETURN_THROWS(); } @@ -902,8 +924,7 @@ static zval *spl_heap_it_get_current_data(zend_object_iterator *iter) /* {{{ */ { spl_heap_object *object = Z_SPLHEAP_P(&iter->data); - if (object->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(object, false) != SUCCESS)) { return NULL; } @@ -920,8 +941,7 @@ static zval *spl_pqueue_it_get_current_data(zend_object_iterator *iter) /* {{{ * zend_user_iterator *user_it = (zend_user_iterator *) iter; spl_heap_object *object = Z_SPLHEAP_P(&iter->data); - if (object->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(object, false) != SUCCESS)) { return NULL; } @@ -949,8 +969,7 @@ static void spl_heap_it_move_forward(zend_object_iterator *iter) /* {{{ */ { spl_heap_object *object = Z_SPLHEAP_P(&iter->data); - if (object->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(object, false) != SUCCESS)) { return; } diff --git a/ext/spl/tests/gh16337.phpt b/ext/spl/tests/gh16337.phpt new file mode 100644 index 00000000000..94cf9d90cb1 --- /dev/null +++ b/ext/spl/tests/gh16337.phpt @@ -0,0 +1,49 @@ +--TEST-- +GH-16337 (Use-after-free in SplHeap) +--FILE-- +extract(); + } catch (Throwable $e) { + echo $e->getMessage(), "\n"; + } + try { + $heap->insert(1); + } catch (Throwable $e) { + echo $e->getMessage(), "\n"; + } + echo $heap->top(), "\n"; + return "0"; + } +} + +$heap = new SplMinHeap; +for ($i = 0; $i < 100; $i++) { + $heap->insert((string) $i); +} +$heap->insert(new C); + +?> +--EXPECT-- +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0 +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0 +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0 +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0 +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0 +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0 From 3ed01d454d371bc5a7423cc1c7b5a01f91f3a5f9 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 11 Oct 2024 21:46:30 +0200 Subject: [PATCH 2/2] Add missing hierarchy checks to replaceChild You can break the hierarchy for attribute nodes, use the helper function introduced recently [1] to fix this issue. [1] 066d18f2 Closes GH-16377. --- NEWS | 1 + ext/dom/node.c | 22 ++++----------- .../replaceChild_attribute_validation.phpt | 27 +++++++++++++++++++ 3 files changed, 33 insertions(+), 17 deletions(-) create mode 100644 ext/dom/tests/replaceChild_attribute_validation.phpt diff --git a/NEWS b/NEWS index 708a3f01993..a1ad51f3694 100644 --- a/NEWS +++ b/NEWS @@ -16,6 +16,7 @@ PHP NEWS - DOM: . Fixed bug GH-16316 (DOMXPath breaks when not initialized properly). (nielsdos) + . Add missing hierarchy checks to replaceChild. (nielsdos) - GD: . Fixed bug GH-16334 (imageaffine overflow on matrix elements). diff --git a/ext/dom/node.c b/ext/dom/node.c index 48e3c5cc2a7..242e56ad2d3 100644 --- a/ext/dom/node.c +++ b/ext/dom/node.c @@ -909,7 +909,7 @@ static xmlNodePtr _php_dom_insert_fragment(xmlNodePtr nodep, xmlNodePtr prevsib, } /* }}} */ -static bool dom_node_check_legacy_insertion_validity(xmlNodePtr parentp, xmlNodePtr child, bool stricterror) +static bool dom_node_check_legacy_insertion_validity(xmlNodePtr parentp, xmlNodePtr child, bool stricterror, bool warn_empty_fragment) { if (dom_node_is_read_only(parentp) == SUCCESS || (child->parent != NULL && dom_node_is_read_only(child->parent) == SUCCESS)) { @@ -927,7 +927,7 @@ static bool dom_node_check_legacy_insertion_validity(xmlNodePtr parentp, xmlNode return false; } - if (child->type == XML_DOCUMENT_FRAG_NODE && child->children == NULL) { + if (warn_empty_fragment && child->type == XML_DOCUMENT_FRAG_NODE && child->children == NULL) { /* TODO Drop Warning? */ php_error_docref(NULL, E_WARNING, "Document Fragment is empty"); return false; @@ -969,7 +969,7 @@ PHP_METHOD(DOMNode, insertBefore) stricterror = dom_get_strict_error(intern->document); - if (!dom_node_check_legacy_insertion_validity(parentp, child, stricterror)) { + if (!dom_node_check_legacy_insertion_validity(parentp, child, stricterror, true)) { RETURN_FALSE; } @@ -1127,19 +1127,7 @@ PHP_METHOD(DOMNode, replaceChild) stricterror = dom_get_strict_error(intern->document); - if (dom_node_is_read_only(nodep) == SUCCESS || - (newchild->parent != NULL && dom_node_is_read_only(newchild->parent) == SUCCESS)) { - php_dom_throw_error(NO_MODIFICATION_ALLOWED_ERR, stricterror); - RETURN_FALSE; - } - - if (newchild->doc != nodep->doc && newchild->doc != NULL) { - php_dom_throw_error(WRONG_DOCUMENT_ERR, stricterror); - RETURN_FALSE; - } - - if (dom_hierarchy(nodep, newchild) == FAILURE) { - php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror); + if (!dom_node_check_legacy_insertion_validity(nodep, newchild, stricterror, false)) { RETURN_FALSE; } @@ -1247,7 +1235,7 @@ PHP_METHOD(DOMNode, appendChild) stricterror = dom_get_strict_error(intern->document); - if (!dom_node_check_legacy_insertion_validity(nodep, child, stricterror)) { + if (!dom_node_check_legacy_insertion_validity(nodep, child, stricterror, true)) { RETURN_FALSE; } diff --git a/ext/dom/tests/replaceChild_attribute_validation.phpt b/ext/dom/tests/replaceChild_attribute_validation.phpt new file mode 100644 index 00000000000..32d5990f75e --- /dev/null +++ b/ext/dom/tests/replaceChild_attribute_validation.phpt @@ -0,0 +1,27 @@ +--TEST-- +replaceChild with attribute children +--EXTENSIONS-- +dom +--FILE-- +createAttribute('attr'); +$attr->textContent = "test"; + +try { + $attr->replaceChild($dom->createProcessingInstruction('pi'), $attr->firstChild); +} catch (DOMException $e) { + echo $e->getMessage(), "\n"; +} + +$root = $dom->appendChild($dom->createElement('root')); +$root->setAttributeNode($attr); + +echo $dom->saveXML(); + +?> +--EXPECT-- +Hierarchy Request Error + +