[wp-trac] [WordPress Trac] #37757: Add `allowed_classes` to `maybe_unserialize` When WordPress is running on PHP 7+

[WordPress Trac]

#37757: Add `allowed_classes` to `maybe_unserialize` When WordPress is running on
PHP 7+
-------------------------------------------------+-------------------------
 Reporter:  chrisguitarguy                       |       Owner:  (none)
     Type:  enhancement                          |      Status:  new
 Priority:  normal                               |   Milestone:  Awaiting
                                                 |  Review
Component:  Security                             |     Version:
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch has-unit-tests 2nd-        |     Focuses:
  opinion                                        |
-------------------------------------------------+-------------------------

Comment (by FrancescoCarlucci):

 Replying to [comment:12 johnbillion]:

 Thanks for the feedback!

 > If a plugin serializes `class Foo {}`, unserialization will break if
 another plugin hooks into the `maybe_unserialize_allowed_classes` filter
 and adds `Bar` to the resulting array of classes names, because `Foo`
 won't be present.


 Of course, but if we initialize the filter as `false`, it will not break
 everything while still allowing developers to harden their WP installation
 without editing the core.

 It would be developers responsibility to check if there are serialized
 classes - if they decide to activate the filter.

 > This will also break unserialization of all WordPress core classes and
 `stdClass` instances that are serialized but aren't present in the
 `maybe_unserialize_allowed_classes` filter return value.

 Are there serialized classes in WP core passed to maybe_unserialize? I
 can't find any: https://github.com/search?q=repo%3AWordPress%2Fwordpress-
 develop%20maybe_unserialize&type=code

 > the original patch on this ticket proposes adding an $options argument
 to maybe_unserialize() instead

 As far as I understand, the original patch is pretty much similar, but
 gives you flexibility on the options to pass, which is good. unserialize()
 at the moment only takes `allowed_classes ` and  `max_depth`, but maybe in
 the future there may be more, so I agree on making the filter act on
 $options.

 > Before doing that though, I think we need to assess the exact threat and
 whether introducing that argument would address it.

 The threat is that every time maybe_unserialize() takes untrusted data as
 an input, PHP object injection is possible and this isn't any good :) If a
 POP Chain is present, it can lead to RCE and arbitrary file deletion. In
 plain English, if a Class that contains a magic method gets unserialized,
 the magic method gets executed. Even the WP Core had a POP chain for a
 while: https://www.wordfence.com/blog/2023/12/psa-critical-pop-chain-
 allowing-remote-code-execution-patched-in-wordpress-6-4-2/

 POP Chains are not rare in libraries as well and they are not considered a
 vulnerability by itself, because they are exploitable only through PHP
 Object Injection, which indeed is considered a vulnerability.

 "every time maybe_unserialize() takes untrusted data as an input" - this
 happens often, because plugin developers tend to use core functions on
 data that are not 100% trusted, a few CVEs to support this:

 - CVE-2024-1770
 - CVE-2024-2693
 - CVE-2024-2694
 - Hundreds more...

 Let me know if it makes any sense!

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