Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:128663
Precedence: list
MIME-Version: 1.0
Date: Tue, 09 Sep 2025 15:39:51 +0200
To: Jakub Zelenka <bukka@php.net>
Cc: Nicolas Grekas <nicolas.grekas+php@gmail.com>, PHP Internals List
 <internals@lists.php.net>
Subject: Re: [PHP-DEV] [RFC] Soft-Deprecate __sleep() and __wakeup()
In-Reply-To: <CAEKnhAHxBZORfkByM-EuBpc1CYiY=3FkwsgBsSubxfChKpXAww@mail.gmail.com>
References: <CAOWwgpk1Z_bns=Z55TqpYOBXvBGpurMfjTGPSgQ6Uf5QXfLOjg@mail.gmail.com>
 <cddcbba0366184b616e34637b424645e@bastelstu.be>
 <CAEKnhAFFgpBSOU+yTAfvY6_ndZwOmKvOKZG=F2+mamqkJPfrrQ@mail.gmail.com>
 <b7d35ecd-d68a-45e8-be88-792a15b3d79b@bastelstu.be>
 <CAEKnhAHxBZORfkByM-EuBpc1CYiY=3FkwsgBsSubxfChKpXAww@mail.gmail.com>
Message-ID: <7b1b5b71cd32bad2646164c624dc6c7f@bastelstu.be>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

Hi

Am 2025-09-08 23:14, schrieb Jakub Zelenka:
> I understand your concern about the complexity but this can apply to 
> many
> other parts in php-src. As I'm sure you know, this still wouldn't 
> likely to
> be considered as a security issue.

I've intentionally said “security sensitive”. I believe the risk of 
security issues for the unserializer is significantly higher than in 
other parts of the language and I also believe that by keeping 
complexity down it is both easier to verify correctness and also to fix 
any bugs that crop up. From what I see __wakeup() and __unserialize() 
work quite differently internally. When there's __unserialize(), the 
deserialization process only creates an empty object shell (similarly to 
newInstanceWithoutConstructor) and then calls __unserialize() at the 
very end. For __wakeup() all the properties are filled directly and then 
__wakeup() is called at the end. In both cases the destructor is skipped 
for all following objects once one of the deserialization hooks fails.

For __wakeup() this has the interesting effect that in case of circular 
structures, some objects may appear to already be properly initialized 
(all the properties are there), but __wakeup() has not executed yet, 
potentially making them unsafe to touch. In case of __unserialize() the 
objects are clearly in a partially initialized state (e.g. properties 
being uninit), which I'd claim is safer:

     <?php

     class A {
         public $a;

         public function __construct(public string $name)
         {

         }

         public function __unserialize(array $data): void
         {
             $this->a = $data['a'];
             $this->name = $data['name'];
             echo "Waking up ", $this->name, PHP_EOL;
             var_dump($this->a->name);
         }

         public function __wakeup(): void
         {
             echo "Waking up ", $this->name, PHP_EOL;
             var_dump($this->a->name);
         }

         public function __destruct()
         {
             echo __METHOD__, PHP_EOL;
         }
     }

     $a = new A('A');
     $a->a = new A('B');
     $a->a->a = new A('C');
     $a->a->a->a = $a;

     echo "Before", PHP_EOL;
     var_dump(unserialize(serialize($a)));
     echo "After", PHP_EOL;

In case of `__unserialize()` the unsafe access to `$this->a->name` will 
throw, whereas in `__wakeup()` it will return `A`, despite `A` not being 
fully available yet. Similarly skipping the destructor for an empty 
object shell is safer than skipping the destructor for an object that 
may appear usable.

>> Saying that “unserialize is not security-relevant because you must 
>> only
>> feed it safe inputs” as an excuse to avoid making unserialize safer is
>> not helping anyone and is downplaying the risks involved in
>> unserialization.
>> 
>> 
> I don't have problem with making unserialize safer as there might be 
> users
> that use it improperly.  But we shouldn't be saying that this is a 
> security
> sensitive part when we don't consider those sort of issues as security
> issues because none of those issues will get fixed in our security 
> support
> only releases. In that sense I don't think we can consider it as a 
> security
> sensitive part.

The unserializer already contains quite a bit of logic to make it as 
safe as possible, e.g. running the unserialization hooks only at the 
very end and skipping destructors for objects where the unserialization 
hook didn't successfully run. The forefathers definitely considered 
possible edge cases that might lead to security issues.

Best regards
Tim Düsterhus