Skip to content

Commit

Permalink
Fix GH-10168: heap-buffer-overflow at zval_undefined_cv
Browse files Browse the repository at this point in the history
The problem is that we're using the variable_ptr in the opcode handler
*after* it has already been destroyed. The solution is to create a
specialised version of zend_assign_to_variable which takes in two
destination zval pointers.

Closes GH-10524
  • Loading branch information
nielsdos authored and iluuu1994 committed Feb 8, 2023
1 parent e6281db commit 71ddede
Show file tree
Hide file tree
Showing 8 changed files with 486 additions and 193 deletions.
2 changes: 2 additions & 0 deletions NEWS
Expand Up @@ -10,6 +10,8 @@ PHP NEWS
Generator emits an unavoidable fatal error or crashes). (Arnaud)
. Fixed bug GH-10437 (Segfault/assertion when using fibers in shutdown
function after bailout). (trowski)
. Fixed bug GH-10168: use-after-free when utilizing assigned object freed
during assignment. (nielsdos)

- Date:
. Fix GH-10447 ('p' format specifier does not yield 'Z' for 00:00). (Derick)
Expand Down
32 changes: 32 additions & 0 deletions Zend/tests/gh10168_1.phpt
@@ -0,0 +1,32 @@
--TEST--
GH-10168 (heap-buffer-overflow at zval_undefined_cv): array variation
--FILE--
<?php

class Test
{
static $instances;
public function __construct() {
(self::$instances[NULL] = $this) > 0;
var_dump(self::$instances);
}

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

?>
--EXPECTF--
Notice: Object of class Test could not be converted to int in %s on line %d
array(1) {
[""]=>
object(Test)#1 (0) {
}
}

Notice: Object of class Test could not be converted to int in %s on line %d
array(0) {
}
32 changes: 32 additions & 0 deletions Zend/tests/gh10168_2.phpt
@@ -0,0 +1,32 @@
--TEST--
GH-10168 (heap-buffer-overflow at zval_undefined_cv): assign global variation
--FILE--
<?php

$a = null;

class Test
{
public function __construct() {
($GLOBALS['a'] = $this) > 0;
// Destructor called after comparison, so a will be NULL
var_dump($GLOBALS['a']);
}

function __destruct() {
unset($GLOBALS['a']);
}
}
new Test();
new Test();

?>
--EXPECTF--
Notice: Object of class Test could not be converted to int in %s on line %d
object(Test)#1 (0) {
}

Notice: Object of class Test could not be converted to int in %s on line %d

Warning: Undefined global variable $a in %s on line %d
NULL
29 changes: 29 additions & 0 deletions Zend/tests/gh10168_3.phpt
@@ -0,0 +1,29 @@
--TEST--
GH-10168 (heap-buffer-overflow at zval_undefined_cv): assign typed prop
--FILE--
<?php
class Test
{
static ?Test $a = null;

public function __construct() {
if (self::$a === null) {
var_dump(self::$a = &$this);
} else {
var_dump(self::$a = $this);
}
}

function __destruct() {
self::$a = null;
}
}
new Test();
new Test();

?>
--EXPECTF--
object(Test)#1 (0) {
}
object(Test)#2 (0) {
}
10 changes: 9 additions & 1 deletion Zend/zend_execute.c
Expand Up @@ -3562,7 +3562,7 @@ static zend_always_inline void i_zval_ptr_dtor_noref(zval *zval_ptr) {
}
}

