Skip to content

Fix GH-9186 @strict-properties can be bypassed using unserialization #9354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Aug 30, 2022
Merged

Fix GH-9186 @strict-properties can be bypassed using unserialization #9354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6d66a7a
Fix GH-9186 @strict-properties can be bypassed using unserialization
kocsismate Aug 17, 2022
0869882
Fix tests and review suggestions
kocsismate Aug 18, 2022
ebafb25
Fix another round of review comments
kocsismate Aug 22, 2022
a8515c7
Add new serialization methods for SplFixedArray
kocsismate Aug 25, 2022
b1f1abc
Fix tests
kocsismate Aug 26, 2022
8c8ff84
Another round of review fixes
kocsismate Aug 29, 2022
f1a6837
Fixes
kocsismate Aug 29, 2022
817adf5
Fix order of dynamic/readonly property checks
kocsismate Aug 29, 2022
d730db9
Optimization + warning fix
kocsismate Aug 29, 2022
90bef0f
Minor review fixes
kocsismate Aug 30, 2022
25863e1
Add news entry
kocsismate Aug 30, 2022
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ PHP NEWS
(cmb)
. Fixed bug GH-9285 (Traits cannot be used in readonly classes).
(kocsismate)
. Fixed bug GH-9186 (@strict-properties can be bypassed using
unserialization). (kocsismate)

- Date:
. Fixed bug GH-9431 (DateTime::getLastErrors() not returning false when no
Expand Down
3 changes: 2 additions & 1 deletion Zend/tests/gc_043.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ STR;
var_dump(unserialize($s));
gc_collect_cycles();
?>
--EXPECT--
--EXPECTF--
Deprecated: Creation of dynamic property RegexIterator::$5 is deprecated in %s on line %d
object(stdClass)#1 (2) {
["5"]=>
object(SplStack)#2 (2) {
Expand Down
16 changes: 16 additions & 0 deletions Zend/tests/readonly_classes/readonly_class_unserialize_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
--TEST--
Fix GH-9186 Readonly classes can have dynamic properties created by unserialize()
--FILE--
<?php

readonly class C {}

try {
$readonly = unserialize('O:1:"C":1:{s:1:"x";b:1;}');
} catch (Error $exception) {
echo $exception->getMessage() . "\n";
}

?>
--EXPECT--
Cannot create dynamic property C::$x
17 changes: 17 additions & 0 deletions Zend/zend_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1628,13 +1628,30 @@ ZEND_API void object_properties_load(zend_object *object, HashTable *properties)
zend_hash_update(object->properties, key, &tmp);
}
} else {
if (UNEXPECTED(object->ce->ce_flags & ZEND_ACC_NO_DYNAMIC_PROPERTIES)) {
zend_throw_error(NULL, "Cannot create dynamic property %s::$%s",
ZSTR_VAL(object->ce->name), property_info != ZEND_WRONG_PROPERTY_INFO ? zend_get_unmangled_property_name(key): "");
return;
} else if (!(object->ce->ce_flags & ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES)) {
zend_error(E_DEPRECATED, "Creation of dynamic property %s::$%s is deprecated",
ZSTR_VAL(object->ce->name), property_info != ZEND_WRONG_PROPERTY_INFO ? zend_get_unmangled_property_name(key): "");
}

if (!object->properties) {
rebuild_object_properties(object);
}
prop = zend_hash_update(object->properties, key, prop);
zval_add_ref(prop);
}
} else {
if (UNEXPECTED(object->ce->ce_flags & ZEND_ACC_NO_DYNAMIC_PROPERTIES)) {
zend_throw_error(NULL, "Cannot create dynamic property %s::$" ZEND_LONG_FMT, ZSTR_VAL(object->ce->name), h);
return;
} else if (!(object->ce->ce_flags & ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES)) {
zend_error(E_DEPRECATED, "Creation of dynamic property %s::$" ZEND_LONG_FMT " is deprecated",
ZSTR_VAL(object->ce->name), h);
}

if (!object->properties) {
rebuild_object_properties(object);
}
Expand Down
24 changes: 24 additions & 0 deletions ext/gmp/tests/gmp_dynamic_property.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
GH-9186 Dynamic property unserialization should trigger a deprecated notice
--EXTENSIONS--
gmp
--FILE--
<?php

$g = new GMP();
$g->{1} = 123;

$serialized = serialize($g);
var_dump(unserialize($serialized));

?>
--EXPECTF--
Deprecated: Creation of dynamic property GMP::$1 is deprecated in %s on line %d

Deprecated: Creation of dynamic property GMP::$1 is deprecated in %s on line %d
object(GMP)#%d (%d) {
[1]=>
int(123)
["num"]=>
string(1) "0"
}
2 changes: 2 additions & 0 deletions ext/gmp/tests/serialize.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ object(GMP)#%d (1) {

