Unfreeze the code of is_serialized()

[whitewinterwolf]

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.

[dmsnell]

While working on #59233 I came across this issue and thought I’d share some updates that have occurred which might be relevant, and some testing data.

• WordPress’ minimum-supported version of PHP is 7.2.24 at the time of writing this comment. Every supported version allows passing array( "allowed_classes" => false ) to unserialize().
• Passing false for allowed_classes seems to be 100% safe against executing code during the is_serialized() check. Every supported version of PHP specifically skips over any attempt to autoload, call the unserialize_callback_func function, or execute __wakeup(). __unserialize(), or unserialize() methods.

Not everything has changed:

• Protection for one specific security issue in a large category is provided by the current freeze.
• Practical risk remains mostly a concern of unserialize() where people are intending to execute wake-up code (whether they realize this or not).
• Risk with unserialize() applies potentially to all objects and all properties, not just those dealing with unserialization. If user-provided input can leak into unserialize() then it can leak into methods which are triggered at runtime after unserialization.

It can be helpful to balance our discussion with an acknowledgement that maintaining a spec-non-compliant function called is_serialized() can be misleading and invite its own set of security risks. Subtle differences in the official and custom behaviors can lead to thinking data is safe when it isn’t.

Can we unserialize without executing code?

From what I can tell, the primary question is whether we have a way to prevent user-provided data from executing when calling unserialize(). The clear and definitive answer to this is “no.” The reason is that we cannot generally know which parts of the data are user-provided.

• We could prevent all code execution, use reflection to create instances of classes of the requested type, fill their properties, and then treat them as if they were initialized.
• But if we do this, we knowingly corrupt classes which depend on initialization and transformation of their serialized values.
• Worst of all, preventing execution during is_serialized() and during unserialize() does not guarantee that WordPress will not execute user-provided code.

From my limited perspective on this issue, it seems like the responsibility has fallen on authors of new classes to ensure that those classes avoid introducing a path from unserialization to execution. This can be difficult since “callback”s are often nothing more than string names, some of this conflate with common non-callback vocabulary, e.g. add_role or remove_action (which are names I could imagine a class using internally in some state designation, or user data which might return true when passed into is_callable()).

---

If we are going to consider cases such as calling unserialize() on a user’s display name then I think we might want to consider attempting to more intentionally discourage treating non-serialized data as potentially serialized.

In any case, I think it’s a reasonable perspective that attempting to solve deeper runtime security issues at the surface like this has its limits and may not be fully coherent. We can easily point to a single case that it guards against but it’s more difficult to assess the risk we create by advertising something with a well-defined meaning is_serialized() that does something unexpected like should_be_unseralized() or allowed_unserializable().

We can attack the problem at many fronts simultaneously, improving the safety of unserialize() and is_serialized(), encouraging stronger patterns when passing data into unserialization, educating each other on __wakeup() and other callback property issues, and providing a more reliable answer to the question, “could this value represent serialized data?”