From 727e26f9f27ed0737fdbf6d2626d37a916e08c22 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 4 Dec 2022 21:59:18 +0100 Subject: [PATCH] Fix #97836 and #81705: Segfault / type confusion in concat_function The following sequence of actions was happening which caused a null pointer dereference: 1. debug_backtrace() returns an array 2. The concatenation to $c will transform the array to a string via `zval_get_string_func` for op2 and output a warning. Note that zval op1 is of type string due to the first do-while sequence. 3. The warning of an implicit "array to string conversion" triggers the ob_start callback to run. This code transform $c (==op1) to a long. 4. The code below the 2 do-while sequences assume that both op1 and op2 are strings, but this is no longer the case. A dereference of the string will therefore result in a null pointer dereference. The solution used here is to work with the zend_string directly instead of with the ops. For the tests: Co-authored-by: changochen1@gmail.com Co-authored-by: cmbecker69@gmx.de Co-authored-by: yukik@risec.co.jp Closes GH-10049. --- NEWS | 3 + Zend/tests/bug79836.phpt | 18 +++ Zend/tests/bug79836_1.phpt | 18 +++ Zend/tests/bug79836_2.phpt | 25 ++++ Zend/tests/bug81705.phpt | 19 +++ ...tring_concat_non_interned_with_itself.phpt | 21 ++++ .../class_toString_concat_with_itself.phpt | 16 +++ Zend/zend_operators.c | 118 ++++++++++++------ 8 files changed, 198 insertions(+), 40 deletions(-) create mode 100644 Zend/tests/bug79836.phpt create mode 100644 Zend/tests/bug79836_1.phpt create mode 100644 Zend/tests/bug79836_2.phpt create mode 100644 Zend/tests/bug81705.phpt create mode 100644 Zend/tests/class_toString_concat_non_interned_with_itself.phpt create mode 100644 Zend/tests/class_toString_concat_with_itself.phpt diff --git a/NEWS b/NEWS index b2527affb28ba..53e4fe73519b2 100644 --- a/NEWS +++ b/NEWS @@ -37,6 +37,9 @@ PHP NEWS index). (ColinHDev) . Fix bug GH-8846 (Implement delayed early binding for classes without parents). (ilutov) + . Fix bug #79836 (Segfault in concat_function). (nielsdos) + . Fix bug #81705 (type confusion/UAF on set_error_handler with concat + operation). (nielsdos) - Date: . Implement More Appropriate Date/Time Exceptions RFC. (Derick) diff --git a/Zend/tests/bug79836.phpt b/Zend/tests/bug79836.phpt new file mode 100644 index 0000000000000..5fb07396762f5 --- /dev/null +++ b/Zend/tests/bug79836.phpt @@ -0,0 +1,18 @@ +--TEST-- +Bug #79836 (Segfault in concat_function) +--INI-- +opcache.optimization_level = 0x7FFEBFFF & ~0x400 +--FILE-- + +--EXPECT-- +3 diff --git a/Zend/tests/bug79836_1.phpt b/Zend/tests/bug79836_1.phpt new file mode 100644 index 0000000000000..86e7f47671849 --- /dev/null +++ b/Zend/tests/bug79836_1.phpt @@ -0,0 +1,18 @@ +--TEST-- +Bug #79836 (Segfault in concat_function) +--INI-- +opcache.optimization_level = 0x7FFEBFFF & ~0x400 +--FILE-- + +--EXPECT-- +Done diff --git a/Zend/tests/bug79836_2.phpt b/Zend/tests/bug79836_2.phpt new file mode 100644 index 0000000000000..b02fcc13ea11b --- /dev/null +++ b/Zend/tests/bug79836_2.phpt @@ -0,0 +1,25 @@ +--TEST-- +Bug #79836 (Segfault in concat_function) +--FILE-- + +--EXPECT-- +abcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabc diff --git a/Zend/tests/bug81705.phpt b/Zend/tests/bug81705.phpt new file mode 100644 index 0000000000000..1c00b1c77d4bb --- /dev/null +++ b/Zend/tests/bug81705.phpt @@ -0,0 +1,19 @@ +--TEST-- +Bug #81705 (type confusion/UAF on set_error_handler with concat operation) +--FILE-- + +--EXPECT-- +error +string(6) "aArray" \ No newline at end of file diff --git a/Zend/tests/class_toString_concat_non_interned_with_itself.phpt b/Zend/tests/class_toString_concat_non_interned_with_itself.phpt new file mode 100644 index 0000000000000..87b129ce9e796 --- /dev/null +++ b/Zend/tests/class_toString_concat_non_interned_with_itself.phpt @@ -0,0 +1,21 @@ +--TEST-- +Test concatenating a class instance that has __toString with itself that uses a non-interned string +--FILE-- + +--EXPECT-- +aaaaaa diff --git a/Zend/tests/class_toString_concat_with_itself.phpt b/Zend/tests/class_toString_concat_with_itself.phpt new file mode 100644 index 0000000000000..96d28679b2f93 --- /dev/null +++ b/Zend/tests/class_toString_concat_with_itself.phpt @@ -0,0 +1,16 @@ +--TEST-- +Test concatenating a class instance that has __toString with itself +--FILE-- + +--EXPECT-- +abcabc diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c index a9932a6b592b6..c4fce74ebbbee 100644 --- a/Zend/zend_operators.c +++ b/Zend/zend_operators.c @@ -1940,108 +1940,146 @@ ZEND_API zend_result ZEND_FASTCALL shift_right_function(zval *result, zval *op1, ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval *op2) /* {{{ */ { zval *orig_op1 = op1; - zval op1_copy, op2_copy; - - ZVAL_UNDEF(&op1_copy); - ZVAL_UNDEF(&op2_copy); + zend_string *op1_string, *op2_string; + bool free_op1_string = false; + bool free_op2_string = false; do { - if (UNEXPECTED(Z_TYPE_P(op1) != IS_STRING)) { + if (EXPECTED(Z_TYPE_P(op1) == IS_STRING)) { + op1_string = Z_STR_P(op1); + } else { if (Z_ISREF_P(op1)) { op1 = Z_REFVAL_P(op1); - if (Z_TYPE_P(op1) == IS_STRING) break; + if (Z_TYPE_P(op1) == IS_STRING) { + op1_string = Z_STR_P(op1); + break; + } } ZEND_TRY_BINARY_OBJECT_OPERATION(ZEND_CONCAT); - ZVAL_STR(&op1_copy, zval_get_string_func(op1)); + op1_string = zval_get_string_func(op1); if (UNEXPECTED(EG(exception))) { - zval_ptr_dtor_str(&op1_copy); + zend_string_release(op1_string); if (orig_op1 != result) { ZVAL_UNDEF(result); } return FAILURE; } + free_op1_string = true; if (result == op1) { if (UNEXPECTED(op1 == op2)) { - op2 = &op1_copy; + op2_string = op1_string; + goto has_op2_string; } } - op1 = &op1_copy; } } while (0); do { - if (UNEXPECTED(Z_TYPE_P(op2) != IS_STRING)) { - if (Z_ISREF_P(op2)) { - op2 = Z_REFVAL_P(op2); - if (Z_TYPE_P(op2) == IS_STRING) break; - } + if (EXPECTED(Z_TYPE_P(op2) == IS_STRING)) { + op2_string = Z_STR_P(op2); + } else { + if (Z_ISREF_P(op2)) { + op2 = Z_REFVAL_P(op2); + if (Z_TYPE_P(op2) == IS_STRING) { + op2_string = Z_STR_P(op2); + break; + } + } + /* hold an additional reference because a userland function could free this */ + if (!free_op1_string) { + op1_string = zend_string_copy(op1_string); + free_op1_string = true; + } ZEND_TRY_BINARY_OP2_OBJECT_OPERATION(ZEND_CONCAT); - ZVAL_STR(&op2_copy, zval_get_string_func(op2)); + op2_string = zval_get_string_func(op2); if (UNEXPECTED(EG(exception))) { - zval_ptr_dtor_str(&op1_copy); - zval_ptr_dtor_str(&op2_copy); + zend_string_release(op1_string); + zend_string_release(op2_string); if (orig_op1 != result) { ZVAL_UNDEF(result); } return FAILURE; } - op2 = &op2_copy; + free_op2_string = true; } } while (0); - if (UNEXPECTED(Z_STRLEN_P(op1) == 0)) { - if (EXPECTED(result != op2)) { +has_op2_string:; + if (UNEXPECTED(ZSTR_LEN(op1_string) == 0)) { + if (EXPECTED(free_op2_string || result != op2)) { if (result == orig_op1) { i_zval_ptr_dtor(result); } - ZVAL_COPY(result, op2); + if (free_op2_string) { + /* transfer ownership of op2_string */ + ZVAL_STR(result, op2_string); + free_op2_string = false; + } else { + ZVAL_STR_COPY(result, op2_string); + } } - } else if (UNEXPECTED(Z_STRLEN_P(op2) == 0)) { - if (EXPECTED(result != op1)) { + } else if (UNEXPECTED(ZSTR_LEN(op2_string) == 0)) { + if (EXPECTED(free_op1_string || result != op1)) { if (result == orig_op1) { i_zval_ptr_dtor(result); } - ZVAL_COPY(result, op1); + if (free_op1_string) { + /* transfer ownership of op1_string */ + ZVAL_STR(result, op1_string); + free_op1_string = false; + } else { + ZVAL_STR_COPY(result, op1_string); + } } } else { - size_t op1_len = Z_STRLEN_P(op1); - size_t op2_len = Z_STRLEN_P(op2); + size_t op1_len = ZSTR_LEN(op1_string); + size_t op2_len = ZSTR_LEN(op2_string); size_t result_len = op1_len + op2_len; zend_string *result_str; - uint32_t flags = ZSTR_GET_COPYABLE_CONCAT_PROPERTIES_BOTH(Z_STR_P(op1), Z_STR_P(op2)); + uint32_t flags = ZSTR_GET_COPYABLE_CONCAT_PROPERTIES_BOTH(op1_string, op2_string); if (UNEXPECTED(op1_len > ZSTR_MAX_LEN - op2_len)) { + if (free_op1_string) zend_string_release(op1_string); + if (free_op2_string) zend_string_release(op2_string); zend_throw_error(NULL, "String size overflow"); - zval_ptr_dtor_str(&op1_copy); - zval_ptr_dtor_str(&op2_copy); if (orig_op1 != result) { ZVAL_UNDEF(result); } return FAILURE; } - if (result == op1 && Z_REFCOUNTED_P(result)) { + if (result == op1) { + if (free_op1_string) { + /* op1_string will be used as the result, so we should not free it */ + i_zval_ptr_dtor(result); + free_op1_string = false; + } /* special case, perform operations on result */ - result_str = zend_string_extend(Z_STR_P(result), result_len, 0); + result_str = zend_string_extend(op1_string, result_len, 0); + /* account for the case where result_str == op1_string == op2_string and the realloc is done */ + if (op1_string == op2_string) { + if (free_op2_string) { + zend_string_release(op2_string); + free_op2_string = false; + } + op2_string = result_str; + } } else { result_str = zend_string_alloc(result_len, 0); - memcpy(ZSTR_VAL(result_str), Z_STRVAL_P(op1), op1_len); + memcpy(ZSTR_VAL(result_str), ZSTR_VAL(op1_string), op1_len); if (result == orig_op1) { i_zval_ptr_dtor(result); } } GC_ADD_FLAGS(result_str, flags); - /* This has to happen first to account for the cases where result == op1 == op2 and - * the realloc is done. In this case this line will also update Z_STRVAL_P(op2) to - * point to the new string. The first op2_len bytes of result will still be the same. */ ZVAL_NEW_STR(result, result_str); - - memcpy(ZSTR_VAL(result_str) + op1_len, Z_STRVAL_P(op2), op2_len); + memcpy(ZSTR_VAL(result_str) + op1_len, ZSTR_VAL(op2_string), op2_len); ZSTR_VAL(result_str)[result_len] = '\0'; } - zval_ptr_dtor_str(&op1_copy); - zval_ptr_dtor_str(&op2_copy); + if (free_op1_string) zend_string_release(op1_string); + if (free_op2_string) zend_string_release(op2_string); + return SUCCESS; } /* }}} */