Deprecated: Creation of dynamic property GMP::$foo is deprecated in %s on line %d
string(56) "O:3:"GMP":2:{i:0;s:1:"d";i:1;a:1:{s:3:"foo";s:3:"bar";}}"

Deprecated: Creation of dynamic property GMP::$foo is deprecated in %s on line %d
object(GMP)#%d (2) {
["foo"]=>
string(3) "bar"
Expand Down
4 changes: 4 additions & 0 deletions ext/random/engine_mt19937.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,10 @@ PHP_METHOD(Random_Engine_Mt19937, __unserialize)
RETURN_THROWS();
}
object_properties_load(&engine->std, Z_ARRVAL_P(t));
if (EG(exception)) {
zend_throw_exception_ex(NULL, 0, "Invalid serialization data for %s object", ZSTR_VAL(engine->std.ce->name));
RETURN_THROWS();
}

/* state */
t = zend_hash_index_find(d, 1);
Expand Down
8 changes: 6 additions & 2 deletions ext/random/randomizer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ PHP_METHOD(Random_Randomizer, __construct)

/* {{{ Generate positive random number */
PHP_METHOD(Random_Randomizer, nextInt)
{
{
php_random_randomizer *randomizer = Z_RANDOM_RANDOMIZER_P(ZEND_THIS);
uint64_t result;

Expand All @@ -104,7 +104,7 @@ PHP_METHOD(Random_Randomizer, nextInt)
zend_throw_exception(random_ce_Random_RandomException, "Generated value exceeds size of int", 0);
RETURN_THROWS();
}

RETURN_LONG((zend_long) (result >> 1));
}
/* }}} */
Expand Down Expand Up @@ -278,6 +278,10 @@ PHP_METHOD(Random_Randomizer, __unserialize)
RETURN_THROWS();
}
object_properties_load(&randomizer->std, Z_ARRVAL_P(members_zv));
if (EG(exception)) {
zend_throw_exception(NULL, "Invalid serialization data for Random\\Randomizer object", 0);
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)) {
Expand Down
14 changes: 14 additions & 0 deletions ext/random/tests/03_randomizer/gh_9186_unserialize.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
--TEST--
Fix GH-9186 @strict-properties can be bypassed using unserialization
--FILE--
<?php

try {
unserialize('O:17:"Random\Randomizer":1:{i:0;a:2:{s:3:"foo";N;s:6:"engine";O:32:"Random\Engine\Xoshiro256StarStar":2:{i:0;a:0:{}i:1;a:4:{i:0;s:16:"7520fbc2d6f8de46";i:1;s:16:"84d2d2b9d7ba0a34";i:2;s:16:"d975f36db6490b32";i:3;s:16:"c19991ee16785b94";}}}}');
} catch (Exception $error) {
echo $error->getMessage() . "\n";
}

?>
--EXPECT--
Invalid serialization data for Random\Randomizer object
1 change: 1 addition & 0 deletions ext/session/tests/003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ error_reporting(E_ALL);

class foo {
public $bar = "ok";
public $yes;
function method() { $this->yes++; }
}

Expand Down
1 change: 1 addition & 0 deletions ext/session/tests/004.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ $hnd = new handler;

class foo {
public $bar = "ok";
public $yes;
function method() { $this->yes++; }
}

Expand Down
1 change: 1 addition & 0 deletions ext/session/tests/005.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ $hnd = new handler;

class foo {
public $bar = "ok";
public $yes;
function method() { $this->yes++; }
}

Expand Down
1 change: 1 addition & 0 deletions ext/session/tests/023.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ error_reporting(E_ALL);

class foo {
public $bar = "ok";
public $yes;
function method() { $this->yes++; }
}

Expand Down
1 change: 1 addition & 0 deletions ext/session/tests/024.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ $hnd = new handler;

class foo {
public $bar = "ok";
public $yes;
function method() { $this->yes++; }
}

Expand Down
1 change: 1 addition & 0 deletions ext/session/tests/025.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ $hnd = new handler;

class foo {
public $bar = "ok";
public $yes;
function method() { $this->yes++; }
}

Expand Down
2 changes: 2 additions & 0 deletions ext/soap/tests/bug70388.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ Bug #70388 (SOAP serialize_function_call() type confusion / RCE)
soap
--FILE--
<?php

#[AllowDynamicProperties]
class MySoapClient extends SoapClient {
public function __doRequest($request, $location, $action, $version, $one_way = 0): string {
echo $request, "\n";
Expand Down
1 change: 1 addition & 0 deletions ext/soap/tests/bugs/bug69085.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ soap.wsdl_cache_enabled=0
--FILE--
<?php

#[AllowDynamicProperties]
class MySoapClient extends SoapClient {
public function __doRequest($request, $location, $action, $version, $one_way = 0): string {
echo $request, "\n";
Expand Down
3 changes: 3 additions & 0 deletions ext/spl/spl_array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1740,6 +1740,9 @@ PHP_METHOD(ArrayObject, __unserialize)
}

object_properties_load(&intern->std, Z_ARRVAL_P(members_zv));
if (EG(exception)) {
RETURN_THROWS();
}

if (iterator_class_zv && Z_TYPE_P(iterator_class_zv) == IS_STRING) {
zend_class_entry *ce = zend_lookup_class(Z_STR_P(iterator_class_zv));
Expand Down
85 changes: 81 additions & 4 deletions ext/spl/spl_fixedarray.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,18 @@ static void spl_fixedarray_init_elems(spl_fixedarray *array, zend_long from, zen
}
}

static void spl_fixedarray_init_non_empty_struct(spl_fixedarray *array, zend_long size)
{
array->size = 0; /* reset size in case ecalloc() fails */
array->elements = size ? safe_emalloc(size, sizeof(zval), 0) : NULL;
array->size = size;
array->should_rebuild_properties = true;
}

static void spl_fixedarray_init(spl_fixedarray *array, zend_long size)
{
if (size > 0) {
array->size = 0; /* reset size in case ecalloc() fails */
array->elements = safe_emalloc(size, sizeof(zval), 0);
array->size = size;
array->should_rebuild_properties = true;
spl_fixedarray_init_non_empty_struct(array, size);
spl_fixedarray_init_elems(array, 0, size);
} else {
spl_fixedarray_default_ctor(array);
Expand Down Expand Up @@ -582,6 +587,78 @@ PHP_METHOD(SplFixedArray, __wakeup)
}
}

PHP_METHOD(SplFixedArray, __serialize)
{
spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS);
zval *current;
zend_string *key;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

uint32_t property_num = zend_hash_num_elements(intern->std.properties);
array_init_size(return_value, intern->array.size + property_num);

/* elements */
for (zend_long i = 0; i < intern->array.size; i++) {
current = &intern->array.elements[i];
zend_hash_next_index_insert(Z_ARRVAL_P(return_value), current);
Z_TRY_ADDREF_P(current);
}

/* members */
ZEND_HASH_FOREACH_STR_KEY_VAL(intern->std.properties, key, current) {
zend_hash_add(Z_ARRVAL_P(return_value), key, current);
Z_TRY_ADDREF_P(current);
} ZEND_HASH_FOREACH_END();
}

PHP_METHOD(SplFixedArray, __unserialize)
{
spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS);
HashTable *data;
zval members_zv, *elem;
zend_string *key;
zend_long size;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "h", &data) == FAILURE) {
RETURN_THROWS();
}

if (intern->array.size == 0) {
size = zend_hash_num_elements(data);
spl_fixedarray_init_non_empty_struct(&intern->array, size);
if (!size) {
return;
}
array_init(&members_zv);

intern->array.size = 0;
ZEND_HASH_FOREACH_STR_KEY_VAL(data, key, elem) {
if (key == NULL) {
ZVAL_COPY(&intern->array.elements[intern->array.size], elem);
intern->array.size++;
} else {
Z_TRY_ADDREF_P(elem);
zend_hash_add(Z_ARRVAL(members_zv), key, elem);
}
} ZEND_HASH_FOREACH_END();

if (intern->array.size != size) {
if (intern->array.size) {
intern->array.elements = erealloc(intern->array.elements, sizeof(zval) * intern->array.size);
} else {
efree(intern->array.elements);
intern->array.elements = NULL;
}
}

object_properties_load(&intern->std, Z_ARRVAL(members_zv));
zval_ptr_dtor(&members_zv);
}
}

