heap-buffer-overflow at zval_undefined_cv

[Changochen]

Description

The following code:

<?php
class test
{
    protected $_id;
    static $instances;
    public function __construct($id) {
      11 < self::$instances[$this->_id] = $this;
    }

    function __destruct() { unset(self::$instances[$this->_id]);
    }
}
new test(2);
new test(2);
new test(3);
?>

Resulted in this output:

./php-fuzz-execute poc1.php
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2128458864
INFO: Loaded 1 modules   (147832 inline 8-bit counters): 147832 [0x1d60c20, 0x1d84d98),
INFO: Loaded 1 PC tables (147832 PCs): 147832 [0x1d84d98,0x1fc6518),
./php-fuzz-execute: Running 1 inputs 1 time(s) each.
Running: poc1.php
=================================================================
==2827130==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000002108 at pc 0x00000120a6bf bp 0x7fffffffd8e0 sp 0x7fffffffd8d8
READ of size 8 at 0x602000002108 thread T0

Git commit: ff42cb0

PHP Version

8.3.0-dev

Operating System

No response

[nielsdos]

I spent some time analyzing the bug and I think I understand what's happening.

Slightly simplified reproducer:

<?php
class test
{
    static $instances;
    public function __construct(private $id) {
      (self::$instances[$undefined_variable] = $this) > 0;
    }

    function __destruct() {
      unset(self::$instances[NULL]);
    }
}
new test(2);
new test(3);
?>

Reasoning:
It is during the constructor of "new test(3)" that PHP crashes. During the ASSIGN_DIM is executed, the destructor of "new test(2)" is called. As both "new test(2)" and "new test(3)" occupy the same slot in the array, the result of the assignment becomes UNDEF. This makes sure that OP2 of ASSIGN_DIM is UNDEF (T3 in the opcodes). Therefore, the RHS of the comparison (T5) is UNDEF.

0000 CV0($id) = RECV 1
0001 ASSIGN_OBJ THIS string("id")
0002 OP_DATA CV0($id)
0003 T4 = FETCH_THIS
0004 V2 = FETCH_STATIC_PROP_W (dim write) string("instances") (self) (exception)
0005 T3 = ASSIGN_DIM V2 CV1($undefined_variable)
0006 OP_DATA T4
0007 T5 = IS_SMALLER int(0) T3
0008 FREE T5
0009 RETURN null

In the VM's comparison code, _zval_undefined_op2 is called because we got an UNDEF for T3. But _zval_undefined_op{1,2} assume that we're using CVs, not TMPs, so the lookup of the variable name crashes.

[nielsdos]

I made a quick PoC patch, although I'm not sure if this is the right way to solve this. If it is, I'll happily make a PR :)

diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c
index fc54a8e026..a2c98cffc9 100644
--- a/Zend/zend_execute.c
+++ b/Zend/zend_execute.c
@@ -273,14 +273,26 @@ static zend_never_inline ZEND_COLD zval* zval_undefined_cv(uint32_t var EXECUTE_
 	return &EG(uninitialized_zval);
 }
 
+static zend_never_inline ZEND_COLD zval* zval_undefined_var(uint32_t var, zend_uchar type EXECUTE_DATA_DC)
+{
+	if (type & IS_CV) {
+		return zval_undefined_cv(var EXECUTE_DATA_CC);
+	} else {
+		if (EXPECTED(EG(exception) == NULL)) {
+			zend_error(E_WARNING, "Undefined operand");
+		}
+		return &EG(uninitialized_zval);
+	}
+}
+
 static zend_never_inline ZEND_COLD zval* ZEND_FASTCALL _zval_undefined_op1(EXECUTE_DATA_D)
 {
-	return zval_undefined_cv(EX(opline)->op1.var EXECUTE_DATA_CC);
+	return zval_undefined_var(EX(opline)->op1.var, EX(opline)->op1_type EXECUTE_DATA_CC);
 }
 
 static zend_never_inline ZEND_COLD zval* ZEND_FASTCALL _zval_undefined_op2(EXECUTE_DATA_D)
 {
-	return zval_undefined_cv(EX(opline)->op2.var EXECUTE_DATA_CC);
+	return zval_undefined_var(EX(opline)->op2.var, EX(opline)->op2_type EXECUTE_DATA_CC);
 }
 
 #define ZVAL_UNDEFINED_OP1() _zval_undefined_op1(EXECUTE_DATA_C)

[iluuu1994]

VAR/TMPVAR are not supposed to be UNDEF. I took a look but still don't understand exactly what happens, but it seems like something goes wrong with refcounting at some point. The constructor of the second instance frees the first element by overwriting the given index in the array. There seems to be some confusion happening after the destructor was called, possibly expecting the array element to still be the same? I'll have a closer look soon.

