Intended RFC Proposal: Side-Effect Filtering for unserialize()

7 years ago by David Strauss — view source

unread

Despite providing class whitelisting [1] and documentation about warnings
about security impacts [2], we continue to see vulnerable uses of
unserialize() in Drupal modules [3] and partially effective attempts to
mitigate vulnerabilities from user-supplied, serialized data [4].

Whitelisting legal classes for unserializing data is, unfortunately, not
seeing widespread uptake in community-created code that I review. It also
doesn't push us toward more secure defaults shipping with OS packages and
on PHP-supporting platforms. An additional option that would generally work
with existing, legitimate use but would also block use for exploits could
turn that tide.

I propose adding a new key to the $options parameter for unserialize(). The
new key would be "exception_on_side_effects", have a Boolean value, and
would (if true) cause unserialization to halt (and an exception to be
thrown) if any of the data being unserialized contains objects with magic
methods that will automatically execute on object wakeup, destruction, or
other events that the PHP interpreter (almost) always triggers.

To help push toward better defaults, I also suggest adding a related
configuration option:

Name: unserialize_side_effect_protection
Default: 0
Changeable: PHP_INI_ALL

If enabled, it would cause "exception_on_side_effects" to be enabled by
default on all unserialize() calls that don't specify "allowed_classes" or
override the default. By making it PHP_INI_ALL, frameworks could lock down
usage during bootstrap (or at least before reading request data).

I am a member of the Drupal Security Team and would also be the implementer
of this feature. My username on the wiki is "dts", and I'm requesting RFC
karma.

[1] https://wiki.php.net/rfc/secure_unserialize
[2] https://secure.php.net/manual/en/function.unserialize.php
[3] https://www.ambionics.io/blog/drupal-services-module-rce
[4]
https://github.com/WordPress/WordPress/blob/efab6e06cae3ed14c6b681570dfd5f81917d9f9c/wp-includes/functions.php#L341-L394

7 years ago by Nikita Popov — view source

unread

On Mon, Jun 12, 2017 at 10:48 PM, David Strauss david@davidstrauss.net
wrote:

Despite providing class whitelisting [1] and documentation about warnings
about security impacts [2], we continue to see vulnerable uses of
unserialize() in Drupal modules [3] and partially effective attempts to
mitigate vulnerabilities from user-supplied, serialized data [4].

Whitelisting legal classes for unserializing data is, unfortunately, not
seeing widespread uptake in community-created code that I review. It also
doesn't push us toward more secure defaults shipping with OS packages and
on PHP-supporting platforms. An additional option that would generally work
with existing, legitimate use but would also block use for exploits could
turn that tide.

I propose adding a new key to the $options parameter for unserialize(). The
new key would be "exception_on_side_effects", have a Boolean value, and
would (if true) cause unserialization to halt (and an exception to be
thrown) if any of the data being unserialized contains objects with magic
methods that will automatically execute on object wakeup, destruction, or
other events that the PHP interpreter (almost) always triggers.

To help push toward better defaults, I also suggest adding a related
configuration option:

Name: unserialize_side_effect_protection
Default: 0
Changeable: PHP_INI_ALL

If enabled, it would cause "exception_on_side_effects" to be enabled by
default on all unserialize() calls that don't specify "allowed_classes" or
override the default. By making it PHP_INI_ALL, frameworks could lock down
usage during bootstrap (or at least before reading request data).

I am a member of the Drupal Security Team and would also be the implementer
of this feature. My username on the wiki is "dts", and I'm requesting RFC
karma.

[1] https://wiki.php.net/rfc/secure_unserialize
[2] https://secure.php.net/manual/en/function.unserialize.php
[3] https://www.ambionics.io/blog/drupal-services-module-rce
[4]
https://github.com/WordPress/WordPress/blob/efab6e06cae3ed14c6b681570dfd5f
81917d9f9c/wp-includes/functions.php#L341-L394

Hi,

how do you intend to handle internal classes in this regard? Internal
classes (if they support serialization at all) will pretty much always use
either __wakeup or Serializable::unserialize(). Does that mean that if
exception_on_side_effects is enabled, most internal classes will be
prohibited?

Regards,
Nikita