Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions UPGRADING.INTERNALS
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
50 changes: 42 additions & 8 deletions Zend/zend_API.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -1785,18 +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 */ 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;
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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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);
Expand Down
8 changes: 1 addition & 7 deletions ext/random/randomizer.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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));
}
/* }}} */
14 changes: 14 additions & 0 deletions ext/random/tests/03_randomizer/gh_9708_unserialize.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
--TEST--
GH-9708: object_properties_load() bypasses typed property checks
--FILE--
<?php

try {
unserialize('O:17:"Random\Randomizer":1:{i:0;a:1:{s:6:"engine";N;}}');
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), "\n";
}

?>
--EXPECT--
Exception: Invalid serialization data for Random\Randomizer object
41 changes: 41 additions & 0 deletions ext/spl/tests/ArrayObject/gh_9707_unserialize.phpt
Original file line number Diff line number Diff line change
@@ -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--
<?php

class Foo extends ArrayObject {
public int $a = 0;
public int $b = 0;
}

$f = new Foo();
$r = &$f->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) {
}
}
21 changes: 21 additions & 0 deletions ext/spl/tests/ArrayObject/gh_9708_unserialize.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
--TEST--
GH-9708: object_properties_load() bypasses typed property checks
--FILE--
<?php

class Foo extends ArrayObject {
public int $a = 5;
public string $b = "10";
}

try {
// a = "10", b = 5
unserialize('O:3:"Foo":4:{i:0;i:0;i:1;a:0:{}i:2;a:2:{s:1:"a";s:2:"10";s:1:"b";i:5;}i:3;N;}');
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), "\n";
}


?>
--EXPECT--
TypeError: Cannot assign string to property Foo::$a of type int
Loading