From 45d3916e7a62e40ced70df22b9c7c5c67d77ff92 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 14 Aug 2023 01:41:54 +0200 Subject: [PATCH 1/3] Fix #81992: SplFixedArray::setSize() causes use-after-free Upon resizing, the elements are destroyed from lower index to higher index. When an element refers to an object with a destructor, it can refer to a lower (i.e. already destroyed) element, causing a uaf. Set refcounted zvals to NULL after destroying them to avoid a uaf. --- ext/spl/spl_fixedarray.c | 8 +++++++- ext/spl/tests/bug81992.phpt | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 ext/spl/tests/bug81992.phpt diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index 2cf5e2360adb..217a6184dcc3 100644 --- a/ext/spl/spl_fixedarray.c +++ b/ext/spl/spl_fixedarray.c @@ -150,7 +150,13 @@ static void spl_fixedarray_dtor_range(spl_fixedarray *array, zend_long from, zen { zval *begin = array->elements + from, *end = array->elements + to; while (begin != end) { - zval_ptr_dtor(begin++); + if (Z_REFCOUNTED_P(begin)) { + /* Using inline variant to get rid of the redundant check */ + i_zval_ptr_dtor(begin); + /* Get rid of the dangling zval, there might be a destructor or error handling accessing the array */ + ZVAL_NULL(begin); + } + begin++; } } diff --git a/ext/spl/tests/bug81992.phpt b/ext/spl/tests/bug81992.phpt new file mode 100644 index 000000000000..eefaae46d27b --- /dev/null +++ b/ext/spl/tests/bug81992.phpt @@ -0,0 +1,20 @@ +--TEST-- +Bug #81992 (SplFixedArray::setSize() causes use-after-free) +--FILE-- +setSize(2); +?> +--EXPECT-- +NULL +string(10) "CCCCCCCCCC" From 9358ab01c1f3500eadea9d255386adf1489e468d Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 14 Aug 2023 13:16:30 +0200 Subject: [PATCH 2/3] Fix using invalid index, fixes another problem too --- ext/spl/spl_fixedarray.c | 13 ++++--------- ext/spl/tests/bug81992.phpt | 18 +++++++++++++++--- ext/spl/tests/bug81992b.phpt | 23 +++++++++++++++++++++++ 3 files changed, 42 insertions(+), 12 deletions(-) create mode 100644 ext/spl/tests/bug81992b.phpt diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index 217a6184dcc3..56e72b9f0e59 100644 --- a/ext/spl/spl_fixedarray.c +++ b/ext/spl/spl_fixedarray.c @@ -148,15 +148,10 @@ static void spl_fixedarray_copy_ctor(spl_fixedarray *to, spl_fixedarray *from) */ static void spl_fixedarray_dtor_range(spl_fixedarray *array, zend_long from, zend_long to) { + array->size = from; zval *begin = array->elements + from, *end = array->elements + to; while (begin != end) { - if (Z_REFCOUNTED_P(begin)) { - /* Using inline variant to get rid of the redundant check */ - i_zval_ptr_dtor(begin); - /* Get rid of the dangling zval, there might be a destructor or error handling accessing the array */ - ZVAL_NULL(begin); - } - begin++; + zval_ptr_dtor(begin++); } } @@ -194,15 +189,15 @@ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) if (size == 0) { spl_fixedarray_dtor(array); array->elements = NULL; + array->size = 0; } else if (size > array->size) { array->elements = safe_erealloc(array->elements, size, sizeof(zval), 0); spl_fixedarray_init_elems(array, array->size, size); + array->size = size; } else { /* size < array->size */ spl_fixedarray_dtor_range(array, size, array->size); array->elements = erealloc(array->elements, sizeof(zval) * size); } - - array->size = size; } static HashTable* spl_fixedarray_object_get_gc(zend_object *obj, zval **table, int *n) diff --git a/ext/spl/tests/bug81992.phpt b/ext/spl/tests/bug81992.phpt index eefaae46d27b..52235218a78f 100644 --- a/ext/spl/tests/bug81992.phpt +++ b/ext/spl/tests/bug81992.phpt @@ -5,16 +5,28 @@ Bug #81992 (SplFixedArray::setSize() causes use-after-free) class InvalidDestructor { public function __destruct() { global $obj; - var_dump($obj[2], $obj[4]); + var_dump($obj[0]); + try { + var_dump($obj[2]); + } catch (Throwable $e) { + echo $e->getMessage(), "\n"; + } + try { + var_dump($obj[4]); + } catch (Throwable $e) { + echo $e->getMessage(), "\n"; + } } } $obj = new SplFixedArray(5); +$obj[0] = str_repeat("A", 10); $obj[2] = str_repeat('B', 10); $obj[3] = new InvalidDestructor(); $obj[4] = str_repeat('C', 10); $obj->setSize(2); ?> --EXPECT-- -NULL -string(10) "CCCCCCCCCC" +string(10) "AAAAAAAAAA" +Index invalid or out of range +Index invalid or out of range diff --git a/ext/spl/tests/bug81992b.phpt b/ext/spl/tests/bug81992b.phpt new file mode 100644 index 000000000000..374a94ec0692 --- /dev/null +++ b/ext/spl/tests/bug81992b.phpt @@ -0,0 +1,23 @@ +--TEST-- +Bug #81992 (SplFixedArray::setSize() causes use-after-free) - setSize variation +--FILE-- +setSize(1); + echo "Destroyed\n"; + } +} + +$obj = new SplFixedArray(5); +$obj[0] = str_repeat("A", 10); +$obj[2] = str_repeat('B', 10); +$obj[3] = new InvalidDestructor(); +$obj[4] = str_repeat('C', 10); +$obj->setSize(2); +echo "Done\n"; +?> +--EXPECT-- +Destroyed +Done From 439934250f36dbf8de5bc671abed72995b3cd91a Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 14 Aug 2023 13:53:39 +0200 Subject: [PATCH 3/3] Properly fix resize uaf --- ext/spl/spl_fixedarray.c | 19 +++++++++++ ext/spl/tests/bug81992b.phpt | 65 ++++++++++++++++++++++++++++++------ 2 files changed, 73 insertions(+), 11 deletions(-) diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index 56e72b9f0e59..562806a5c284 100644 --- a/ext/spl/spl_fixedarray.c +++ b/ext/spl/spl_fixedarray.c @@ -46,6 +46,8 @@ typedef struct _spl_fixedarray { zval *elements; /* True if this was modified after the last call to get_properties or the hash table wasn't rebuilt. */ bool should_rebuild_properties; + /* If positive, it's a resize within a resize and the value gives the desired size. If -1, it's not. */ + zend_long cached_resize; } spl_fixedarray; typedef struct _spl_fixedarray_methods { @@ -117,6 +119,7 @@ static void spl_fixedarray_init(spl_fixedarray *array, zend_long size) } else { spl_fixedarray_default_ctor(array); } + array->cached_resize = -1; } /* Copies the range [begin, end) into the fixedarray, beginning at `offset`. @@ -185,6 +188,14 @@ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) return; } + if (UNEXPECTED(array->cached_resize >= 0)) { + /* We're already resizing, so just remember the desired size. + * The resize will happen later. */ + array->cached_resize = size; + return; + } + array->cached_resize = size; + /* clearing the array */ if (size == 0) { spl_fixedarray_dtor(array); @@ -195,9 +206,17 @@ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) spl_fixedarray_init_elems(array, array->size, size); array->size = size; } else { /* size < array->size */ + /* Size set in spl_fixedarray_dtor_range() */ spl_fixedarray_dtor_range(array, size, array->size); array->elements = erealloc(array->elements, sizeof(zval) * size); } + + /* If resized within the destructor, take the last resize command and perform it */ + zend_long cached_resize = array->cached_resize; + array->cached_resize = -1; + if (cached_resize != size) { + spl_fixedarray_resize(array, cached_resize); + } } static HashTable* spl_fixedarray_object_get_gc(zend_object *obj, zval **table, int *n) diff --git a/ext/spl/tests/bug81992b.phpt b/ext/spl/tests/bug81992b.phpt index 374a94ec0692..ba4736dfff1f 100644 --- a/ext/spl/tests/bug81992b.phpt +++ b/ext/spl/tests/bug81992b.phpt @@ -3,21 +3,64 @@ Bug #81992 (SplFixedArray::setSize() causes use-after-free) - setSize variation --FILE-- obj->setSize($this->desiredSize); + echo "Destroyed, size is now still ", $this->obj->getSize(), "\n"; + } +} + +class DestructorLogger { + public function __construct(private int $id) {} + public function __destruct() { - global $obj; - $obj->setSize(1); - echo "Destroyed\n"; + echo "Destroyed the logger with id ", $this->id, "\n"; } } -$obj = new SplFixedArray(5); -$obj[0] = str_repeat("A", 10); -$obj[2] = str_repeat('B', 10); -$obj[3] = new InvalidDestructor(); -$obj[4] = str_repeat('C', 10); -$obj->setSize(2); -echo "Done\n"; +function test(int $desiredSize) { + $obj = new SplFixedArray(5); + $obj[0] = str_repeat("A", 10); + $obj[1] = new DestructorLogger(1); + $obj[2] = str_repeat('B', 10); + $obj[3] = new InvalidDestructor($desiredSize, $obj); + $obj[4] = new DestructorLogger(4); + $obj->setSize(2); + echo "Size is now ", $obj->getSize(), "\n"; + echo "Done\n"; +} + +echo "--- Smaller size test ---\n"; +test(1); +echo "--- Equal size test ---\n"; +test(2); +echo "--- Larger size test ---\n"; +test(10); ?> --EXPECT-- -Destroyed +--- Smaller size test --- +In destructor +Destroyed, size is now still 2 +Destroyed the logger with id 4 +Destroyed the logger with id 1 +Size is now 1 +Done +--- Equal size test --- +In destructor +Destroyed, size is now still 2 +Destroyed the logger with id 4 +Size is now 2 +Done +Destroyed the logger with id 1 +--- Larger size test --- +In destructor +Destroyed, size is now still 2 +Destroyed the logger with id 4 +Size is now 10 Done +Destroyed the logger with id 1