Add `allowed_classes` to `maybe_unserialize` When WordPress is running on PHP 7+

[chrisguitarguy]

PHP 7 added an options array to unserialize: ​http://php.net/manual/en/function.unserialize.php

The most notable option is passing a whitelist of classes that can be unserialized which can help mitigate some remote code execution vulnerabilities.

Something like this (PHP 5.X will throw a warning if a second argument is provided to unserialize).

if (!is_serialized($input)) {
  return false;
}

return PHP_MAJOR_VERSION >= 7 ? @unserialize($input, apply_filters('wp_maybe_unserialize_options', [])) : @unserialize($input);

By default, I don't think any whitelisting needs to be done -- would be a huge BC break. But it would be nice to give developers an option to lock down what can be unserialized via maybe_unserialize.

[NathanAtmoz]

first pass at a patch

[seuser]

Updated patch with tests and minor bugfix

[seuser]

Updated patch with tests and minor bugfix (fixed)

[seuser]

Added a new patch with unit tests including coverage for ensuring vulnerabilities aren't introduced in interface or extended classes. Updated the defined() to add single quotes to match phpdoc for that and fixed minor coding standards issues.

[iandunn]

Would this need to be a filter (like in the ticket description) in order to be useful? Most calls to maybe_unserialize() are done internally by WP (e.g., inside get_option()), rather than directly by plugins, so I'm not sure how plugins or site admins would be able to take advantage of the extra parameter setup in ticket-37757.2.patch​, unless they started bypassing the API and calling it directly, which I don't think we want to encourage.

I think it'd also be good to keep in mind that ​`unserialize()` is considered dangerous even with `allowed_classes`, so if something like this is merged, it might be good to make it very clear in the filter docblock that it shouldn't be considered a safe way to use unserialize(), and that there's no guarantee it'll prevent vulnerabilities; it's just extra hardening, and unserialize() should still be avoided as much as possible. It's fine to use maybe_unserialize() indirectly through the API, since Core keeps it safe, but inputs should still be validated, and if a plugin needs to directly encode non-scalar data, it should use JSON.

[seuser]

Updated patch with filter and tests for filter.

[seuser]

Makes sense, thanks for the feedback. I've added the filter and a test for that filter and added specific documentation to the docblocks - will that automatically get picked up by the documentation pages?

[NathanAtmoz]

Replying to iandunn:

Would this need to be a filter (like in the ticket description) in order to be useful? Most calls to maybe_unserialize() are done internally by WP (e.g., inside get_option()), rather than directly by plugins, so I'm not sure how plugins or site admins would be able to take advantage of the extra parameter setup in ticket-37757.2.patch​, unless they started bypassing the API and calling it directly, which I don't think we want to encourage.

I can see the reasoning behing wanting this to be a filterable option. But adding a filter to allowed classes seems like a bad idea to me. This would allow an arbitrary plugin or theme to basically 'reset' the allowed classes to accept all classes.

<?php
  add_filter( 'unserialization_options', '__return_true', 99999999 );

Instead of one filter to control all unserialization options, I think it'd be better to introduce the unserialization_options_{$option} filter. So, e.g. in get_option() we would add the unserialization options filter just before returning the options.

<?php
  $unserialization_options = apply_filters( "unserialization_options_{$option}", true, $option );
  return apply_filters( "option_{$option}", maybe_unserialize( $value, $unserialization_options ), $option );

[iandunn]

This would allow an arbitrary plugin or theme to basically 'reset' the allowed classes to accept all classes.

I'm not really tracking with that line of thinking. A plugin could also also add_filter( 'has_cap', '__return_true' ) or add_filter( 'authenticate', function() { return get_user_by( 'id', 1 ); } ), but that doesn't mean we shouldn't have those filters. Is there something about this particular case that makes it more likely to be misused than those?

I don't think of installed plugins and themes as being arbitrary, since they were intentionally chosen and installed, and by their nature have root access to WP and the database. A site owner has the responsibility to only install plugins that they trust with that access. The owner could also add_filter( 'unserialization_options', 'whitelist_unserialize_classes', 100000000 ), if they wanted to override unwanted behavior from a plugin.

