diff --git a/NEWS b/NEWS index 4efc8258fc2d..68c96424a9ce 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,8 @@ PHP NEWS - Core: . Fixed incorrect internal pointer and foreach iterator positions when compacting arrays with holes. (Weilin Du) + . Fix handling of references to typed properties during unserialization + of various internal classes. (ndossche, timwolla) - DOM: . Fixed use-after-free when re-constructing a DOMXPath whose php:function diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS index 31995eb80752..8bbdc5caabe5 100644 --- a/UPGRADING.INTERNALS +++ b/UPGRADING.INTERNALS @@ -182,6 +182,8 @@ PHP 8.6 INTERNALS UPGRADE NOTES instead of a zval*. Accordingly, zend_get_closure_this_ptr() now returns that zend_object*, or NULL when the closure is unbound, instead of a zval* that is IS_UNDEF when the closure is unbound. + . object_properties_load() now verifies that the given value is assignable + to typed properties. The check is performed in strict mode. - Added: . New zend_class_entry.ce_flags2 and zend_function.fn_flags2 fields were diff --git a/Zend/zend_API.c b/Zend/zend_API.c index 43e21cafd564..4f84404919fa 100644 --- a/Zend/zend_API.c +++ b/Zend/zend_API.c @@ -1758,7 +1758,7 @@ ZEND_API void object_properties_load(zend_object *object, const HashTable *prope zval *prop, tmp; zend_string *key; zend_long h; - const zend_property_info *property_info; + zend_property_info *property_info; ZEND_HASH_FOREACH_KEY_VAL(properties, h, key, prop) { if (key) { @@ -1785,18 +1785,56 @@ ZEND_API void object_properties_load(zend_object *object, const HashTable *prope if (property_info != ZEND_WRONG_PROPERTY_INFO && property_info && (property_info->flags & ZEND_ACC_STATIC) == 0) { + bool is_typed = ZEND_TYPE_IS_SET(property_info->type); + + /* Mimick unserialize behaviour for virtual properties. */ + if (UNEXPECTED(property_info->flags & ZEND_ACC_VIRTUAL)) { + zend_throw_error(NULL, "Cannot unserialize value for virtual property %s::$%s", ZSTR_VAL(object->ce->name), zend_get_unmangled_property_name(property_info->name)); + return; + } + zval *slot = OBJ_PROP(object, property_info->offset); - if (UNEXPECTED((property_info->flags & ZEND_ACC_READONLY) && !Z_ISUNDEF_P(slot))) { - if (Z_PROP_FLAG_P(slot) & IS_PROP_REINITABLE) { - Z_PROP_FLAG_P(slot) &= ~IS_PROP_REINITABLE; + zval val; + + if (is_typed) { + if (UNEXPECTED(Z_ISREF_P(prop))) { + /* Block taking a reference to a readonly property. */ + if (UNEXPECTED(property_info->flags & ZEND_ACC_READONLY)) { + zend_readonly_property_indirect_modification_error(property_info); + return; + } + if (UNEXPECTED(!zend_verify_prop_assignable_by_ref(property_info, prop, /* strict */ true))) { + ZEND_ASSERT(EG(exception)); + return; + } + ZVAL_COPY(&val, prop); + ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(&val), property_info); } else { - zend_readonly_property_modification_error(property_info); - return; + /* Mimick zend_assign_to_typed_prop() by reporting the error before doing work. */ + if (UNEXPECTED((property_info->flags & ZEND_ACC_READONLY) + && !Z_ISUNDEF_P(slot) + && !(Z_PROP_FLAG_P(slot) & IS_PROP_REINITABLE))) { + zend_readonly_property_modification_error(property_info); + return; + } + + ZVAL_COPY(&val, prop); + if (UNEXPECTED(!zend_verify_property_type(property_info, &val, /* strict */ true))) { + zval_ptr_dtor(&val); + return; + } } + if (UNEXPECTED(Z_ISREF_P(slot)) + && (ZEND_DEBUG || ZEND_REF_HAS_TYPE_SOURCES(Z_REF_P(slot)))) { + ZEND_REF_DEL_TYPE_SOURCE(Z_REF_P(slot), property_info); + } + } else { + ZVAL_COPY(&val, prop); } + + Z_PROP_FLAG_P(slot) &= ~IS_PROP_REINITABLE; zval_ptr_dtor(slot); - ZVAL_COPY_VALUE(slot, prop); - zval_add_ref(slot); + ZVAL_COPY_VALUE(slot, &val); if (object->properties) { ZVAL_INDIRECT(&tmp, slot); zend_hash_update(object->properties, key, &tmp); diff --git a/ext/date/tests/time/duration/gh23639.phpt b/ext/date/tests/time/duration/gh23639.phpt new file mode 100644 index 000000000000..adb9d4fc71f8 --- /dev/null +++ b/ext/date/tests/time/duration/gh23639.phpt @@ -0,0 +1,34 @@ +--TEST-- +GH-23639 (object_properties_load allows creating readonly reference properties) +--CREDITS-- +arnaud-lb +ndossche +--XFAIL-- +Test can only succeed when GH-23629 is also merged +--FILE-- +seconds = 1; +$a = [$d, &$d->seconds]; + +$payload = serialize($a); + +try { + unserialize(str_replace('Time_Duration', 'Time\\Duration', $payload)); +} catch (Throwable $e) { + do { + echo $e::class, ": ", $e->getMessage(), "\n"; + } while ($e = $e->getPrevious()); +} + +?> +--EXPECT-- +Exception: Invalid serialization data for Time\Duration object +Error: Cannot indirectly modify readonly property Time\Duration::$seconds diff --git a/ext/random/randomizer.c b/ext/random/randomizer.c index 0738380ca253..4c5ff34c9fea 100644 --- a/ext/random/randomizer.c +++ b/ext/random/randomizer.c @@ -508,7 +508,6 @@ PHP_METHOD(Random_Randomizer, __unserialize) php_random_randomizer *randomizer = Z_RANDOM_RANDOMIZER_P(ZEND_THIS); HashTable *d; zval *members_zv; - zval *zengine; ZEND_PARSE_PARAMETERS_START(1, 1) Z_PARAM_ARRAY_HT(d); @@ -531,12 +530,7 @@ PHP_METHOD(Random_Randomizer, __unserialize) RETURN_THROWS(); } - zengine = zend_read_property(randomizer->std.ce, &randomizer->std, "engine", strlen("engine"), 1, NULL); - if (Z_TYPE_P(zengine) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(zengine), random_ce_Random_Engine)) { - zend_throw_exception(NULL, "Invalid serialization data for Random\\Randomizer object", 0); - RETURN_THROWS(); - } - + zval *zengine = zend_read_property(randomizer->std.ce, &randomizer->std, "engine", strlen("engine"), /* silent */ true, NULL); randomizer_common_init(randomizer, Z_OBJ_P(zengine)); } /* }}} */ diff --git a/ext/random/tests/03_randomizer/gh_9708_unserialize.phpt b/ext/random/tests/03_randomizer/gh_9708_unserialize.phpt new file mode 100644 index 000000000000..c5689d7b6b94 --- /dev/null +++ b/ext/random/tests/03_randomizer/gh_9708_unserialize.phpt @@ -0,0 +1,14 @@ +--TEST-- +GH-9708: object_properties_load() bypasses typed property checks +--FILE-- +getMessage(), "\n"; +} + +?> +--EXPECT-- +Exception: Invalid serialization data for Random\Randomizer object diff --git a/ext/spl/tests/ArrayObject/gh_9707_unserialize.phpt b/ext/spl/tests/ArrayObject/gh_9707_unserialize.phpt new file mode 100644 index 000000000000..ee11c5c28e5c --- /dev/null +++ b/ext/spl/tests/ArrayObject/gh_9707_unserialize.phpt @@ -0,0 +1,41 @@ +--TEST-- +GH-9707: object_properties_load crashes in debug mode when unserializing references to typed properties in php 8.1+ +--FILE-- +a; +$f->b = &$r; + +$f->b = 1; +var_dump($unserialized = unserialize(serialize($f))); + +$unserialized->b = 2; + +var_dump($unserialized); + +?> +--EXPECTF-- +object(Foo)#%d (3) { + ["a"]=> + &int(1) + ["b"]=> + &int(1) + ["storage":"ArrayObject":private]=> + array(0) { + } +} +object(Foo)#%d (3) { + ["a"]=> + &int(2) + ["b"]=> + &int(2) + ["storage":"ArrayObject":private]=> + array(0) { + } +} diff --git a/ext/spl/tests/ArrayObject/gh_9708_unserialize.phpt b/ext/spl/tests/ArrayObject/gh_9708_unserialize.phpt new file mode 100644 index 000000000000..fec7ef60346c --- /dev/null +++ b/ext/spl/tests/ArrayObject/gh_9708_unserialize.phpt @@ -0,0 +1,21 @@ +--TEST-- +GH-9708: object_properties_load() bypasses typed property checks +--FILE-- +getMessage(), "\n"; +} + + +?> +--EXPECT-- +TypeError: Cannot assign string to property Foo::$a of type int