From 65ae09beab1a75a05b0eba05e0cc1e2dd9af8054 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 11 Jan 2025 11:26:59 +0100 Subject: [PATCH 1/3] Fix GH-17442: Engine UAF with reference assign and dtor --- UPGRADING.INTERNALS | 5 +++ Zend/tests/weakrefs/gh17442_1.phpt | 22 +++++++++++++ Zend/tests/weakrefs/gh17442_2.phpt | 35 ++++++++++++++++++++ Zend/zend_API.c | 3 +- Zend/zend_API.h | 53 +++++++++++++++++++----------- Zend/zend_variables.c | 14 ++++++++ Zend/zend_variables.h | 1 + 7 files changed, 111 insertions(+), 22 deletions(-) create mode 100644 Zend/tests/weakrefs/gh17442_1.phpt create mode 100644 Zend/tests/weakrefs/gh17442_2.phpt diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS index fa8c33f863731..a1b5ba1c1e088 100644 --- a/UPGRADING.INTERNALS +++ b/UPGRADING.INTERNALS @@ -14,6 +14,11 @@ PHP 8.5 INTERNALS UPGRADE NOTES 1. Internal API changes ======================== +- Zend + . Added ZEND_SAFE_ASSIGN_VALUE() macro to safely assign a value to a zval. + . Added zval_ptr_safe_dtor() to safely destroy a zval when a destructor + could interfere. + ======================== 2. Build system changes ======================== diff --git a/Zend/tests/weakrefs/gh17442_1.phpt b/Zend/tests/weakrefs/gh17442_1.phpt new file mode 100644 index 0000000000000..fc7f60174ed9e --- /dev/null +++ b/Zend/tests/weakrefs/gh17442_1.phpt @@ -0,0 +1,22 @@ +--TEST-- +GH-17442 (Engine UAF with reference assign and dtor) - untyped +--CREDITS-- +YuanchengJiang +--FILE-- + +--EXPECTF-- +Fatal error: Uncaught Exception: Test in %s:%d +Stack trace: +#0 [internal function]: class@anonymous->__destruct() +#1 %s(%d): headers_sent(NULL, 0) +#2 {main} + thrown in %s on line %d diff --git a/Zend/tests/weakrefs/gh17442_2.phpt b/Zend/tests/weakrefs/gh17442_2.phpt new file mode 100644 index 0000000000000..296a23a651383 --- /dev/null +++ b/Zend/tests/weakrefs/gh17442_2.phpt @@ -0,0 +1,35 @@ +--TEST-- +GH-17442 (Engine UAF with reference assign and dtor) - typed +--CREDITS-- +YuanchengJiang +nielsdos +--FILE-- +obj = new stdClass; + +$map[$test->obj] = new class { + function __destruct() { + global $test; + var_dump($test->obj); + throw new Exception("Test"); + } +}; + +headers_sent($test->obj); +?> +--EXPECTF-- +string(0) "" + +Fatal error: Uncaught Exception: Test in %s:%d +Stack trace: +#0 [internal function]: class@anonymous->__destruct() +#1 %s(%d): headers_sent('') +#2 {main} + thrown in %s on line %d diff --git a/Zend/zend_API.c b/Zend/zend_API.c index 8c239a952f32b..65dcbe19fcc36 100644 --- a/Zend/zend_API.c +++ b/Zend/zend_API.c @@ -4666,8 +4666,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_ex(zend_reference *ref, zval *val zval_ptr_dtor(val); return FAILURE; } else { - zval_ptr_dtor(&ref->val); - ZVAL_COPY_VALUE(&ref->val, val); + ZEND_SAFE_ASSIGN_VALUE(&ref->val, val); return SUCCESS; } } diff --git a/Zend/zend_API.h b/Zend/zend_API.h index d91da91bf299e..cbc461548d8b2 100644 --- a/Zend/zend_API.h +++ b/Zend/zend_API.h @@ -1097,6 +1097,22 @@ ZEND_API zend_result zend_try_assign_typed_ref_res(zend_reference *ref, zend_res ZEND_API zend_result zend_try_assign_typed_ref_zval(zend_reference *ref, zval *zv); ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval *zv, bool strict); +#define ZEND_SAFE_ASSIGN_VALUE(zv, value) do { \ + zval *_zv = zv; \ + zval *_value = value; \ + if (Z_REFCOUNTED_P(_zv)) { \ + zend_refcounted *rc = Z_COUNTED_P(_zv); \ + ZVAL_COPY_VALUE(_zv, _value); \ + if (!GC_DELREF(rc)) { \ + rc_dtor_func(rc); \ + } else { \ + gc_check_possible_root(rc); \ + } \ + } else { \ + ZVAL_COPY_VALUE(_zv, _value); \ + } \ +} while (0) + #define _ZEND_TRY_ASSIGN_NULL(zv, is_ref) do { \ zval *_zv = zv; \ if (is_ref || UNEXPECTED(Z_ISREF_P(_zv))) { \ @@ -1107,7 +1123,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_NULL(_zv); \ } while (0) @@ -1129,7 +1145,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_FALSE(_zv); \ } while (0) @@ -1151,7 +1167,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_TRUE(_zv); \ } while (0) @@ -1173,7 +1189,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_BOOL(_zv, bval); \ } while (0) @@ -1195,7 +1211,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_LONG(_zv, lval); \ } while (0) @@ -1217,7 +1233,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_DOUBLE(_zv, dval); \ } while (0) @@ -1239,7 +1255,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_EMPTY_STRING(_zv); \ } while (0) @@ -1261,7 +1277,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_STR(_zv, str); \ } while (0) @@ -1283,7 +1299,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_NEW_STR(_zv, str); \ } while (0) @@ -1305,7 +1321,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_STRING(_zv, string); \ } while (0) @@ -1327,7 +1343,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_STRINGL(_zv, string, len); \ } while (0) @@ -1349,7 +1365,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_ARR(_zv, arr); \ } while (0) @@ -1371,7 +1387,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_RES(_zv, res); \ } while (0) @@ -1393,7 +1409,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_COPY_VALUE(_zv, other_zv); \ } while (0) @@ -1415,7 +1431,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_COPY_VALUE(_zv, other_zv); \ } while (0) @@ -1447,7 +1463,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval } \ _zv = &ref->val; \ } \ - zval_ptr_dtor(_zv); \ + zval_ptr_safe_dtor(_zv); \ ZVAL_COPY_VALUE(_zv, other_zv); \ } while (0) @@ -1485,10 +1501,7 @@ static zend_always_inline zval *zend_try_array_init_size(zval *zv, uint32_t size } zv = &ref->val; } - zval garbage; - ZVAL_COPY_VALUE(&garbage, zv); - ZVAL_NULL(zv); - zval_ptr_dtor(&garbage); + zval_ptr_safe_dtor(zv); ZVAL_ARR(zv, arr); return zv; } diff --git a/Zend/zend_variables.c b/Zend/zend_variables.c index 27e09d7db22b1..00f10b08f80ab 100644 --- a/Zend/zend_variables.c +++ b/Zend/zend_variables.c @@ -85,6 +85,20 @@ ZEND_API void zval_ptr_dtor(zval *zval_ptr) /* {{{ */ } /* }}} */ +ZEND_API void zval_ptr_safe_dtor(zval *zval_ptr) +{ + if (Z_REFCOUNTED_P(zval_ptr)) { + zend_refcounted *ref = Z_COUNTED_P(zval_ptr); + + if (GC_DELREF(ref) == 0) { + ZVAL_NULL(zval_ptr); + rc_dtor_func(ref); + } else { + gc_check_possible_root(ref); + } + } +} + ZEND_API void zval_internal_ptr_dtor(zval *zval_ptr) /* {{{ */ { if (Z_REFCOUNTED_P(zval_ptr)) { diff --git a/Zend/zend_variables.h b/Zend/zend_variables.h index d504b0f0f5795..1cb745ca1b1dc 100644 --- a/Zend/zend_variables.h +++ b/Zend/zend_variables.h @@ -78,6 +78,7 @@ static zend_always_inline void zval_ptr_dtor_str(zval *zval_ptr) } ZEND_API void zval_ptr_dtor(zval *zval_ptr); +ZEND_API void zval_ptr_safe_dtor(zval *zval_ptr); ZEND_API void zval_internal_ptr_dtor(zval *zvalue); /* Kept for compatibility */ From a8b52713ec8200ec2f7f0a286fa051ef5270ec67 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 27 Jan 2025 19:42:06 +0100 Subject: [PATCH 2/3] Address review comment --- UPGRADING.INTERNALS | 3 ++- Zend/zend_API.c | 2 +- Zend/zend_API.h | 16 ---------------- Zend/zend_execute.h | 15 +++++++++++++++ 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS index a1b5ba1c1e088..eda6ed5eb7a26 100644 --- a/UPGRADING.INTERNALS +++ b/UPGRADING.INTERNALS @@ -15,7 +15,8 @@ PHP 8.5 INTERNALS UPGRADE NOTES ======================== - Zend - . Added ZEND_SAFE_ASSIGN_VALUE() macro to safely assign a value to a zval. + . Added zend_safe_assign_to_variable_noref() function to safely assign + a value to a non-reference zval. . Added zval_ptr_safe_dtor() to safely destroy a zval when a destructor could interfere. diff --git a/Zend/zend_API.c b/Zend/zend_API.c index 65dcbe19fcc36..90bb9370e1fdd 100644 --- a/Zend/zend_API.c +++ b/Zend/zend_API.c @@ -4666,7 +4666,7 @@ ZEND_API zend_result zend_try_assign_typed_ref_ex(zend_reference *ref, zval *val zval_ptr_dtor(val); return FAILURE; } else { - ZEND_SAFE_ASSIGN_VALUE(&ref->val, val); + zend_safe_assign_to_variable_noref(&ref->val, val); return SUCCESS; } } diff --git a/Zend/zend_API.h b/Zend/zend_API.h index cbc461548d8b2..6aeffce25d8e5 100644 --- a/Zend/zend_API.h +++ b/Zend/zend_API.h @@ -1097,22 +1097,6 @@ ZEND_API zend_result zend_try_assign_typed_ref_res(zend_reference *ref, zend_res ZEND_API zend_result zend_try_assign_typed_ref_zval(zend_reference *ref, zval *zv); ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval *zv, bool strict); -#define ZEND_SAFE_ASSIGN_VALUE(zv, value) do { \ - zval *_zv = zv; \ - zval *_value = value; \ - if (Z_REFCOUNTED_P(_zv)) { \ - zend_refcounted *rc = Z_COUNTED_P(_zv); \ - ZVAL_COPY_VALUE(_zv, _value); \ - if (!GC_DELREF(rc)) { \ - rc_dtor_func(rc); \ - } else { \ - gc_check_possible_root(rc); \ - } \ - } else { \ - ZVAL_COPY_VALUE(_zv, _value); \ - } \ -} while (0) - #define _ZEND_TRY_ASSIGN_NULL(zv, is_ref) do { \ zval *_zv = zv; \ if (is_ref || UNEXPECTED(Z_ISREF_P(_zv))) { \ diff --git a/Zend/zend_execute.h b/Zend/zend_execute.h index a1fbe049f3f99..3c5d567e67eaa 100644 --- a/Zend/zend_execute.h +++ b/Zend/zend_execute.h @@ -207,6 +207,21 @@ static zend_always_inline zval* zend_assign_to_variable_ex(zval *variable_ptr, z return variable_ptr; } +static zend_always_inline void zend_safe_assign_to_variable_noref(zval *variable_ptr, zval *value) { + if (Z_REFCOUNTED_P(variable_ptr)) { + ZEND_ASSERT(Z_TYPE_P(variable_ptr) != IS_REFERENCE); + zend_refcounted *ref = Z_COUNTED_P(variable_ptr); + ZVAL_COPY_VALUE(variable_ptr, value); + if (!GC_DELREF(ref)) { + rc_dtor_func(ref); + } else { + gc_check_possible_root(ref); + } + } else { + ZVAL_COPY_VALUE(variable_ptr, value); + } +} + 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); ZEND_API zend_result ZEND_FASTCALL zval_update_constant_with_ctx(zval *pp, zend_class_entry *scope, zend_ast_evaluate_ctx *ctx); From d93aa1b7ff864606f87d5b7185729e6214fd9811 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 28 Jan 2025 08:48:48 +0100 Subject: [PATCH 3/3] Use GC_DTOR_NO_REF --- Zend/zend_execute.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Zend/zend_execute.h b/Zend/zend_execute.h index 3c5d567e67eaa..1734269186116 100644 --- a/Zend/zend_execute.h +++ b/Zend/zend_execute.h @@ -212,11 +212,7 @@ static zend_always_inline void zend_safe_assign_to_variable_noref(zval *variable ZEND_ASSERT(Z_TYPE_P(variable_ptr) != IS_REFERENCE); zend_refcounted *ref = Z_COUNTED_P(variable_ptr); ZVAL_COPY_VALUE(variable_ptr, value); - if (!GC_DELREF(ref)) { - rc_dtor_func(ref); - } else { - gc_check_possible_root(ref); - } + GC_DTOR_NO_REF(ref); } else { ZVAL_COPY_VALUE(variable_ptr, value); }