Hi,
On Tue, Sep 9, 2025 at 3:39 PM Tim Düsterhus <t...@bastelstu.be> wrote:
> 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.
>
>
I can see what you mean but you could say in the same way that many parts
in the engine and other parts are security sensitive. Almost anything that
is complex can lead to a security issue given some preconditions. The fact
is that we have not had any serialization issue for a long time so to me
there are places that are more security sensitive and more problematic than
seriallization.
Kind regards,
Jakub
