Add allowed_classes_callback to unserialize()

[langemeijer]

I want to propose an added allowed_classes_callback option to unserialize()

The class name as parsed from the serialized string is passed as a parameter to the callback, it should return a boolean. true would allow the class, false would block it the same way 'allowed_classes' would, using __PHP_Incomplete_Class.

The callback will be triggered after allowed_classes is evaluated (if present).

This callback would solve a few problems where allowed_classes is not sufficient:

• It would allow for an is_subclass_of() check where for example an interface can be added to classes that are safe to get unserialized.

• It would allow for better ways to fail, I think the __PHP_Incomplete_Class situation is kinda yucky. Since this is a callable the developer could just find another solution, for example throwing an exception, do a trigger_error() or exit().

• This would also allow for fixing legacy applications where it is not exactly clear what is being unserialized. The callable would return a true value but a E_USER_DEPRECATION is triggered. This way data can be collected about what classes to allow. Providing a non-disrupting way to secure unserialize calls. This is especially helpful in very generic unserialize usages like caches.

• This would make the unserialize_callback_func ini setting redundant for many use-cases, as any custom autoloading routine could also be executed in this allowed_classes_callback.

Feedback is very much appreciated.

[Girgias]

I'm not exactly sure of all the interactions, but can't this be solved by using the unserialize_callback_func INI setting?

[langemeijer]

That ini setting works like an autoloader. It probably pre-dates autoloading, but I didn't check.

From the manual:

The callback specified is called when unserialize() attempts to use an undefined class.

[langemeijer]

I checked: Yes indeed. Young Derick merged unserialize_callback_func ending November 2001, probably hitting 4.3. There is no mention of it in any changelog, but I guess it's too late to complain. :-) This pre-dates autoloading (introduced in 5.0) by a few years. I love git-archeology.

Back then this callback was very convenient, because it was assumed to be very reasonable that before unserializing, you never knew what your serialized string might hold. And this was therefore one of the first places almost everybody needed some autoloading. In the mean time we have learnt that it's pretty dangerous that we don't know what is being unserialized, and we want to limit this as much as we can.

This article by Mathieu Farrell is an excelect writeup btw:
https://blog.quarkslab.com/php-deserialization-attacks-and-a-new-gadget-chain-in-laravel.html

Problem is that for older applications we sometimes still don't know what is being unserialized and there is no easy way to find out reliably because it is in the data, not the code. My PR is aimed to solve this problem and help close this security issue.

[langemeijer]

I think for a realistic refactor-path allowed_classes_callback is needed for larger projects. I needed it for mine. Also I think that allowed_classes_callback offers a better developer experience than the allowed_classes array.

In my opinion the old PHP4 unserialize_callback_func ini setting should be deprecated as soon as possible, because it is confusing and adds nothing that cannot be solved already with standard autoloading.

My long-term agenda is pushing to close the security gap in unserialize():

I wish in the future (hopefully 8.6/9.0?) we could throw a deprecation message when an object is unserialize()'d without specifying allowed_classes or allowed_classes_callback, when a class was disallowed, and when __PHP_Incomplete_Class would used.

After deprecation, unserialize() should never return anything objects other than explicitly allowed classes. Any disallowed or unknown classes should trigger an exception. One could always do:

'allowed_classes_callback' => fn ($className) => true,

but that is obviously dangerous.

[TimWolla]

Related: https://wiki.php.net/rfc/improve_unserialize_error_handling (see Future Scope).

[langemeijer]

@TimWolla I've read this and I'm happy that moving to exceptions has already been decided upon. (I read it before, but forgot about it) I'm sad that it will be taking > 4 years to make progress, especially since it was only 20 in favour, 12 against for changing this in 8.x. For a BC break in an error condition. After surviving strftime() and no-null-to-string-parameters-hell this seems a weird choice.

I'm suprised something like JSON_THROW_ON_ERROR in the options array was not proposed, making transitioning to exceptions a lot smoother, and possibly would have a bigger chance of getting into 8.x. But that's water under the bridge as we're getting close to 9.0 anyway, and too close to the 8.5 freeze.

In the RFC:

A unserialize_callback_func option as replacement for the ini setting with the same name: https://externals.io/message/118566#118672

I don't think unserialize_callback_func is very useful. It was originally designed as an autoloader that is only triggered when an undefined class is encountered in the serialized data. From what I see in a quick source code search on github I mostly see ini_set('unserialize_callback_func', 'spl_autoload_call'); which is redundant, as unserialize() already triggers autoloading.

Another use is to throw an exception in unserialize_callback_func, effectively partially implementing something similar to the described \UnserializationFailedException behaviour in the RFC. which will be redundant per PHP 9.0.

Personally I don't think adding unserialize_callback_func to the options array makes sense. It is a confusing setting with limited use.

But more importantly: None of the features in the RFC directly adress the security gap that unserialize() opens. unserialize() in PHP 9.0 wil still happily unserialize any object that will be loaded by the autoloader, unless allowed_classes is specified. I found allowed_classes is not enough. I needed allowed_classes_callback for transitioning. I'd like to keep using the callback to do is_subclass_of() in the future. All this can not be solved with unserialize_callback_func.