Fix bug 81111: Prevent serialization of anonymous classes

[camporter]

This is a pretty basic solution for preventing __serialize on anonymous classes for bug 81111.

It's probably not very elegant though...

This has the side effect of ignoring __serialize for classes that extend internal classes that use zend_class_serialize_deny. For example,

class Something extends CURLFile {
    public function __serialize() { return []; }
    public function __unserialize($value) { return new self('file'); }
}
var_dump(serialize(new Something('file')));
// Fatal error: Uncaught Exception: Serialization of 'Something' is not allowed

That seems like its better than allowing serialization. However, using __unserialize for a class that extends an internal class is still possible:

var_dump(unserialize('O:9:"Something":0:{}'));
// object(Something)#2 (3) {
//  ["name"]=>
//  string(0) ""
//  ["mime"]=>
//  string(0) ""
//  ["postname"]=>
//  string(0) ""
//}

[TysonAndre]

The patch seems like it should prevent serialization.
Unserialization should probably also be forbidden at the same time - as I assumed, it was allowed if __unserialize was defined.

--- a/ext/standard/var_unserializer.re
+++ b/ext/standard/var_unserializer.re
@@ -17,6 +17,7 @@
 #include "php.h"
 #include "ext/standard/php_var.h"
 #include "php_incomplete_class.h"
+#include "zend_interfaces.h"
 #include "zend_portability.h"
 
 /* {{{ reference-handling for unserializer: var_* */
@@ -756,12 +757,16 @@ static inline int object_custom(UNSERIALIZE_PARAMETER, zend_class_entry *ce)
 #ifdef PHP_WIN32
 # pragma optimize("", off)
 #endif
