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.
php · May 16, 2023 · 727e26f · 727e26f · dstogov · May 22, 2023
1 parent 7914b8c
commit 727e26f
Show file tree

Hide file tree

Showing 8 changed files with 198 additions and 40 deletions.
diff --git 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
@@ -0,0 +1,18 @@
+--TEST--
+Bug #79836 (Segfault in concat_function)
+--INI--
+opcache.optimization_level = 0x7FFEBFFF & ~0x400
+--FILE--
+<?php
+$counter = 0;
+ob_start(function ($buffer) use (&$c, &$counter) {
+	$c = 0;
+	++$counter;
+}, 1);
+$c .= [];
+$c .= [];
+ob_end_clean();
+echo $counter . "\n";
+?>
+--EXPECT--
+3
diff --git a/Zend/tests/bug79836_1.phpt b/Zend/tests/bug79836_1.phpt
@@ -0,0 +1,18 @@
+--TEST--
+Bug #79836 (Segfault in concat_function)
+--INI--
+opcache.optimization_level = 0x7FFEBFFF & ~0x400
+--FILE--
+<?php
+$x = 'non-empty';
+ob_start(function () use (&$c) {
+	$c = 0;
+}, 1);
+$c = [];
+$x = $c . $x;
+$x = $c . $x;
+ob_end_clean();
+echo "Done\n";
+?>
+--EXPECT--
+Done
diff --git a/Zend/tests/bug79836_2.phpt b/Zend/tests/bug79836_2.phpt
@@ -0,0 +1,25 @@
+--TEST--
+Bug #79836 (Segfault in concat_function)
+--FILE--
+<?php
+$c = str_repeat("abcd", 10);
+
+ob_start(function () use (&$c) {
+	$c = 0;
+}, 1);
+
+class X {
+	function __toString() {
+		echo "a";
+		return "abc";
+	}
+}
+
+$xxx = new X;
+
+$x = $c . $xxx;
+ob_end_clean();
+echo $x . "\n";
+?>
+--EXPECT--
+abcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabc
diff --git a/Zend/tests/bug81705.phpt b/Zend/tests/bug81705.phpt
@@ -0,0 +1,19 @@
+--TEST--
+Bug #81705 (type confusion/UAF on set_error_handler with concat operation)
+--FILE--
+<?php
+
+$arr = [0];
+$my_var = str_repeat("a", 1);
+set_error_handler(
+    function() use(&$my_var) {
+        echo("error\n");
+        $my_var = 0x123;
+    }
+);
+$my_var .= $GLOBALS["arr"];
+var_dump($my_var);
+?>
+--EXPECT--
+error
+string(6) "aArray"
diff --git a/Zend/tests/class_toString_concat_non_interned_with_itself.phpt 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--
+<?php
+$global_non_interned_string = str_repeat("a", 3);
+
+class Test {
+	public function __toString() {
+		global $global_non_interned_string;
+		return $global_non_interned_string;
+	}
+}
+
+$test1 = new Test;
+$test2 = new Test;
+$test1 .= $test2;
+
+echo $test1 . "\n";
+?>
+--EXPECT--
+aaaaaa
diff --git a/Zend/tests/class_toString_concat_with_itself.phpt 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--
+<?php
+class Tmp {
+	public function __toString() {
+		return "abc";
+	}
+}
+
+$tmp = new Tmp;
+$tmp .= $tmp;
+echo $tmp . "\n";
+?>
+--EXPECT--
+abcabc
diff --git 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;
 }
 /* }}} */