[wp-trac] [WordPress Trac] #62970: Adding Defense-in-Depth against PHP Object Injection

[WordPress Trac]

#62970: Adding Defense-in-Depth against PHP Object Injection
-------------------------------+-----------------------------
 Reporter:  FrancescoCarlucci  |      Owner:  (none)
     Type:  enhancement        |     Status:  new
 Priority:  normal             |  Milestone:  Awaiting Review
Component:  Security           |    Version:
 Severity:  normal             |   Keywords:
  Focuses:                     |
-------------------------------+-----------------------------
 The WordPress core function maybe_unserialize() uses PHP unserialization
 in an insecure way, and I think we are all well aware of that.

 The reason is that PHP unserialize automatically instantiates any object
 received as input, and if the object uses magic methods it can lead to
 RCE, file deletion and other dangerous security issues. In the last year
 there were hundreds of security vulnerabilities caused by unsafe use of
 maybe_unserialize(), and with this ticket I want to propose a fix at core
 level, which in my opinion is easy straightforward :)

 PHP unserialize() accepts a second parameter `allowed_classes` that can be
 set to false to disallow automatic class instantiation, or can be
 restricted to specific classes.

 So we can tweak maybe_unserialize() this way:

 {{{#!php
 <?php
 function maybe_unserialize( $data ) {
 if ( is_serialized( $data ) ) { // Don't attempt to unserialize data that
 wasn't serialized going in.
   $allowed_classes = apply_filters( 'maybe_unserialize_allowed_classes',
 false );
   return @unserialize( trim( $data ), [ 'allowed_classes' =>
 $allowed_classes ] );
 }

 return $data;
 }
 }}}

 This way allowed_classes is false by default and can be hooked to true or
 a set of classes, this would be a security-first approach.

 Other ways, we can have a backward-compatibility first approach:

 {{{#!php
 <?php
 function maybe_unserialize( $data ) {
 if ( is_serialized( $data ) ) { // Don't attempt to unserialize data that
 wasn't serialized going in.
   $allowed_classes = apply_filters( 'maybe_unserialize_allowed_classes',
 true );
   return @unserialize( trim( $data ), [ 'allowed_classes' =>
 $allowed_classes ] );
 }

 return $data;
 }
 }}}

 And then site owners can harden their WP instance with this one-liner:

 {{{#!php
 <?PHP

 add_filter( 'maybe_unserialize_allowed_classes', '__return_false' );

 }}}

 To restrict instantiations to specific classes, plugins could hook in like
 this:

 {{{#!php
 <?php
 add_filter( 'maybe_unserialize_allowed_classes', function() {
     return ['MyClass', 'AnotherClass', 'SomeOtherClass'];
 });

 }}}

 Anything else needed to merge it in the core? Does anyone see anything
 that could go wrong?

 Thanks,
 Francesco

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/62970>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform