Repository navigation
@strict-properties can be bypassed using unserialization #9186
Description
Description
The following code:
<?php
var_dump(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";}}}}'));Resulted in this output:
object(Random\Randomizer)#1 (2) {
["engine"]=>
object(Random\Engine\Xoshiro256StarStar)#2 (1) {
["__states"]=>
array(4) {
[0]=>
string(16) "7520fbc2d6f8de46"
[1]=>
string(16) "84d2d2b9d7ba0a34"
[2]=>
string(16) "d975f36db6490b32"
[3]=>
string(16) "c19991ee16785b94"
}
}
["foo"]=>
NULL
}
But I expected an error instead, because the ->foo property should not exist in the Randomizer as per:
php-src/ext/random/random.stub.php
Lines 125 to 127 in 7ab22aa
Not sure if this is a bug in the implementation of Randomizer or a more general unserialization bug.
PHP Version
Current git master
Operating System
No response
Activity
[-]`@strict-properties` can be bypassed using serialization[/-][+]`@strict-properties` can be bypassed using unserialization[/+]cmb69 commentedon Jul 29, 2022
The problem is
php-src/ext/random/randomizer.c
Line 280 in 9e2de4c
That initializes any given property. We likely want to catch cases where undeclared properties are given.
TimWolla commentedon Jul 29, 2022
I assumed as much. But I was not able to determine, whether ext/random should've used a different function (making this a bug in ext/random) or whether the function itself should be fixed (not making this a bug in ext/random).
cmb69 commentedon Jul 29, 2022
Good question! It seems to me that the dynamic properties deprecation missed unserialization and related things. E.g. https://3v4l.org/FUgco/rfc#output behaves unexpected for me.
I'm not sure whether we can "fix" this now (in the general case); might be a bit too late in the pre-release cycle.
@ramsey, @saundefined, @adoy, thoughts?
iluuu1994 commentedon Aug 11, 2022
Was a decision made here whether this should be fixed in PHP 8.2?
ramsey commentedon Aug 12, 2022
We've not discussed it. I need to read up on it and understand it a bit more, and will see if we can make a decision soon.
saundefined commentedon Aug 16, 2022
@cmb69 we're probably a little late with this change, because RC1 is coming out soon,
but we can discuss whether to include it in PHP 8.2 (and maybe we can introduce Beta 4, so there won't be a big change in the RC cycle).
Let's discuss it in internals.
kocsismate commentedon Aug 17, 2022
As far as I see, this is a bug which should be fixed ASAP. For any case, I created a fix in #9354. It also deals with Tyson's suggestion in #9325 about triggering dynamic property deprecations. (The fix may be yet be exhaustive as I'm not very familiar with the unserialization code, but it is hopefully a good first step into the right direction)
kocsismate commentedon Aug 30, 2022
There was not much discussion..., and I still firmly believe we should ship this fix as soon as possible, so I'll merge my fix in ~2 hours, before branching takes place, unless there is a strong resistance.