[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 whitewinterwolf):

 > in particular I believe the 'C' test was intended to test for equality
 instead of inequality

 Thanks, this is now fixed.

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

 Currently, `is_serialized()` **''already returns true''** for such
 serialized objects :

 {{{
 var_dump(is_serialized('O:8:"Example1":1:{s:10:"cache_file";s:15:"../../index.php";}'));
 #Output: bool(true)
 }}}

 So there is strictly no point in expecting this function to protect
 against any kind of untrusted user input.


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

 If a module stores serialized data in the database, then it won't be
 stored as the raw data but as a serialized string:
 `s:60:"O:8:"Example1":1:{s:10:"cache_file";s:15:"../../index.php";}";`.

 When the modules fetches the value and unserializes it, it will retrieve
 the initial string, not the object.

 If, at any point, a remote user has the ability to confuse some module and
 force it to process a user-controlled raw input string as a serialized
 data, then this means that it is ''the code of this module'' which is
 vulnerable and there is nothing that WordPress could do to prevent this.

 A very quick check return me several Insecure Deserialization
 vulnerabilities affecting Wordpress modules (CVE-2020-36326,
 CVE-2020-28032, CVE-2019-12240, CVE-2018-20148, etc.). Would this
 protection be of any use, WordPress should be immune to this class of
 vulnerabilities. But as explained above, there is strictly no point in
 expecting this function to protect against any kind of untrusted user
 input.

 I understand nevertheless that one may prefer to keep legacy code for the
 legacy PHP versions and keep any historical behavior as much as possible
 to limit the risk of regression to the minimum, and I already updated my
 PR that way.

 Nevertheless, since `is_serialized()` already returns true for serialized
 objects and malicious strings, I do not see any behavior change: it
 returned true before the PR, it returns true after the PR, and do perceive
 one "true" to be safer than the other ;) , the only difference being that
 before the PR it is not possible to enable any actual protection against
 this threat class.

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