-static inline int object_common(UNSERIALIZE_PARAMETER, zend_long elements, bool has_unserialize)
+static inline int object_common(UNSERIALIZE_PARAMETER, zend_class_entry *ce, zend_long elements, bool has_unserialize)
 {
        HashTable *ht;
        bool has_wakeup;
 
        if (has_unserialize) {
+               if (UNEXPECTED(ce->unserialize == zend_class_unserialize_deny)) {
+                       zend_class_unserialize_deny(rval, ce, (const unsigned char *)"", 0, (zend_unserialize_data *)var_hash);
+                       return 0;
+               }
                zval ary, *tmp;
 
                if (elements >= HT_MAX_SIZE) {
@@ -1315,7 +1320,7 @@ object ":" uiv ":" ["]    {
        }
        zend_string_release_ex(class_name, 0);
 
-       return object_common(UNSERIALIZE_PASSTHRU, elements, has_unserialize);
+       return object_common(UNSERIALIZE_PASSTHRU, ce, elements, has_unserialize);
 }
 
 "E:" uiv ":" ["] {

php > echo serialize(new SplFileInfo('.'));

Warning: Uncaught Exception: Serialization of 'SplFileInfo' is not allowed in php shell code:1
Stack trace:
#0 php shell code(1): serialize(Object(SplFileInfo))
#1 {main}
  thrown in php shell code on line 1
php > class Y extends SplFileInfo { public function __serialize() { return []; } public function __unserialize($data) { $this->data = $data; } }
php > echo serialize(new Y('.'));
O:1:"Y":0:{}
php > var_dump(unserialize('O:1:"Y":0:{}'));
object(Y)#2 (2) {
  ["data"]=>
  array(0) {
  }
  ["pathName":"SplFileInfo":private]=>
  string(0) ""
}

[camporter]

Thanks, I hadn't tracked down the unserialize side because I wasn't sure if blocking on zend_class_serialize_deny/zend_class_unserialize_deny was ok as a larger backwards incompatible change to resolve anonymous classes. I'll add that and tests here soon.

[dstogov]

The patch is OK for me. I'm not completely sure how it it's going to work after removing Serializable interface. (It's already deprecated in 8.1).

I think, it way be easier to check ZEND_ACC_ANON_CLASS flag.
@nikic please take a look

[dstogov]

It's also possible to disable declaration of __serialize/__unserialize methods for anonymous classes in first place (in compiler).

[smalyshev]

@dstogov this may lead to problems with anonymous classes extending ones that already declare those methods. There should be no problem with doing this, as long as these objects aren't actually serialized. One may use serialization in one scenario and anonymous class in another, it shouldn't be a problem.

[smalyshev]

Wasn't zend_class_serialize_deny already supposed to, you know, deny serialization by itself?

[TysonAndre]

The patch is OK for me. I'm not completely sure how it it's going to work after removing Serializable interface. (It's already deprecated in 8.1).

I assume the ce->serialize may be moved to a bit flag, or it may be preserved if any important PECLs haven't started implementing __serialize

Wasn't zend_class_serialize_deny already supposed to, you know, deny serialization by itself?

It was added before __serialize was added in php 7.4 - a zend_class_entry * has both ->serialize and ->__serialize. Because __serialize takes precedence over userland Serializable::serialize() (and a prefix determines which is checked when unserializing), this code wouldn't even call zend_class_serialize_deny or zend_class_unserialize_deny.

It's also possible to disable declaration of __serialize/__unserialize methods for anonymous classes in first place (in compiler).

I'd also considered doing that but things like creating anonymous classes extending classes that (1) extend Serializable, or (2) implement __serialize and __unserialize would break.

Ignoring __serialize silently seems consistent with ignoring Serializable silently (if deprecation notices are suppressed)

php > $x = new class implements Serializable{ public function serialize() { return '';} public function unserialize($value) { return new self(); } };
php > var_export(serialize($x));

Warning: Uncaught Exception: Serialization of 'class@anonymous' is not allowed in php shell code:1
Stack trace:
#0 php shell code(1): serialize(Object(class@anonymous))
#1 {main}
  thrown in php shell code on line 1

[TysonAndre]

I think, it way be easier to check ZEND_ACC_ANON_CLASS flag.

1. It's possible to extend an anonymous class using class_alias+get_class($anonymous) to give the anonymous class a valid identifier as a name
2. Anonymous classes are not the only thing in php-src using zend_class_(un)serialize_deny - my bug report also applies to SplFileInfo and others - see my previous comment

php > class Y extends SplFileInfo { public function __serialize() { return []; } public function __unserialize($data) { $this->data = $data; } }
php > echo serialize(new Y('.'));
O:1:"Y":0:{}
php > var_dump(unserialize('O:1:"Y":0:{}'));
object(Y)#2 (2) {
  ["data"]=>
  array(0) {
  }
  ["pathName":"SplFileInfo":private]=>
  string(0) ""
}

[camporter]

@TysonAndre Something that is a bit confusing about checking for zend_class_unserialize_deny and calling it if __unserialize is also present is that:

var_dump(unserialize('O:15:"ReflectionClass":0:{}'));

provides a warning and notice and returns false from unserialize.
Whereas:

class Something extends ReflectionClass {
    public function __serialize() { return []; }
    public function __unserialize($value) { return new self(''); }
}
var_dump(unserialize('O:9:"Something":0:{}'));

would throw an exception.
Should the same behavior of providing a warning and notice and returning false from unserialize be preferred?

[krakjoe]

Just to note that Nikita and I discussed this briefly when it was reported.
These handlers are destined for removal, and so checking ACC_ANON will soon not be acceptable. We will at that time need an additional ACC constant.

We consider it worth while implementing that now, rather than putting a band aid on this, only to have to change it later.

I don't have time to do this right now myself.

[TysonAndre]

Anonymous classes are not the only thing in php-src using zend_class_(un)serialize_deny - my bug report also applies to SplFileInfo and others - see my previous comment

@krakjoe when that does get solved, the same fix would need to be made for SplFileInfo

The feature freeze is in 5 days - @dstogov already approved this, and I was waiting for feedback from @nikic (e.g. I doubt many projects would be affected by this, but do any of the top composer packages combine __serialize/__unserialize with anonymous classes (not from inheritance, not to throw)? I can't imagine that being done since the name would change)

[nikic]

I've opened #7249 for the alternative approach.

[nikic]

Closing this in favor of #7249.