zend_API: Verify property types in object_properties_load()

[TimWolla]

Fixes #9708. Fixes #9707.

Dependency for #23629.

[ndossche]

This broke now:

<?php
class Foo extends ArrayObject {
    public int $a = 0;
    public int $b = 0;
}
$f = new Foo();
$r = &$f->a;
$f->b = &$r;
unserialize(serialize($f));

Furthermore, since we use weak coercion in the new code, userland code can run (e.g. via toString) and access partially-initialized objects that way.

zend_assign_to_typed_prop may need to be used.

In general, I fear that object_properties_load in serialization context was never a safe choice because it allows making readonly refs, but I didn't check this. May be solvable though.

I also found that:

• Virtual property hooks are incompatible because the offset of the slot will be -1, again. We should deprecate property hooks.
• The code of this API is ad-hoc and ugly.

[ndossche]

This is orthogonal but may be worth investigating: I think that this sequence is dangerous as well due to destructor behaviour, should probably use the zend_safe_assign_to_variable_noref or alike

[ndossche]

zend_verify_property_type operates in-place, so it can mutate constants or shm data.

[ndossche]

The following, on top of this PR, fixes everything but the coercion-causing-userland-code-to-run issue:

diff --git a/Zend/zend_API.c b/Zend/zend_API.c
index 3b4c49535b1..72fa3c634ae 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,22 +1785,52 @@ 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;
+
+				/* 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 val;
+
+				if (is_typed) {
+					if (UNEXPECTED(Z_ISREF_P(prop))) {
+						if (UNEXPECTED(!zend_verify_prop_assignable_by_ref(property_info, prop, /* strict */ false))) {
+							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;
+						ZVAL_COPY(&val, prop);
+						if (UNEXPECTED(!zend_verify_property_type(property_info, &val, /* strict */ false))) {
+							zval_ptr_dtor(&val);
+							return;
+						}
 					}
-				}
-				if (ZEND_TYPE_IS_SET(property_info->type) && !zend_verify_property_type(property_info, prop, /* strict */ false)) {
-					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);

[ndossche]

You could make an argument that using strict properties is fine here, because normal serialization won't violate typing rules. However, Hyrum's law applies, so maybe not the greatest idea.

[TimWolla]

You could make an argument that using strict properties is fine here, because normal serialization won't violate typing rules.

My reasoning is that __unserialize() for internal classes is an internal function and thus doesn't observe strict_properties.

[ndossche]

Right, which makes sense, although object_properties_load may be used in other places too.
The toString issue I described above can be fixed in one go if Ilija and Arnaud work on the general memory safety issues.

[Girgias]

But the default deserialisation mechanism does seem to reject invalid types for typed properties: https://3v4l.org/B71tR#v even in weak mode.

Thus I think we should just be strict here.

[TimWolla]

But the default deserialisation mechanism does seem to reject invalid types for typed properties: https://3v4l.org/B71tR#v even in weak mode.

Indeed:

php-src/ext/standard/var_unserializer.re

Line 701 in e79d7e5

if (!zend_verify_prop_assignable_by_ref(info, data, /* strict */ 1)) {

Semantically a difference between unserialize() and __unserialize() could be argued, since that is an additional layer of indirection, but even unserialize() would technically already be running as strict_types=0. I'll adjust.

[Girgias]

This seems correct to me, but would be good if @ndossche can double check as the cursed bug hunting witch she is!

Aside: it may make sense to see if part of the implementation can be combined for use in object_properties_init_ex() as it's not quite duplication but close enough.