Built-in classes that cannot be serialized View as Markdown

On Mon, Nov 26, 2018 at 12:27 PM Sebastian Bergmann sebastian@php.net
wrote:

Am 26.11.2018 um 12:20 schrieb Nikita Popov:

Apart from serialize_deny, a pretty common pattern is throwing __wakeup.
See for example CURLFile.

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).

In any case, what's your motivation here? As long as throwing
(un)serialize/__sleep/__wakeup exist, you will not be able to determine
whether a class can be (un)serialized a priori. It may even be that a
class
can only sometimes be serialized. The only reliable way to find out is to
actually try it. What prevents you from attempting (un)serialization and
catching potentially thrown exceptions?

If I can rely on classes to throw an exception when serialize() is
performed (be it through zend_class_serialize_deny or in __sleep())
then, yes, I can just try it.

I believe you can rely on this. Not on any specific exception type, but the
fact that it will throw.

I, too, think that classes that currently throw in __sleep() should be
migrated to use zend_class_serialize_deny.

I've switched CURLFile, PDO and PDOStatement over to use serialize_deny. I
couldn't find other classes in bundled extensions that were manually
throwing on serialization.

Only if all classes that cannot be serialized use
zend_class_serialize_deny then it would make sense to expose that
information through the Reflection API.

We could do that, but I'm not sure how useful it really is. After all, even
if a class can in principle be serialized, it might still have a property
that contains a class that cannot be serialized. Serializability is not a
property of a single class or object, it's a property of the whole object
graph that is being serialized.

Nikita

On Mon, Nov 26, 2018 at 11:47 PM Stanislav Malyshev smalyshev@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