diff --git a/NEWS b/NEWS index ad06a4eef1b7b..b13c914d650f9 100644 --- a/NEWS +++ b/NEWS @@ -73,6 +73,7 @@ PHP NEWS . Fix theoretical issues with hrtime() not being available. (nielsdos) . Fixed bug GH-19300 (Nested array_multisort invocation with error breaks). (nielsdos) + . Fixed bug GH-16649 (UAF during array_splice). (alexandre-daubois) - Windows: . Free opened_path when opened_path_len >= MAXPATHLEN. (dixyes) diff --git a/ext/standard/array.c b/ext/standard/array.c index a2a0459ec3b29..45e36ba09ce70 100644 --- a/ext/standard/array.c +++ b/ext/standard/array.c @@ -3214,6 +3214,9 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H zval *entry; /* Hash entry */ uint32_t iter_pos = zend_hash_iterators_lower_pos(in_hash, 0); + GC_ADDREF(in_hash); + HT_ALLOW_COW_VIOLATION(in_hash); /* Will be reset when setting the flags for in_hash */ + /* Get number of entries in the input hash */ num_in = zend_hash_num_elements(in_hash); @@ -3372,6 +3375,15 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H HT_SET_ITERATORS_COUNT(&out_hash, HT_ITERATORS_COUNT(in_hash)); HT_SET_ITERATORS_COUNT(in_hash, 0); in_hash->pDestructor = NULL; + + if (UNEXPECTED(GC_DELREF(in_hash) == 0)) { + /* Array was completely deallocated during the operation */ + zend_array_destroy(in_hash); + zend_hash_destroy(&out_hash); + zend_throw_error(NULL, "Array was modified during array_splice operation"); + return; + } + zend_hash_destroy(in_hash); HT_FLAGS(in_hash) = HT_FLAGS(&out_hash); diff --git a/ext/standard/tests/array/gh16649/array_splice_normal_destructor.phpt b/ext/standard/tests/array/gh16649/array_splice_normal_destructor.phpt new file mode 100644 index 0000000000000..59c0b316f54d5 --- /dev/null +++ b/ext/standard/tests/array/gh16649/array_splice_normal_destructor.phpt @@ -0,0 +1,20 @@ +--TEST-- +GH-16649: array_splice with normal destructor should work fine +--FILE-- + +--EXPECT-- +Destructor called +array(1) { + [0]=> + string(1) "1" +} diff --git a/ext/standard/tests/array/gh16649/array_splice_uaf_add_elements.phpt b/ext/standard/tests/array/gh16649/array_splice_uaf_add_elements.phpt new file mode 100644 index 0000000000000..f5e538122f415 --- /dev/null +++ b/ext/standard/tests/array/gh16649/array_splice_uaf_add_elements.phpt @@ -0,0 +1,22 @@ +--TEST-- +GH-16649: array_splice UAF when destructor adds elements to array +--FILE-- +getMessage() . "\n"; +} +?> +--EXPECT-- +Exception caught: Array was modified during array_splice operation diff --git a/ext/standard/tests/array/gh16649/array_splice_uaf_array_deallocated.phpt b/ext/standard/tests/array/gh16649/array_splice_uaf_array_deallocated.phpt new file mode 100644 index 0000000000000..1874ddad8934d --- /dev/null +++ b/ext/standard/tests/array/gh16649/array_splice_uaf_array_deallocated.phpt @@ -0,0 +1,22 @@ +--TEST-- +GH-16649: array_splice UAF when array is released entirely +--FILE-- +getMessage() . "\n"; +} +?> +--EXPECT-- +Exception caught: Array was modified during array_splice operation diff --git a/ext/standard/tests/array/gh16649/array_splice_uaf_complex_modification.phpt b/ext/standard/tests/array/gh16649/array_splice_uaf_complex_modification.phpt new file mode 100644 index 0000000000000..8ad4133300ec5 --- /dev/null +++ b/ext/standard/tests/array/gh16649/array_splice_uaf_complex_modification.phpt @@ -0,0 +1,25 @@ +--TEST-- +GH-16649: array_splice UAF with complex array modification +--FILE-- +getMessage() . "\n"; +} +?> +--EXPECT-- +Exception caught: Array was modified during array_splice operation diff --git a/ext/standard/tests/array/gh16649/array_splice_uaf_multiple_destructors.phpt b/ext/standard/tests/array/gh16649/array_splice_uaf_multiple_destructors.phpt new file mode 100644 index 0000000000000..88c6e4286c4d6 --- /dev/null +++ b/ext/standard/tests/array/gh16649/array_splice_uaf_multiple_destructors.phpt @@ -0,0 +1,33 @@ +--TEST-- +GH-16649: array_splice UAF with multiple destructors +--FILE-- +id = $id; + } + + function __destruct() { + global $arr; + echo "Destructor {$this->id} called\n"; + if ($this->id == 2) { + $arr = null; + } + } +} + +$arr = ["start", new MultiDestructor(1), new MultiDestructor(2), "end"]; + +try { + array_splice($arr, 1, 2); + echo "ERROR: Should have thrown exception\n"; +} catch (Error $e) { + echo "Exception caught: " . $e->getMessage() . "\n"; +} +?> +--EXPECT-- +Destructor 1 called +Destructor 2 called +Exception caught: Array was modified during array_splice operation diff --git a/ext/standard/tests/array/gh16649/array_splice_uaf_original_case.phpt b/ext/standard/tests/array/gh16649/array_splice_uaf_original_case.phpt new file mode 100644 index 0000000000000..4a82d5893157d --- /dev/null +++ b/ext/standard/tests/array/gh16649/array_splice_uaf_original_case.phpt @@ -0,0 +1,29 @@ +--TEST-- +GH-16649: array_splice UAF with destructor modifying array (original case) +--FILE-- + "1", "3" => new C, "2" => "2"]; + +try { + array_splice($arr, 1, 2); + echo "ERROR: Should have thrown exception\n"; +} catch (Error $e) { + echo "Exception caught: " . $e->getMessage() . "\n"; +} +?> +--EXPECT-- +Exception caught: Array was modified during array_splice operation diff --git a/ext/standard/tests/array/gh16649/array_splice_uaf_packed_to_hash.phpt b/ext/standard/tests/array/gh16649/array_splice_uaf_packed_to_hash.phpt new file mode 100644 index 0000000000000..bd8f511b6253e --- /dev/null +++ b/ext/standard/tests/array/gh16649/array_splice_uaf_packed_to_hash.phpt @@ -0,0 +1,23 @@ +--TEST-- +GH-16649: array_splice UAF when array is converted from packed to hash +--FILE-- +getMessage() . "\n"; +} +?> +--EXPECT-- +Exception caught: Array was modified during array_splice operation diff --git a/ext/standard/tests/array/gh16649/array_splice_with_replacement.phpt b/ext/standard/tests/array/gh16649/array_splice_with_replacement.phpt new file mode 100644 index 0000000000000..8754a913a24e4 --- /dev/null +++ b/ext/standard/tests/array/gh16649/array_splice_with_replacement.phpt @@ -0,0 +1,23 @@ +--TEST-- +GH-16649: array_splice with replacement array when destructor modifies array +--FILE-- +getMessage() . "\n"; +} +?> +--EXPECT-- +Exception caught: Array was modified during array_splice operation