[wp-trac] [WordPress Trac] #53295: Serialized data should be handled as an opaque value

[WordPress Trac]

#53295: Serialized data should be handled as an opaque value
-----------------------------+------------------------------
 Reporter:  whitewinterwolf  |       Owner:  (none)
     Type:  defect (bug)     |      Status:  new
 Priority:  normal           |   Milestone:  Awaiting Review
Component:  General          |     Version:
 Severity:  normal           |  Resolution:
 Keywords:  has-patch        |     Focuses:
-----------------------------+------------------------------

Comment (by siliconforks):

 Replying to [comment:13 whitewinterwolf]:
 > **A)** The `allowed_classes` filter has been added to the
 `unserialize()` function by the PHP team with the sole goal to prevent any
 possibility of malicious code execution triggered by the deserialization
 process (see [https://wiki.php.net/rfc/secure_unserialize secure
 unserialize]). Do you mean that they failed their attempt and that calling
 `unserialized($data, ['allowed_classes' => false])` is still prone to
 unsecure deserialization vulnerabilities? Do you have any reference or
 example?

 My concern is not just with vulnerabilities inside the implementation of
 `is_serialized()` itself, but also in the grander scheme of things - what
 are the consequences if `is_serialized()` gives the wrong answer?  If it
 returns the wrong result, can this lead to a case where WordPress calls
 `unserialize()` on untrusted user input?

 > **B)** Do you mean that the legacy and new code returns different values
 for the same serialized object data provided as input? If so, do you have
 any example?

 Well, in the current version of the PR, the PHP 7 branch seems buggy - in
 particular I believe the `'C'` test was intended to test for equality
 instead of inequality - but even if you fix issues like this, I don't
 think there's really any way to make it work without introducing a
 vulnerability.

 For example, consider the string
 `O:8:"Example1":1:{s:10:"cache_file";s:15:"../../index.php";}ab84a81d5a516892c86d3a620ac3e714286b8dcdcdebaf67e8112a54ed2514f0`.
 My understanding is that this is what serialized data looks like when
 Snuffleupagus is enabled.  Currently in WordPress, `is_serialized()` will
 return `false` for that.  If you want Snuffleupagus to work with that
 string, you'll want to change `is_serialized()` to return `true` - but now
 the behavior of `is_serialized()` has changed.  This means that, with the
 current version of WordPress, an attacker could input that string as, say,
 his first name, and that would not be considered serialized data.  But in
 the future, if WordPress were upgraded to a version with an implementation
 of `is_serialized()` returning `true` for that string, it would then be
 considered serialized data and would be unserialized, potentially causing
 code execution.

 > **C)** Do you mean that this function has been stated as ''"frozen in
 time"'', so there is by principle no point discussing any change to its
 content anyway?

 I might be missing something, but I can't think of any way you could
 change the behavior of `is_serialized()` without breaking something or
 potentially introducing a vulnerability.  (The implementation of
 `is_serialized()` could change, but it would still need to return `true`
 for the same strings and `false` for the same strings as it did before.)

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/53295#comment:14>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform