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

[WordPress Trac]

#53390: 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           |  Resolution:
 Keywords:                   |     Focuses:
-----------------------------+------------------------------

Comment (by 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?”

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