Fix uaf of MBSTRG(all_encodings_list)

We need to remove the value from the GC buffer before freeing it. Otherwise shutdown will uaf when running the gc. Do that by switching from zend_hash_destroy to zend_array_destroy, which should also be faster for freeing members due to inlining of i_zval_ptr_dtor. Closes GH-11822
php · Jul 31, 2023 · 7364b7b · 7364b7b
1 parent 655f116
commit 7364b7b
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 2 deletions.
diff --git a/NEWS b/NEWS
@@ -15,6 +15,9 @@ PHP                                                                        NEWS
 - FFI:
   . Fix leaking definitions when using FFI::cdef()->new(...). (ilutov)
 
+- MBString:
+  . Fix use-after-free of mb_list_encodings() return value. (ilutov)
+
 - Streams:
   . Fixed bug GH-11735 (Use-after-free when unregistering user stream wrapper
     from itself). (ilutov)

diff --git a/ext/mbstring/mbstring.c b/ext/mbstring/mbstring.c
@@ -1159,8 +1159,7 @@ PHP_RSHUTDOWN_FUNCTION(mbstring)
 
 	if (MBSTRG(all_encodings_list)) {
 		GC_DELREF(MBSTRG(all_encodings_list));
-		zend_hash_destroy(MBSTRG(all_encodings_list));
-		efree(MBSTRG(all_encodings_list));
+		zend_array_destroy(MBSTRG(all_encodings_list));
 		MBSTRG(all_encodings_list) = NULL;
 	}
 

diff --git a/ext/mbstring/tests/mb_list_encodings_gc_uaf.phpt b/ext/mbstring/tests/mb_list_encodings_gc_uaf.phpt
@@ -0,0 +1,9 @@
+--TEST--
+Use-after-free of MBSTRG(all_encodings_list) on shutdown
+--EXTENSIONS--
+mbstring
+--FILE--
+<?php
+mb_list_encodings();
+?>
+--EXPECT--