There are 16 calls to maybe_unserialize() across the codebase, so, if I understand it correctly, an approach like unserialization_options_{$option} would require a different variation of the filter to be added and maintained for each of them, as opposed to just a single filter inside maybe_unserialize() itself. Is that right? So far I'm not personally seeing a benefit to the approach that would outweigh that added complexity.

[iandunn]

Replying to seuser:

added specific documentation to the docblocks - will that automatically get picked up by the documentation pages?

Yeah, developer.wordpress.org/reference is built automatically from the inline docs.

[seuser]

Nice, thanks for the info.

Your arguments make sense to me - once a plugin is installed, regardless of whether it interacts with WP, it would need to be trusted as you say.

A compromise could be to pass through the option name from the get_option function as a third argument to this. That could allow the developer to modify the unserialise options only for a specific option.

<?php
function get_option( ... ) {
        // ...
        return apply_filters( "option_{$option}", maybe_unserialize( $value, array(), $option ), $option );
}

<?php
function maybe_unserialize( $value, $option = array(), $key ) {
        // ...
        $options = apply_filters( 'unserialization_options', $options, $original, $key ); 
        // ...
}

But in that case, in the pre filter in get_option could similarly be used to target a specific option, albeit with a bit more effort. For that reason, I'd be happy to leave as is.

[johnbillion]

#62970 was marked as a duplicate.

[FrancescoCarlucci]

Hello people :)

This is the patch I proposed if you wanna take a look: ​https://github.com/WordPress/wordpress-develop/pull/8332

[johnbillion]

I think the proposed filter will break a lot of things.

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. 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.

Example code that breaks everything:

<?php
add_filter( 'maybe_unserialize_allowed_classes', function( $allowed ) {
  if ( ! is_array( $allowed ) ) {
    $allowed = [];
  }

  $allowed[] = 'Bar';

  return $allowed;
} );

This example code also demonstrates that plugins will need to include logic to handle a boolean value. A plugin would be unable to determine if a value of true is the default value or one explicitly set by another plugin.

The original patch on this ticket proposes adding an $options argument to maybe_unserialize() instead. This is useful when code calls maybe_unserialize() directly, but not useful when code calls functions that make use of it such as get_option(), get_transient(), the various get_*_meta() functions, etc.

Perhaps a better approach would be to add an $options argument to all functions that can call maybe_unserialize() or unserialize() and pass the $options argument through.

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

[FrancescoCarlucci]

Replying to 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!

[siliconforks]

Replying to FrancescoCarlucci:

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

Everything in the wp_options table and metadata tables gets passed to maybe_unserialize. WordPress core sometimes stores objects of type stdClass here (I'm not sure if there are other classes).

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.

I believe that @johnbillion is talking about this patch which added another parameter to maybe_unserialize without adding a filter.

[FrancescoCarlucci]

Replying to siliconforks:

Everything in the wp_options table and metadata tables gets passed to maybe_unserialize. WordPress core sometimes stores objects of type stdClass here (I'm not sure if there are other classes).

That's what I am saying, the majority of things are serialized as strings, not objects. If WP uses stdClass we could comment the filter to highlight that stdClass class should be added in the allowed_classes whitelist :)

I believe that @johnbillion is talking about this patch which added another parameter to maybe_unserialize without adding a filter.

I see. That's why I believe a filter is the best option. While is true by default, there is no risk of breaking BC, while it offers developers a hook to harden WP against PHP Object injection.

Here is another patch that merges not approaches and shouldn't break stuff :)

<?php
function maybe_unserialize( $data ) {
        if ( is_serialized( $data ) ) { // Don't attempt to unserialize data that wasn't serialized going in.

                $options = apply_filters(
                        'maybe_unserialize_options', 
                        [
                                'allowed_classes' => true, 
                                'max_depth'       => 4096,
                        ]
                );

                return @unserialize( trim( $data ), $options );
        }

        return $data;
}

Example usage of the filter:

<?php
add_filter( 'maybe_unserialize_options', function( $options ) {
    $options['allowed_classes'] = [ 'AllowedClass1', 'AllowedClass2' ]; // Define your whitelist.
    return $options;
});