PHP_METHOD(SplFixedArray, count)
{
zval *object = ZEND_THIS;
Expand Down
4 changes: 4 additions & 0 deletions ext/spl/spl_fixedarray.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ public function __construct(int $size = 0) {}
/** @tentative-return-type */
public function __wakeup(): void {}
Copy link
Contributor

Choose a reason for hiding this comment

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

__wakeup is mostly only called for unserialization, not directly

  1. I think this should emit a deprecation notice - once php fully deprecates dynamic properties in php 9.0 there will be even less of a reason to ever call this. Adding an @deprecated line to the PHPDoc comment does that, I believe
  2. Personally, I think the implementation of __wakeup should be changed to do nothing (and emit a deprecation notice), and be removed in the next major version. User code calling $fixedArray->__wakeup directly and copying declared properties (not changing them) and moving+removing dynamic properties to the array (but only if the array size is 0) is unintuitive and not something you'd deliberately do. The deprecation notice will tell anything doing that that it will no longer work.
  3. The behavior of get_object_properties{,_for} has weirdness as a result of what __wakeup does right now - thankfully, the impact of some of that will go away with the switch to __serialize and the rest is out of scope
php > class MyClass extends SplFixedArray { public function __construct(public readonly int $abc) {} }
php > $x = new MyClass(123);
php > var_export($x);
\MyClass::__set_state(array(
   'abc' => 123,
))
php > echo serialize($x);
O:7:"MyClass":1:{s:3:"abc";i:123;}
php > $x->__wakeup();
php > echo serialize($x);
O:7:"MyClass":1:{i:0;i:123;}  // <-- no longer serializing $x->abc
php > var_export($x->abc);
123

Copy link
Member Author

Choose a reason for hiding this comment

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

I also support the deprecation of __wakeup(), but it seems a bit late, just before RC1. Of course, it is possible that doing so is still the least worst option. To be honest, I definitely wouldn't remove the whole code of __wakeup just yet, deprecating first seems a smoother upgrade path IMO.


public function __serialize(): array {}

public function __unserialize(array $data): void {}

/** @tentative-return-type */
public function count(): int {}

Expand Down
16 changes: 13 additions & 3 deletions ext/spl/spl_fixedarray_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading