From 7364b7bc0b3df6b5ed67cbf86c78e4da1bdb6930 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Sat, 29 Jul 2023 17:03:20 +0200 Subject: [PATCH] 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 --- NEWS | 3 +++ ext/mbstring/mbstring.c | 3 +-- ext/mbstring/tests/mb_list_encodings_gc_uaf.phpt | 9 +++++++++ 3 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 ext/mbstring/tests/mb_list_encodings_gc_uaf.phpt diff --git a/NEWS b/NEWS index 982898615a3..9506c5960fd 100644 --- 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 index 13ad7952b98..3e59806b867 100644 --- 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 new file mode 100644 index 00000000000..4a83372b167 --- /dev/null +++ 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-- + +--EXPECT--