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: p...@golemon.com <p...@golemon.com> on behalf of Sara Golemon <poll...@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
