[wp-trac] [WordPress Trac] #53391: Unfreeze the code of is_serialized()

[WordPress Trac]

#53391: Unfreeze the code of is_serialized()
-----------------------------+-----------------------------
 Reporter:  whitewinterwolf  |      Owner:  (none)
     Type:  defect (bug)     |     Status:  new
 Priority:  normal           |  Milestone:  Awaiting Review
Component:  General          |    Version:
 Severity:  normal           |   Keywords:
  Focuses:                   |
-----------------------------+-----------------------------
 The code of [https://github.com/WordPress/wordpress-
 develop/blob/24a70204db8d9ca5c6ddfc2c22247418fa0af5a1/src/wp-
 includes/functions.php#L642 `is_serialized()`] is frozen
 [https://core.trac.wordpress.org/ticket/17375#comment:37 since 2016] as a
 security measure to protect WordPress and its plugins against PHP Object
 Injection vulnerabilities, linking to the
 [https://www.owasp.org/index.php/PHP_Object_Injection OWASP website] as a
 reference.

 The initial idea was that the `is_serialized()` function should return
 `true` only for safe serialized strings, and `false` for unsafe ones to
 prevent their deserialization by the caller.

 However, the current implementation completely fails to achieve that and
 does not bring any protection at all against the aforementioned threat.

 Not only it doesn't add any security, but the fact that the code has been
 flagged as "frozen in time" is nefarious as it blocks any further
 developments (both [https://core.trac.wordpress.org/ticket/53299
 functionally] and [https://core.trac.wordpress.org/ticket/53295 security]
 wise).


 == Standard objects ==

 Let's take all three examples from the [https://owasp.org/www-
 community/vulnerabilities/PHP_Object_Injection#examples OWASP page] used
 to support and document this choice.

 The input string being malicious, `is_serialized()` is expected to return
 `false` for each of the following examples, as per documented in the
 original [https://core.trac.wordpress.org/ticket/17375#comment:37 ticket
 17375].

 **''I highlight that these are the exact examples used to support and
 document the choice of freezing `is_serialized()`''**, nothing has been
 changed.


 === Example 1 ===

 Malicious payload:

 {{{
 http://testsite.com/vuln.php?data=O:8:"Example1":1:{s:10:"cache_file";s:15:"../../index.php";}
 }}}

 Returned value:

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

 The function **failed** to properly detect and block the malicious
 serialized object.


 === Example 2 ===

 Malicious payload:

 {{{
 Cookie:
 data=O%3A8%3A%22Example2%22%3A1%3A%7Bs%3A14%3A%22%00Example2%00hook%22%3Bs%3A10%3A%22phpinfo%28%29%3B%22%3B%7D
 }}}

 Returned value:

 {{{
 var_dump(is_serialized(urldecode('O%3A8%3A%22Example2%22%3A1%3A%7Bs%3A14%3A%22%00Example2%00hook%22%3Bs%3A10%3A%22phpinfo%28%29%3B%22%3B%7D')));
 # Output: bool(true)
 }}}

 The function **failed** to properly detect and block the malicious
 serialized object.


 === Example 3 ===

 Malicious payload generation:

 {{{
 class SQL_Row_Value
 {
    private $_table = "SQL Injection";
 }

 class Example3
 {
    protected $obj;

    function __construct()
    {
       $this->obj = new SQL_Row_Value;
    }
 }

 print urlencode(serialize(new Example3));
 # Output:
 O%3A8%3A%22Example3%22%3A1%3A%7Bs%3A6%3A%22%00%2A%00obj%22%3BO%3A13%3A%22SQL_Row_Value%22%3A1%3A%7Bs%3A21%3A%22%00SQL_Row_Value%00_table%22%3Bs%3A13%3A%22SQL+Injection%22%3B%7D%7D
 }}}

 Returned value:

 {{{
 var_dump(is_serialized(urldecode('O%3A8%3A%22Example3%22%3A1%3A%7Bs%3A6%3A%22%00%2A%00obj%22%3BO%3A13%3A%22SQL_Row_Value%22%3A1%3A%7Bs%3A21%3A%22%00SQL_Row_Value%00_table%22%
 3Bs%3A13%3A%22SQL+Injection%22%3B%7D%7D')));
 # Output: bool(true)
 }}}

 The function **failed** to properly detect and block the malicious
 serialized object.


 == Serializable classes ==

 In addition to the standard serialized objects seen above, PHP also offers
 [https://www.php.net/manual/en/class.serializable.php an alternate
 mechanism] which basically replaces the call to the magic function
 `_wakeup()` by a call to the Serializable interface function
 `deserialize()`.

 A specific attention has been given to prevent the deserialization of such
 objects, notably by the addition of a [https://github.com/WordPress
 /wordpress-
 develop/blob/376840b83cb11167d5fe545e929b37d41ce58d6e/tests/phpunit/tests/functions.php#L367
 dedicated test case]. In particular, `is_serialized()` ensures that the
 first character of the serialized string must not be a 'C'.

 Nevertheless, here again, if we take all OWASP examples and replace the
 standard objects by a Serializable object embedded within an array,
 `is_serialized()` systematically fails to detect and block the malicious
 payload:

 - Example 1:

 {{{
 var_dump(is_serialized('a:1:{i:0;C:8:"Example1":23:{s:15:"../../index.php";}}'));
 # Output: bool(true)
 }}}

 - Example 2:

 {{{
 var_dump(is_serialized(urldecode('a%3A1%3A%7Bi%3A0%3BC%3A8%3A%22Example2%22%3A18%3A%7Bs%3A10%3A%22phpinfo%28%29%3B%22%3B%7D%7D')));
 # Output: bool(true)
 }}}

 - Example 3:

 {{{
 var_dump(is_serialized(urldecode('a%3A1%3A%7Bi%3A0%3BC%3A8%3A%22Example3%22%3A72%3A%7BO%3A13%3A%22SQL_Row_Value%22%3A1%3A%7Bs%3A21%3A%22SQL_Row_Value_table%22%3Bs%3A13%3A%22S
 QL+Injection%22%3B%7D%7D%7D')));
 # Output: bool(true)
 }}}


 == Conclusion ==

 Freezing `is_serialized()` code as a security measure to block malicious
 payloads proves to be a totally ineffective measure as it fails to block
 any single one of the examples linked when this decision was taken and did
 not prevent WordPress from being vulnerable to any Insecure
 Deserialization vulnerabilities since then (a very quick search shows
 CVE-2020-36326, CVE-2020-28032, CVE-2019-12240, CVE-2018-20148, etc.).

 This freeze should therefore be lifted to allow further developments,
 notably the implementation of more efficient protections against such
 threats.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/53391>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform