Skip to content

Commit eafcd68

Browse files
TimWollandosscheGirgiasarnaud-lb
authored
zend_API: Verify property types in object_properties_load() (#23639)
* zend_API: Verify property types in `object_properties_load()` Fixes #9708. * zend_API: Fix various issues in `object_properties_load()` Fixes Fixes #9707. Co-authored-by: Tim Düsterhus <tim@tideways-gmbh.com> * NEWS * random: Remove now-obsolete manual `$engine` type check in `Randomizer::__unserialize()` * zend_API: Make `object_properties_load()` use `strict_types=1` This is consistent with regular unserialization, which also performs strict type checking. Co-authored-by: Gina Peter Banyard <girgias@php.net> * Block creation of readonly reference properties Co-authored-by: Arnaud Le Blanc <365207+arnaud-lb@users.noreply.github.com> --------- Co-authored-by: Nora Dossche <7771979+ndossche@users.noreply.github.com> Co-authored-by: Gina Peter Banyard <girgias@php.net> Co-authored-by: Arnaud Le Blanc <365207+arnaud-lb@users.noreply.github.com>
1 parent c31ae2d commit eafcd68

8 files changed

Lines changed: 161 additions & 15 deletions

File tree

‎NEWS‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ PHP NEWS
55
- Core:
66
. Fixed incorrect internal pointer and foreach iterator positions when
77
compacting arrays with holes. (Weilin Du)
8+
. Fix handling of references to typed properties during unserialization
9+
of various internal classes. (ndossche, timwolla)
810

911
- DOM:
1012
. Fixed use-after-free when re-constructing a DOMXPath whose php:function

‎UPGRADING.INTERNALS‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,8 @@ PHP 8.6 INTERNALS UPGRADE NOTES
182182
instead of a zval*. Accordingly, zend_get_closure_this_ptr() now returns
183183
that zend_object*, or NULL when the closure is unbound, instead of a
184184
zval* that is IS_UNDEF when the closure is unbound.
185+
. object_properties_load() now verifies that the given value is assignable
186+
to typed properties. The check is performed in strict mode.
185187

186188
- Added:
187189
. New zend_class_entry.ce_flags2 and zend_function.fn_flags2 fields were

‎Zend/zend_API.c‎

Lines changed: 46 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1758,7 +1758,7 @@ ZEND_API void object_properties_load(zend_object *object, const HashTable *prope
17581758
zval *prop, tmp;
17591759
zend_string *key;
17601760
zend_long h;
1761-
const zend_property_info *property_info;
1761+
zend_property_info *property_info;
17621762

17631763
ZEND_HASH_FOREACH_KEY_VAL(properties, h, key, prop) {
17641764
if (key) {
@@ -1785,18 +1785,56 @@ ZEND_API void object_properties_load(zend_object *object, const HashTable *prope
17851785
if (property_info != ZEND_WRONG_PROPERTY_INFO &&
17861786
property_info &&
17871787
(property_info->flags & ZEND_ACC_STATIC) == 0) {
1788+
bool is_typed = ZEND_TYPE_IS_SET(property_info->type);
1789+
1790+
/* Mimick unserialize behaviour for virtual properties. */
1791+
if (UNEXPECTED(property_info->flags & ZEND_ACC_VIRTUAL)) {
1792+
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));
1793+
return;
1794+
}
1795+
17881796
zval *slot = OBJ_PROP(object, property_info->offset);
1789-
if (UNEXPECTED((property_info->flags & ZEND_ACC_READONLY) && !Z_ISUNDEF_P(slot))) {
1790-
if (Z_PROP_FLAG_P(slot) & IS_PROP_REINITABLE) {
1791-
Z_PROP_FLAG_P(slot) &= ~IS_PROP_REINITABLE;
1797+
zval val;
1798+
1799+
if (is_typed) {
1800+
if (UNEXPECTED(Z_ISREF_P(prop))) {
1801+
/* Block taking a reference to a readonly property. */
1802+
if (UNEXPECTED(property_info->flags & ZEND_ACC_READONLY)) {
1803+
zend_readonly_property_indirect_modification_error(property_info);
1804+
return;
1805+
}
1806+
if (UNEXPECTED(!zend_verify_prop_assignable_by_ref(property_info, prop, /* strict */ true))) {
1807+
ZEND_ASSERT(EG(exception));
1808+
return;
1809+
}
1810+
ZVAL_COPY(&val, prop);
1811+
ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(&val), property_info);
17921812
} else {
1793-
zend_readonly_property_modification_error(property_info);
1794-
return;
1813+
/* Mimick zend_assign_to_typed_prop() by reporting the error before doing work. */
1814+
if (UNEXPECTED((property_info->flags & ZEND_ACC_READONLY)
1815+
&& !Z_ISUNDEF_P(slot)
1816+
&& !(Z_PROP_FLAG_P(slot) & IS_PROP_REINITABLE))) {
1817+
zend_readonly_property_modification_error(property_info);
1818+
return;
1819+
}
1820+
1821+
ZVAL_COPY(&val, prop);
1822+
if (UNEXPECTED(!zend_verify_property_type(property_info, &val, /* strict */ true))) {
1823+
zval_ptr_dtor(&val);
1824+
return;
1825+
}
17951826
}
1827+
if (UNEXPECTED(Z_ISREF_P(slot))
1828+
&& (ZEND_DEBUG || ZEND_REF_HAS_TYPE_SOURCES(Z_REF_P(slot)))) {
1829+
ZEND_REF_DEL_TYPE_SOURCE(Z_REF_P(slot), property_info);
1830+
}
1831+
} else {
1832+
ZVAL_COPY(&val, prop);
17961833
}
1834+
1835+
Z_PROP_FLAG_P(slot) &= ~IS_PROP_REINITABLE;
17971836
zval_ptr_dtor(slot);
1798-
ZVAL_COPY_VALUE(slot, prop);
1799-
zval_add_ref(slot);
1837+
ZVAL_COPY_VALUE(slot, &val);
18001838
if (object->properties) {
18011839
ZVAL_INDIRECT(&tmp, slot);
18021840
zend_hash_update(object->properties, key, &tmp);

‎ext/date/tests/time/duration/gh23639.phpt‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
--TEST--
2+
GH-23639 (object_properties_load allows creating readonly reference properties)
3+
--CREDITS--
4+
arnaud-lb
5+
ndossche
6+
--XFAIL--
7+
Test can only succeed when GH-23629 is also merged
8+
--FILE--
9+
<?php
10+
11+
class Time_Duration {
12+
public int $seconds;
13+
public int $nanoseconds;
14+
public bool $negative;
15+
}
16+
17+
$d = new Time_Duration();
18+
$d->seconds = 1;
19+
$a = [$d, &$d->seconds];
20+
21+
$payload = serialize($a);
22+
23+
try {
24+
unserialize(str_replace('Time_Duration', 'Time\\Duration', $payload));
25+
} catch (Throwable $e) {
26+
do {
27+
echo $e::class, ": ", $e->getMessage(), "\n";
28+
} while ($e = $e->getPrevious());
29+
}
30+
31+
?>
32+
--EXPECT--
33+
Exception: Invalid serialization data for Time\Duration object
34+
Error: Cannot indirectly modify readonly property Time\Duration::$seconds

‎ext/random/randomizer.c‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -508,7 +508,6 @@ PHP_METHOD(Random_Randomizer, __unserialize)
508508
php_random_randomizer *randomizer = Z_RANDOM_RANDOMIZER_P(ZEND_THIS);
509509
HashTable *d;
510510
zval *members_zv;
511-
zval *zengine;
512511

513512
ZEND_PARSE_PARAMETERS_START(1, 1)
514513
Z_PARAM_ARRAY_HT(d);
@@ -531,12 +530,7 @@ PHP_METHOD(Random_Randomizer, __unserialize)
531530
RETURN_THROWS();
532531
}
533532

534-
zengine = zend_read_property(randomizer->std.ce, &randomizer->std, "engine", strlen("engine"), 1, NULL);
535-
if (Z_TYPE_P(zengine) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(zengine), random_ce_Random_Engine)) {
536-
zend_throw_exception(NULL, "Invalid serialization data for Random\\Randomizer object", 0);
537-
RETURN_THROWS();
538-
}
539-
533+
zval *zengine = zend_read_property(randomizer->std.ce, &randomizer->std, "engine", strlen("engine"), /* silent */ true, NULL);
540534
randomizer_common_init(randomizer, Z_OBJ_P(zengine));
541535
}
542536
/* }}} */

‎ext/random/tests/03_randomizer/gh_9708_unserialize.phpt‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
--TEST--
2+
GH-9708: object_properties_load() bypasses typed property checks
3+
--FILE--
4+
<?php
5+
6+
try {
7+
unserialize('O:17:"Random\Randomizer":1:{i:0;a:1:{s:6:"engine";N;}}');
8+
} catch (Throwable $e) {
9+
echo $e::class, ': ', $e->getMessage(), "\n";
10+
}
11+
12+
?>
13+
--EXPECT--
14+
Exception: Invalid serialization data for Random\Randomizer object

‎ext/spl/tests/ArrayObject/gh_9707_unserialize.phpt‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
--TEST--
2+
GH-9707: object_properties_load crashes in debug mode when unserializing references to typed properties in php 8.1+
3+
--FILE--
4+
<?php
5+
6+
class Foo extends ArrayObject {
7+
public int $a = 0;
8+
public int $b = 0;
9+
}
10+
11+
$f = new Foo();
12+
$r = &$f->a;
13+
$f->b = &$r;
14+
15+
$f->b = 1;
16+
var_dump($unserialized = unserialize(serialize($f)));
17+
18+
$unserialized->b = 2;
19+
20+
var_dump($unserialized);
21+
22+
?>
23+
--EXPECTF--
24+
object(Foo)#%d (3) {
25+
["a"]=>
26+
&int(1)
27+
["b"]=>
28+
&int(1)
29+
["storage":"ArrayObject":private]=>
30+
array(0) {
31+
}
32+
}
33+
object(Foo)#%d (3) {
34+
["a"]=>
35+
&int(2)
36+
["b"]=>
37+
&int(2)
38+
["storage":"ArrayObject":private]=>
39+
array(0) {
40+
}
41+
}

‎ext/spl/tests/ArrayObject/gh_9708_unserialize.phpt‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
--TEST--
2+
GH-9708: object_properties_load() bypasses typed property checks
3+
--FILE--
4+
<?php
5+
6+
class Foo extends ArrayObject {
7+
public int $a = 5;
8+
public string $b = "10";
9+
}
10+
11+
try {
12+
// a = "10", b = 5
13+
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;}');
14+
} catch (Throwable $e) {
15+
echo $e::class, ': ', $e->getMessage(), "\n";
16+
}
17+
18+
19+
?>
20+
--EXPECT--
21+
TypeError: Cannot assign string to property Foo::$a of type int

0 commit comments

Comments
 (0)