@strict-properties can be bypassed using unserialization

[TimWolla]

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

	/**
	* @strict-properties
	*/

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

[cmb69]

The problem is

php-src/ext/random/randomizer.c

Line 280 in 9e2de4c

object_properties_load(&randomizer->std, Z_ARRVAL_P(members_zv));

That initializes any given property. We likely want to catch cases where undeclared properties are given.

[TimWolla]

The problem is

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]

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]

Was a decision made here whether this should be fixed in PHP 8.2?

[ramsey]

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]

@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]

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]

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.