ZEND_API zval* zend_assign_to_typed_ref(zval *variable_ptr, zval *orig_value, zend_uchar value_type, bool strict)
ZEND_API zval* zend_assign_to_typed_ref_and_result(zval *variable_ptr, zval *orig_value, zend_uchar value_type, bool strict, zval *result_variable_ptr)
{
bool ret;
zval value;
Expand All @@ -3582,6 +3582,9 @@ ZEND_API zval* zend_assign_to_typed_ref(zval *variable_ptr, zval *orig_value, ze
} else {
zval_ptr_dtor_nogc(&value);
}
if (result_variable_ptr) {
ZVAL_COPY(result_variable_ptr, variable_ptr);
}
if (value_type & (IS_VAR|IS_TMP_VAR)) {
if (UNEXPECTED(ref)) {
if (UNEXPECTED(GC_DELREF(ref) == 0)) {
Expand All @@ -3595,6 +3598,11 @@ ZEND_API zval* zend_assign_to_typed_ref(zval *variable_ptr, zval *orig_value, ze
return variable_ptr;
}

ZEND_API zval* zend_assign_to_typed_ref(zval *variable_ptr, zval *orig_value, zend_uchar value_type, bool strict)
{
return zend_assign_to_typed_ref_and_result(variable_ptr, orig_value, value_type, strict, NULL);
}

ZEND_API bool ZEND_FASTCALL zend_verify_prop_assignable_by_ref(zend_property_info *prop_info, zval *orig_val, bool strict) {
zval *val = orig_val;
if (Z_ISREF_P(val) && ZEND_REF_HAS_TYPE_SOURCES(Z_REF_P(val))) {
Expand Down
50 changes: 41 additions & 9 deletions Zend/zend_execute.h
Expand Up @@ -108,6 +108,7 @@ ZEND_API bool zend_verify_internal_return_type(zend_function *zf, zval *ret);
ZEND_API void ZEND_FASTCALL zend_ref_add_type_source(zend_property_info_source_list *source_list, zend_property_info *prop);
ZEND_API void ZEND_FASTCALL zend_ref_del_type_source(zend_property_info_source_list *source_list, zend_property_info *prop);

ZEND_API zval* zend_assign_to_typed_ref_and_result(zval *variable_ptr, zval *orig_value, zend_uchar value_type, bool strict, zval *result_variable_ptr);
ZEND_API zval* zend_assign_to_typed_ref(zval *variable_ptr, zval *value, zend_uchar value_type, bool strict);

static zend_always_inline void zend_copy_to_variable(zval *variable_ptr, zval *value, zend_uchar value_type)
Expand Down Expand Up @@ -137,12 +138,22 @@ static zend_always_inline void zend_copy_to_variable(zval *variable_ptr, zval *v
}
}

static zend_always_inline void zend_handle_garbage_from_variable_assignment(zend_refcounted *garbage)
{
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(zval *variable_ptr, zval *value, zend_uchar value_type, bool strict)
{
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);
Expand All @@ -153,21 +164,42 @@ static zend_always_inline zval* zend_assign_to_variable(zval *variable_ptr, zval
break;
}
}
garbage = Z_COUNTED_P(variable_ptr);
zend_refcounted *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);
zend_handle_garbage_from_variable_assignment(garbage);
return variable_ptr;
}
} while (0);

zend_copy_to_variable(variable_ptr, value, value_type);
return variable_ptr;
}

static zend_always_inline zval* zend_assign_to_two_variables(zval *result_variable_ptr, zval *variable_ptr, zval *value, zend_uchar value_type, bool strict)
{
do {
if (UNEXPECTED(Z_REFCOUNTED_P(variable_ptr))) {
if (Z_ISREF_P(variable_ptr)) {
if (UNEXPECTED(ZEND_REF_HAS_TYPE_SOURCES(Z_REF_P(variable_ptr)))) {
variable_ptr = zend_assign_to_typed_ref_and_result(variable_ptr, value, value_type, strict, result_variable_ptr);
return variable_ptr;
}

variable_ptr = Z_REFVAL_P(variable_ptr);
if (EXPECTED(!Z_REFCOUNTED_P(variable_ptr))) {
break;
}
}
zend_refcounted *garbage = Z_COUNTED_P(variable_ptr);
zend_copy_to_variable(variable_ptr, value, value_type);
ZVAL_COPY(result_variable_ptr, variable_ptr);
zend_handle_garbage_from_variable_assignment(garbage);
return variable_ptr;
}
} while (0);

zend_copy_to_variable(variable_ptr, value, value_type);
ZVAL_COPY(result_variable_ptr, variable_ptr);
return variable_ptr;
}

Expand Down
20 changes: 13 additions & 7 deletions Zend/zend_vm_def.h
Expand Up @@ -2577,6 +2577,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) {
Expand All @@ -2588,10 +2591,11 @@ 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);
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
zend_assign_to_two_variables(EX_VAR(opline->result.var), variable_ptr, value, OP_DATA_TYPE, EX_USES_STRICT_TYPES());
} else {
zend_assign_to_variable(variable_ptr, value, OP_DATA_TYPE, EX_USES_STRICT_TYPES());
}
}
} else {
if (EXPECTED(Z_ISREF_P(object_ptr))) {
Expand Down Expand Up @@ -2685,12 +2689,14 @@ ZEND_VM_HANDLER(22, ZEND_ASSIGN, VAR|CV, CONST|TMP|VAR|CV, SPEC(RETVAL))
value = GET_OP2_ZVAL_PTR(BP_VAR_R);
variable_ptr = GET_OP1_ZVAL_PTR_PTR_UNDEF(BP_VAR_W);

value = zend_assign_to_variable(variable_ptr, value, OP2_TYPE, EX_USES_STRICT_TYPES());
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
ZVAL_COPY(EX_VAR(opline->result.var), value);
zend_assign_to_two_variables(EX_VAR(opline->result.var), variable_ptr, value, OP2_TYPE, EX_USES_STRICT_TYPES());
} else {
zend_assign_to_variable(variable_ptr, value, OP2_TYPE, EX_USES_STRICT_TYPES());
}

FREE_OP1_VAR_PTR();
/* zend_assign_to_variable() always takes care of op2, never free it! */
/* zend_assign_to_(two_)variable(s)() always takes care of op2, never free it! */

ZEND_VM_NEXT_OPCODE_CHECK_EXCEPTION();
}
Expand Down

2 comments on commit 71ddede

@Girgias
Copy link
Member

@Girgias Girgias commented on 71ddede Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zend/tests/gh10168_3.phpt fails on master on 32bits.

@iluuu1994
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Girgias Thanks, I'm on it.

Please sign in to comment.