Make WordPress Core

Opened 8 years ago

Last modified 7 years ago

#37757 new enhancement

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

Reported by: chrisguitarguy's profile chrisguitarguy Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Security Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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.

Attachments (4)

37757.diff (1.2 KB) - added by NathanAtmoz 7 years ago.
first pass at a patch
ticket-37757.patch (1.2 KB) - added by seuser 7 years ago.
Updated patch with tests and minor bugfix
ticket-37757.2.patch (6.5 KB) - added by seuser 7 years ago.
Updated patch with tests and minor bugfix (fixed)
ticket-37757.3.patch (8.2 KB) - added by seuser 7 years ago.
Updated patch with filter and tests for filter.

Download all attachments as: .zip

Change History (13)

#1 @swissspidy
8 years ago

  • Version 4.7 deleted

@NathanAtmoz
7 years ago

first pass at a patch

#2 @NathanAtmoz
7 years ago

  • Keywords has-patch needs-unit-tests added

@seuser
7 years ago

Updated patch with tests and minor bugfix

@seuser
7 years ago

Updated patch with tests and minor bugfix (fixed)

#3 @seuser
7 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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.

#4 follow-up: @iandunn
7 years ago

  • Component changed from General to Security

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
7 years ago

Updated patch with filter and tests for filter.

#5 follow-up: @seuser
7 years ago

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?

#6 in reply to: ↑ 4 @NathanAtmoz
7 years ago

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 );

#7 @iandunn
7 years ago

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.

#8 in reply to: ↑ 5 @iandunn
7 years ago

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.

#9 @seuser
7 years ago

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.

Note: See TracTickets for help on using tickets.