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
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.
Hi!
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.
I don't remember proposing exactly that, though the idea looks worthy to
me :) I did something different though - allowing limiting unserialize
to accept only certain set of classes, which alleviates some security
issues, but not all of them. This probably would catch more cases,
though also not all of them.
As for validation pass, the only issue I foresee is handling custom
serializers. We'd need to either add validation handler too (probably a
good idea) or somehow handle that. Needs some investigation there.
As for this specific bug, it looks like an oversight. I'm not sure why
we encode visibility in the property at all... Probably for efficiency,
but I think we need to be smarter there.
--
Stas Malyshev
smalyshev@gmail.com