On Mon, Nov 26, 2018 at 11:47 PM Stanislav Malyshev <smalys...@gmail.com> wrote:
> Hi! > > > We should migrate such cases to serialize_deny though. I think it's > pretty > > weird to explicitly implement __wakeup (signalling that yes, you can be > > unserialized), and then use it to throw (sorry, I lied). > > Throwing in __wakeup does not signal that it can be serialized. What it > says that if you try to unserialize it (note that the code doing > unserialize is not the same that does serialize and has no control over > what the argument string says - it may demand to unserialize anything) > it won't work. That _implies_ you shouldn't also serialize it (because > what's the point) but the important part is not to produce broken > objects from unserialization loop. > > Also, for CURLFile for example there are additional things that happen > on __wakeup besides throwing, probably for security reasons. I am not > sure whether they are necessary anymore as we pretty much tell people > "don't unserialize external data" but they are there for now. Just > moving to _deny handler would probably not keep them. > Historically, __wakeup() has been the correct way to prevent unserialization and/or mitigate issues relating to dangerous unserialized state. The reason is that it was possible to bypass the unserialize_deny handler by using the O-style, rather than the C-style serialization format. At some point this whole was plugged and we don't allow O-unserializing classes that have serialize/unserialize handlers. Which is why nowadays, (un)serialize_deny is our strongest defense against unserialization vulnerabilities, because it prevents unserialization *before* the object is constructed. In the __wakeup case the object is created first and __wakeup is only called much later at the end of unserialization, which creates a lot more opportunities for the usual shenanigans. Basically, anything using a throwing __wakeup() nowadays is a leftover from times where unserialize_deny was not properly enforced. To the best of my knowledge, there is no good reason to use the throwing __wakeup pattern nowadays anymore. Regards, Nikita
