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 | 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)
Change History (13)
#3
@
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:
↓ 6
@
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.
#5
follow-up:
↓ 8
@
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
@
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., insideget_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
@
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
@
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
@
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.
first pass at a patch