Heap buffer overflow in ZEND_ADD_ARRAY_UNPACK_SPEC_HANDLER

[Changochen]

Description

The following code:

<?php
ob_flush ( ) ;
echo [ 6.5 => 0.5 , ... [ [ 1 ] , [ 1 ] , 1 ]] ;

Using the asan build and USE_TRACKED_ALLOC=1 USE_ZEND_ALLOC=0 php-src/asan/sapi/cli/php -f poc1.php
Resulted in this output:

heap-buffer-overflow write

But I expected this output instead:

Array

PHP Version

PHP 8.3.0-dev

Operating System

No response

[nielsdos]

Very slightly simplified reproducer:

<?php
$x = [6.5 => 0, ...[1, 1, 1]];

Note: The floating point key is important to make it overflow, and the length of the nested list must be at least 3.
EDIT: the reason is that the index needs to be high enough for it to overflow, we're not reserving enough hash buckets in the unpacked VM handler.

[nielsdos]

My gut feeling says this is the right approach, but I would need to read more about the hashtables to know for sure.
It does get rid of the Valgrind complaint.
If someone confirms this is the right idea, I'll check other places in the VM and PR this up with a test.

diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h
index 7ef8cf1922..b085a30f67 100644
--- a/Zend/zend_vm_def.h
+++ b/Zend/zend_vm_def.h
@@ -6128,7 +6128,7 @@ ZEND_VM_C_LABEL(add_unpack_again):
 		zval *val;
 
 		if (HT_IS_PACKED(ht) && (zend_hash_num_elements(result_ht) == 0 || HT_IS_PACKED(result_ht))) {
-			zend_hash_extend(result_ht, zend_hash_num_elements(result_ht) + zend_hash_num_elements(ht), 1);
+			zend_hash_extend(result_ht, result_ht->nNumUsed + ht->nNumUsed, 1);
 			ZEND_HASH_FILL_PACKED(result_ht) {
 				ZEND_HASH_PACKED_FOREACH_VAL(ht, val) {
 					if (UNEXPECTED(Z_ISREF_P(val)) &&
diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h
index 19149c8c18..bbba8b6da5 100644
--- a/Zend/zend_vm_execute.h
+++ b/Zend/zend_vm_execute.h
@@ -2652,7 +2652,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_ADD_ARRAY_UNPACK_SPEC_HANDLER(
 		zval *val;
 
 		if (HT_IS_PACKED(ht) && (zend_hash_num_elements(result_ht) == 0 || HT_IS_PACKED(result_ht))) {
-			zend_hash_extend(result_ht, zend_hash_num_elements(result_ht) + zend_hash_num_elements(ht), 1);
+			zend_hash_extend(result_ht, result_ht->nNumUsed + ht->nNumUsed, 1);
 			ZEND_HASH_FILL_PACKED(result_ht) {
 				ZEND_HASH_PACKED_FOREACH_VAL(ht, val) {
 					if (UNEXPECTED(Z_ISREF_P(val)) &&

[iluuu1994]

I think you're right for result_ht. We're appending to the existing array which may have holes (nNumUsed > nNumOfElements). However, we only need to grow to result_ht->nNumOfElements + zend_hash_num_elements(ht) in that case (since it doesn't matter if ht has holes).

[nielsdos]

Makes sense to me.
I'll check that and PR it tomorrow evening.
I'll add a Suggested-by / Co-authored-by tag for you for the correction, if that's okay.

[dstogov]

I think, the proper fix in conjunction with #9796 should be

zend_hash_extend(result_ht, result_ht->nNumUsed + zend_hash_num_elements(ht), 1);

[nielsdos]

@dstogov I had already made a PR last night which fixes it the way you propose: #11021