php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #74436 Mixing Serializable and __wakeup() causes conflict
Submitted: 2017-04-13 20:41 UTC Modified: 2020-02-28 18:32 UTC
Votes:2
Avg. Score:4.5 ± 0.5
Reproduced:2 of 2 (100.0%)
Same Version:1 (50.0%)
Same OS:1 (50.0%)
From: tom at altrooz dot com Assigned: cmb (profile)
Status: Wont fix Package: Class/Object related
PHP Version: 5.6.30 OS: Centos 6
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: tom at altrooz dot com
New email:
PHP Version: OS:

 

 [2017-04-13 20:41 UTC] tom at altrooz dot com
Description:
------------
This issue was introduced in 5.6.30, and does not exist in 5.6.29.  Unfortunately the data structures that showed this effect are too complicated to include, and my effort to create a smaller reproducable case was not successful.  But here's the story:

*) A class implemented Serializable
*) It has a member of type SplFixedArray, which has a __wakeup() function
*) The container held other class instances that did not override the default serialization behavior
*) Some of those classes included instances of ImmutableDateTime, which also has a __wakeup() function

When unserializing the top-level class, the __wakeup() function on the SplFixedArray and ImmutableDateTime instances did not get called.


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-04-14 17:49 UTC] danack@php.net
The version of PHP you've reported this bug for is only receiving security updates.

If you can't provide a reproduce case that shows the problem in PHP 7, it is unlikely that anyone would look at this.
 [2017-04-14 17:58 UTC] tom at altrooz dot com
And introducing a new significant flaw in security update 5.6.30 isn't a concern?  As stated, I was not able to create a reproducable case but it seems a code review of http://bugs.php.net/70213 and http://bugs.php.net/73825 might be wise.
 [2017-04-14 19:18 UTC] nikic@php.net
To clarify, at which point did you check whether the __wakeup() has been called? Did you check this after the unserialization finished completely, or did you check this in the Serializable::unserialize() method of your class?

If the latter, then this is indeed how unserialization behaves now: __wakeup() will only be called at the very end of the unserialization, which also means that Serializable::unserialize() methods (unfortunately) will see the objects prior to __wakeup(). The objects will be woken up at a later point though. If this is the problem you're experiencing, I'm sorry to say that this is unlikely to change -- delaying __wakeup() calls until after ::unserialize() is an important part of the security issue being fixed here. The only alternative to this would be to remove the serialization context sharing for Serialization classes, which we believe to be a larger backwards compatibility break.
 [2017-04-14 19:38 UTC] tom at altrooz dot com
That is valuable information and one would hope that future security patches don't change application behavior.  I'm sure you can now create a simple test case - The code that failed here is in the CakePHP framework, and has this innocuous construct (edited for brevity):

class XXX implements Serializable {
   private $contents;  // expected to be an instance of SplFixedArray
   private $contents_size;

   public function serialize() {
      return serialize($this->contents);
   }

   public function unserialize($data) {
      $this->contents = unserialize($data);
      $this->contents_size = count($this->contents);
   }
}

It is expected that when the SplFixedArray instance is rehydrated, it is now complete and can be referenced.  In 5.6.29 (and of course every version before) this works correctly, but in 5.6.30 the count(SplFixedArray) returns 0 because __wakeup() has not been called on it.

*All* object-oriented languages guarantee that class instances are completely instantiated before they are able to be accessed by the developer.
 [2017-04-17 17:40 UTC] tom at altrooz dot com
It does appear that this has significantly affected users of the CakePHP community.  See https://github.com/cakephp/cakephp/issues/10111 for a sense of how much developer time is being wasted, and also with PHP 7.x.
 [2020-02-28 18:32 UTC] cmb@php.net
-Status: Open +Status: Wont fix -Assigned To: +Assigned To: cmb
 [2020-02-28 18:32 UTC] cmb@php.net
Sorry, if that change caused any harm, but a *vulnerability* had
to be fixed, and due to fundamental issues of Serializable that
was not possible without a BC break.  The likely minor break has
been chosen, and this will stay this way.

The good news: as of PHP 7.4.0, there is a third serialization
mechanism[1] which does not have these fundamental issues.  I
suggest to upgrade relevant code to use this new mechanism as soon
as possible.

Thanks.

[1] <https://wiki.php.net/rfc/custom_object_serialization>
 
PHP Copyright © 2001-2020 The PHP Group
All rights reserved.
Last updated: Fri Nov 27 15:01:23 2020 UTC