Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#53390 new defect (bug)

Unfreeze the code of is_serialized()

Reported by: whitewinterwolf's profile whitewinterwolf Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description

The code of `is_serialized()` is frozen since 2016 as a security measure to protect WordPress and its plugins against PHP Object Injection vulnerabilities, linking to the 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 functionally and security wise).

Standard objects

Let's take all three examples from the 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 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 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 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.

Change History (1)

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.