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

[kocsismate]

No description provided.

[kocsismate]

I'm not sure why ext/standard/tests/serialize/unserialize_ref_to_overwritten_declared_prop.phpt fails 🤔

[TysonAndre]

I'm not sure why ext/standard/tests/serialize/unserialize_ref_to_overwritten_declared_prop.phpt fails thinking

// ext/standard/tests/serialize/unserialize_ref_to_overwritten_declared_prop.phpt
$str = <<<STR
O:5:"Error":2:{S:8:"previous";N;S:8:"previous";R:2;}
STR;
var_dump(unserialize($str));

https://www.php.net/Error says previous is private, but the test is creating a public property of the same name. This is probably a mistake by the author of the tests. Private properties would be "\x00Error\x00previous" (using var_representation PECL to dump null bytes)

php > echo var_representation(serialize(new Error('', 0, new RuntimeException())));
"O:5:\"Error\":7:{s:10:\"\x00*\x00message\";s:0:\"\";s:13:\"\x00Error\x00string\";s:0:\"\";s:7:\"\x00*\x00code\";i:0;s:7:\"\x00*\x00file\";s:14:\"php shell code\";s:7:\"\x00*\x00line\";i:1;s:12:\"\x00Error\x00trace\";a:0:{}s:15:\"\x00Error\x00previous\";O:16:\"RuntimeException\":7:{s:10:\"\x00*\x00message\";s:0:\"\";s:17:\"\x00Exception\x00string\";s:0:\"\";s:7:\"\x00*\x00code\";i:0;s:7:\"\x00*\x00file\";s:14:\"php shell code\";s:7:\"\x00*\x00line\";i:1;s:16:\"\x00Exception\x00trace\";a:0:{}s:19:\"\x00Exception\x00previous\";N;}}"

class Error implements Throwable
{
    /**
     * Intentionally left untyped for BC reasons
     * @var string
     */
    protected $message = "";
    private string $string = "";
    /**
     * Intentionally left untyped for BC reasons
     * @var int
     */
    protected $code = 0; // TODO add proper type (i.e. int|string)
    protected string $file = "";
    protected int $line;
    private array $trace = [];
    private ?Throwable $previous = null;

[TysonAndre]

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
GMP serialization and unserialization [ext/gmp/tests/serialize.phpt]
session object deserialization [ext/session/tests/003.phpt]
session_set_save_handler test [ext/session/tests/004.phpt]
custom save handler, multiple session_start()s, complex data structure test. [ext/session/tests/005.phpt]
session object deserialization [ext/session/tests/023.phpt]
session_set_save_handler test [ext/session/tests/024.phpt]
custom save handler, multiple session_start()s, complex data structure test. [ext/session/tests/025.phpt]
Bug #70388 (SOAP serialize_function_call() type confusion / RCE) [ext/soap/tests/bug70388.phpt]
Bug #69085 (SoapClient's __call() type confusion through unserialize()) [ext/soap/tests/bugs/bug69085.phpt]
SplFixedArray serialisation [ext/spl/tests/SplFixedArray_serialize.phpt]
SPL: Bug #70155 Use After Free Vulnerability in unserialize() with SPLArrayObject [ext/spl/tests/bug70155.phpt]
Bug #74669: Unserialize ArrayIterator broken [ext/spl/tests/bug74669.phpt]
Trying to create a reference to an overwritten declared property [ext/standard/tests/serialize/unserialize_ref_to_overwritten_declared_prop.phpt]
=====================================================================

You have some other tests to update, as well https://github.com/php/php-src/runs/7876075206?check_suite_focus=true and possibly others

[kocsismate]

https://www.php.net/Error says previous is private, but the test is creating a public property of the same name.

Nice, thank you for the help, I didn't notice it's private, only saw that it's declared indeed.

You have some other tests to update, as well https://github.com/php/php-src/runs/7876075206?check_suite_focus=true and possibly others

Yeah, I haven't yet have time to fix all the tests, I could only search for failures in a few prominent directories like Zend and ext/standard), so I'll continue later :)

[kocsismate]

I've just fixed a bunch of tests (hopefully), but I'm unsure about what to do with ArrayObject and SplFixedArray (e.g. ext/spl/tests/bug74669.phpt): should they allow dynamic properties or they should override the default serialization mechanism in order to allow an array-like usage? I'd probably prefer the latter, but I'd like to ensure that it's the better solution :)

[TysonAndre]

but I'm unsure about what to do with ArrayObject and SplFixedArray (e.g. ext/spl/tests/bug74669.phpt): should they allow dynamic properties or they should override the default serialization mechanism in order to allow an array-like usage? I'd probably prefer the latter, but I'd like to ensure that it's the better solution :)

EDIT: You can/should use the default serialization mechanism and the alternative serialization format from serialize() is deprecated - the issue is that they need to switch to __unserialize to avoid this deprecation.

I think that switching them to implementing php 7.4 __unserialize(array $data) instead of __wakeup would be the best option.

See e2ea0f1 and https://bugs.php.net/bug.php?id=77866 - It seems like the only reason other SPL classes weren't switched was because they didn't implement the deprecated Serializable interface

ext/spl/spl_dllist.c PHP_METHOD(SplDoublyLinkedList, __unserialize) { would be a useful reference. (https://github.com/TysonAndre/pecl-teds/blob/main/teds_stricthashmap.c - I also worked on an alternative collection of data structures supporting only php 8.0+ which only used __unserialize instead of __wakeup, which may be useful as a reference for edge case handling, etc)

// ext/spl/spl_fixedarray.c
PHP_METHOD(SplFixedArray, __wakeup)
{
	spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS);
	HashTable *intern_ht = zend_std_get_properties(Z_OBJ_P(ZEND_THIS));
	zval *data;

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

	if (intern->array.size == 0) {
		int index = 0;
		int size = zend_hash_num_elements(intern_ht);

		spl_fixedarray_init(&intern->array, size);

		ZEND_HASH_FOREACH_VAL(intern_ht, data) {
			ZVAL_COPY(&intern->array.elements[index], data);
			index++;
		} ZEND_HASH_FOREACH_END();

		/* Remove the unserialised properties, since we now have the elements
		 * within the spl_fixedarray_object structure. */
		zend_hash_clean(intern_ht);
	}
}

[kocsismate]

I'd consider calling __construct or __unserialize after an object was already unserialized or constructed/cloned a misuse of those methods, and would rather throw.

Yes, I definitely agree.

Out of scope, but worth mentioning for context - I'm fine with not doing that and continuing to only check for non-empty arrays, to avoid increasing scope beyond what's needed

And I agree here as well :) We should change these later, possibly including other classes as well.

[nicolas-grekas]

I'd consider calling __construct or __unserialize after an object was already unserialized or constructed/cloned a misuse of those methods, and would rather throw.

For the record, there are legit use cases for calling both these methods explicitly in userland. For __construct: 1. lazy initialization after calling ReflectionClass::newInstanceWithoutConstructor and 2. for resetting instances to some initial states. For __unserialize: implement userland serialization logic as done in eg symfony/var-exporter

I also support the deprecation of __wakeup()

If you mean to deprecate __wakeup at the engine level, aka for all classes, then I would be very cautious as implementing an alternative using __serialize/__unserialize is quite complex. __sleep/__wakeup are just fine for simple needs.
But I think you were talking about deprecating __wakeup on a specific internal class, isn't it?

[kocsismate]

But I think you were talking about deprecating __wakeup on a specific internal class, isn't it?

Yes :) #8422 and a few other PRs added support for the new serialization method in ext/date, so I think we could safely get rid of their __wakeup() counterpart.

[TysonAndre]

After those review comments are addressed and tests pass, I think this should be ready

========DIFF========
001+ Deprecated: Creation of dynamic property SNMP::$quick_print is deprecated in /home/runner/work/php-src/php-src/ext/snmp/tests/bug72479.php on line 4
     int(1)
========DONE========
FAIL Bug #72479: Use After Free Vulnerability in SNMP with GC and unserialize() [ext/snmp/tests/bug72479.phpt] 

========DIFF========
001+ Deprecated: Creation of dynamic property ZipArchive::$filename is deprecated in /home/runner/work/php-src/php-src/ext/standard/tests/strings/bug72434.php on line 6
     array(1) refcount(3){
       [0]=>
       object(stdClass)#%d (0) refcount(1){
--
========DONE========

The latest changes to php-src should be fetched and this should be rebased to pick up e3034db for fpm test failures

(Aside: Tomorrow, the target branch may need to be changed to PHP-8.2 when that's created)

[kocsismate]

The test should be green now! 🤞

(Aside: Tomorrow, the target branch may need to be changed to PHP-8.2 when that's created)

It would be awesome if this could be merged before branching. :) But now, I'm off to bed.

[TysonAndre]

I took one last look at the entire PR and found one thing I missed - lgtm other than comment about reverting ZEND_COLD->ZEND_API now that it's no longer needed by ext/standard/var_unserializer.re