[nielsdos]

Even if this is a refcounting bug, the destructor does not necessarily need to run deterministically right? For example if there is a reference cycle somewhere, then the cyclic garbage collector may effectively still free the first instance during the second instance's construction and call the destructor of the first one, setting the temporary to UNDEF anyway?

[iluuu1994]

@nielsdos The cycle collector doesn't consider static properties to be internal cycles. Even if one were to call gc_collect_cycles() the instance won't be freed until it gets removed from the static property. Thus, the destructor does get called deterministically here, as soon as the instance is removed from the array and the refcount goes to 0.

TMP/TMPVAR should always be initialized by some opcode handler before being consumed by some other. UNDEF is not a valid value for those, opcode handlers can assume that a TMP/TMPVAR should be a non-UNDEF value. See this great blog post from Nikita.

Of course, this might be the result of some other error (like copying some other undefined zval into the VAR). I just wanted to point out that adding code to handle UNDEF for TMP/TMPVAR is likely not the right solution, as this assumption is being made all over in the VM.

[nielsdos]

Thanks for the comment.
I spent some time on this. I'm not 100% sure about this but maybe it's helpful. I think the following happens (I'm using line numbers from PHP-8.1 at commit 4c9375e:

1. ASSIGN_DIM gets executed and we get in the branch at zend_vm_execute.h lines 50964-50974.
2. Line 50974 gets executed, which calls zend_assign_to_variable. (bullet points 2.x are all in zend_assign_to_variable)
   2.1. Line 156 sets garbage to the zend_refcounted* of variable_ptr.
   2.2. We copy value to variable_ptr at line 157.
   2.3. The old zend_refcounted* from before the copy (garbage) must be destroyed, in doing so it calls rc_dtor_func(garbage) which calls the userland destructor. The userland destructor unsets the array slot in $instances, which means variable_ptr now corresponds to an array slot that's no longer valid.
3. We're back in zend_vm_execute.h at the returning on line 50974. Note that using variable_ptr is effectively using "unset" data now.
4. Line 50977 now puts UNDEF into value.

So it's actually some sort of use-after-free in disguise.

I tried fixing it by refetching variable_ptr from the array when a destructor gets called, but this became quite hairy code. I also wonder whether this isn't a more general problem that's not only limited to arrays.

[nielsdos]

I've been thinking about a solution for this.
I tried things like refetching variable_ptr and temporarily increasing refcounts, but I don't like those solutions because they are complex and have a bad performance impact.
I came up with the following alternative (PoC patch below):

The problem is that we're using the variable_ptr in the opcode handler after it has already been destroyed.
So my idea is to delay the destructors until after the variable_ptr is used.
To accomplish this I made 2 new API functions zend_assign_to_variable_delayed_garbage_handling and zend_assign_to_variable_handle_garbage that allows users to delay the garbage handling.
zend_assign_to_variable is now a wrapper for those such that there is no BC break.

(I also think that the same concept could maybe also be used for #10169)

I checked and all tests pass, including the reduced test code below.

Test

Test code:

<?php
class test
{
    static $instances;
    public function __construct(private $id) {
	    (self::$instances[NULL] = $this) > 0;
	    var_dump(self::$instances);
    }

    function __destruct() {
      unset(self::$instances[NULL]);
    }
}
new test(2);
new test(3);

Now no longer crashes and outputs:

Notice: Object of class test could not be converted to int in /home/niels/php-src/test.php on line 6
array(1) {
  [""]=>
  object(test)#1 (1) {
    ["id":"test":private]=>
    int(2)
  }
}

Notice: Object of class test could not be converted to int in /home/niels/php-src/test.php on line 6
array(0) {
}

Patch

This is a proof-of-concept patch where I used my new API functions in ASSIGN_DIM.
There are probably places other than ASSIGN_DIM where this would be necessary to fix all instances of the same bug. Since this is a PoC I didn't check that yet.
Also: I don't know if I need to change anything to the JIT yet, I haven't touched that code at all yet, so I'd have to check.

(And function names could probably be improved as well)

diff --git a/Zend/zend_execute.h b/Zend/zend_execute.h
index a9b316b8bd..c382f7a3ff 100644
--- a/Zend/zend_execute.h
+++ b/Zend/zend_execute.h
@@ -137,12 +137,24 @@ static zend_always_inline void zend_copy_to_variable(zval *variable_ptr, zval *v
 	}
 }
 
-static zend_always_inline zval* zend_assign_to_variable(zval *variable_ptr, zval *value, zend_uchar value_type, bool strict)
+static zend_always_inline void zend_assign_to_variable_handle_garbage(zend_refcounted *garbage)
+{
+	if (!garbage)
+		return;
+	if (GC_DELREF(garbage) == 0) {
+		rc_dtor_func(garbage);
+	} else { /* we need to split */
+		/* optimized version of GC_ZVAL_CHECK_POSSIBLE_ROOT(variable_ptr) */
+		if (UNEXPECTED(GC_MAY_LEAK(garbage))) {
+			gc_possible_root(garbage);
+		}
+	}
+}
+
+static zend_always_inline zval* zend_assign_to_variable_delayed_garbage_handling(zval *variable_ptr, zval *value, zend_uchar value_type, bool strict, zend_refcounted **garbage)
 {
 	do {
 		if (UNEXPECTED(Z_REFCOUNTED_P(variable_ptr))) {
-			zend_refcounted *garbage;
-
 			if (Z_ISREF_P(variable_ptr)) {
 				if (UNEXPECTED(ZEND_REF_HAS_TYPE_SOURCES(Z_REF_P(variable_ptr)))) {
 					return zend_assign_to_typed_ref(variable_ptr, value, value_type, strict);
@@ -153,16 +165,8 @@ static zend_always_inline zval* zend_assign_to_variable(zval *variable_ptr, zval
 					break;
 				}
 			}
-			garbage = Z_COUNTED_P(variable_ptr);
+			*garbage = Z_COUNTED_P(variable_ptr);
 			zend_copy_to_variable(variable_ptr, value, value_type);
-			if (GC_DELREF(garbage) == 0) {
-				rc_dtor_func(garbage);
-			} else { /* we need to split */
-				/* optimized version of GC_ZVAL_CHECK_POSSIBLE_ROOT(variable_ptr) */
-				if (UNEXPECTED(GC_MAY_LEAK(garbage))) {
-					gc_possible_root(garbage);
-				}
-			}
 			return variable_ptr;
 		}
 	} while (0);
@@ -171,6 +175,14 @@ static zend_always_inline zval* zend_assign_to_variable(zval *variable_ptr, zval
 	return variable_ptr;
 }
 
+static zend_always_inline zval* zend_assign_to_variable(zval *variable_ptr, zval *value, zend_uchar value_type, bool strict)
+{
+	zend_refcounted *garbage = NULL;
+	variable_ptr = zend_assign_to_variable_delayed_garbage_handling(variable_ptr, value, value_type, strict, &garbage);
+	zend_assign_to_variable_handle_garbage(garbage);
+	return variable_ptr;
+}
+
 ZEND_API zend_result ZEND_FASTCALL zval_update_constant(zval *pp);
 ZEND_API zend_result ZEND_FASTCALL zval_update_constant_ex(zval *pp, zend_class_entry *scope);
 
diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h
index 9a1d00d6c7..49dc6b65c5 100644
--- a/Zend/zend_vm_def.h
+++ b/Zend/zend_vm_def.h
@@ -2584,6 +2584,9 @@ ZEND_VM_C_LABEL(try_assign_dim_array):
 					Z_ADDREF_P(value);
 				}
 			}
+			if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
+				ZVAL_COPY(EX_VAR(opline->result.var), value);
+			}
 		} else {
 			dim = GET_OP2_ZVAL_PTR_UNDEF(BP_VAR_R);
 			if (OP2_TYPE == IS_CONST) {
@@ -2595,10 +2598,12 @@ ZEND_VM_C_LABEL(try_assign_dim_array):
 				ZEND_VM_C_GOTO(assign_dim_error);
 			}
 			value = GET_OP_DATA_ZVAL_PTR(BP_VAR_R);
-			value = zend_assign_to_variable(variable_ptr, value, OP_DATA_TYPE, EX_USES_STRICT_TYPES());
-		}
-		if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
-			ZVAL_COPY(EX_VAR(opline->result.var), value);
+			zend_refcounted *garbage = NULL;
+			value = zend_assign_to_variable_delayed_garbage_handling(variable_ptr, value, OP_DATA_TYPE, EX_USES_STRICT_TYPES(), &garbage);
+			if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
+				ZVAL_COPY(EX_VAR(opline->result.var), value);
+			}
+			zend_assign_to_variable_handle_garbage(garbage);
 		}
 	} else {
 		if (EXPECTED(Z_ISREF_P(object_ptr))) {

[nielsdos]

Hey @iluuu1994
I still have my PoC patch (see my last post #10168 (comment)) which I have now finished into a full patch to fix this issue. However, I see you are now assigned to this issue. Is it still worth it to post my patch/PR, or did you already start on your own fix?

[iluuu1994]

@nielsdos Go ahead, I unassigned the issue. 🙂