Looking into the number of unserialize() related "security" issues, I think we should fix all of them once and forever, introducing a validation pass.
In case something in provided data is wrong (e.g. duplicated properties or array keys, unexpected types, invalid references, invalid property visibility, etc), we should just return FALSE.
I think, Stas proposed something similar some time ago.
Thanks. Dmitry.
________________________________
From: php@golemon.com <php@golemon.com> on behalf of Sara Golemon <pollita@php.net>
Sent: Thursday, June 23, 2016 8:53:58 PM
To: PHP internals
Subject: [PHP-DEV] [Bug #68319] unserialize() with modified class definition.
https://bugs.php.net/bug.php?id=68319
https://3v4l.org/irnRC
The crux is this:
* Object instance gets serialized with one definition, maybe stored in
DB/file, whatever, the serialized value lives on.
* Class definition changes slightly. In this case, a property changes
visibility.
* Serialized value is unserialized. The prop visibilities don't match.
* PHP says, "Eh, whatevs, I'll make a dynamic prop of the same name."
Possible resolutions:
1: Raise a warning and return false (as unserialize already does for
parse errors)
2: Raise a warning and "correct" the visibility to match the current
class definition
3: Raise a warning and continue duplicating the properties
I don't think we need to be as terrible as option 3 since any code
facing this problem right now can't actually access the unserialized
value and is therefore broken in much worse ways. I think option 2
presents its own unquantified risks and should probably be avoided.
So obviously, I vote option 1, but I'd like to get other's thoughts
and opinions before addressing this bug.
I'm going to go ahead and say ignore what HHVM does here. In this
specific case they basically take option 2, but in the inverse case
https://3v4l.org/ecM1Q they're precisely as broken as we are.
-Sara
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
