Merge pull request #112 from deliciousbrains/improve-unserialization-security

Background processing class now accepts $allowed_classes in constructor

Author: ianmjones

README.md

@@ -178,12 +178,64 @@ foreach ( $items as $item ) {
 }
 ```
 
+An item can be any valid PHP value, string, integer, array or object. If needed, the $item is serialized when written to the database.
+
 Save and dispatch the queue:
 
 ```php
 $this->example_process->save()->dispatch();
 ```
 
+#### Handling serialized objects in queue items
+
+Queue items that contain non-scalar values are serialized when stored in the database. To avoid potential security issues during unserialize, this library provides the option to set the `allowed_classes` option when calling `unserialize()` which limits which classes can be instantiated. It's kept internally as the protected `$allowed_batch_data_classes` property.
+
+To maintain backward compatibility the default value is `true`, meaning that any serialized object will be instantiated. Please note that this default behavior may change in a future major release.
+
+We encourage all users of this library to take advantage of setting a strict value for `$allowed_batch_data_classes`. If possible, set the value to `false` to disallow any objects from being instantiated, or a very limited list of class names, see examples below.
+
+Objects in the serialized string that are not allowed to be instantiated will instead get the class type `__PHP_Incomplete_Class`.
+
+##### Overriding the default `$allowed_batch_data_classes`
+
+The default behavior can be overridden by passing an array of allowed classes to the constructor:
+
+``` php
+$allowed_batch_data_classes = array( MyCustomItem::class, MyItemHelper::class );
+$this->example_process = new WP_Example_Process( $allowed_batch_data_classes );
+```
+
+Or, set the value to `false`:
+
+``` php
+$this->example_process = new WP_Example_Process( false );
+```
+
+
+Another way to change the default is to override the `$allowed_batch_data_classes` property in your process class:
+
+``` php
+class WP_Example_Process extends WP_Background_Process {
+
+	/**
+	 * @var string
+	 */
+	protected $prefix = 'my_plugin';
+
+	/**
+	 * @var string
+	 */
+	protected $action = 'example_process';
+
+	/**
+	 *
+	 * @var bool|array
+	 */
+	protected $allowed_batch_data_classes = array( MyCustomItem::class, MyItemHelper::class );
+	...
+
+```
+
 #### Background Process Status
 
 A background process can be queued, processing, paused, cancelled, or none of the above (not started or has completed).

classes/wp-background-process.php

@@ -49,6 +49,13 @@ abstract class WP_Background_Process extends WP_Async_Request {
 	 */
 	protected $cron_interval_identifier;
 
+	/**
+	 * Restrict object instantiation when using unserialize.
+	 *
+	 * @var bool|array
+	 */
+	protected $allowed_batch_data_classes = true;
+
 	/**
 	 * The status set when process is cancelling.
 	 *
@@ -65,10 +72,26 @@ abstract class WP_Background_Process extends WP_Async_Request {
 
 	/**
 	 * Initiate new background process.
+	 *
+	 * @param bool|array $allowed_batch_data_classes Optional. Array of class names that can be unserialized. Default true (any class).
 	 */
-	public function __construct() {
+	public function __construct( $allowed_batch_data_classes = true ) {
 		parent::__construct();
 
+		if ( empty( $allowed_batch_data_classes ) && false !== $allowed_batch_data_classes ) {
+			$allowed_batch_data_classes = true;
+		}
+
+		if ( ! is_bool( $allowed_batch_data_classes ) && ! is_array( $allowed_batch_data_classes ) ) {
+			$allowed_batch_data_classes = true;
+		}
+
+		// If allowed_batch_data_classes property set in subclass,
+		// only apply override if not allowing any class.
+		if ( true === $this->allowed_batch_data_classes || true !== $allowed_batch_data_classes ) {
+			$this->allowed_batch_data_classes = $allowed_batch_data_classes;
+		}
+
 		$this->cron_hook_identifier     = $this->identifier . '_cron';
 		$this->cron_interval_identifier = $this->identifier . '_cron_interval';
 
@@ -459,11 +482,13 @@ public function get_batches( $limit = 0 ) {
 		$batches = array();
 
 		if ( ! empty( $items ) ) {
+			$allowed_classes = $this->allowed_batch_data_classes;
+
 			$batches = array_map(
-				static function ( $item ) use ( $column, $value_column ) {
+				static function ( $item ) use ( $column, $value_column, $allowed_classes ) {
 					$batch       = new stdClass();
 					$batch->key  = $item->{$column};
-					$batch->data = maybe_unserialize( $item->{$value_column} );
+					$batch->data = static::maybe_unserialize( $item->{$value_column}, $allowed_classes );
 
 					return $batch;
 				},
@@ -722,4 +747,25 @@ public function cancel_process() {
 	 * @return mixed
 	 */
 	abstract protected function task( $item );
+
+	/**
+	 * Maybe unserialize data, but not if an object.
+	 *
+	 * @param mixed      $data            Data to be unserialized.
+	 * @param bool|array $allowed_classes Array of class names that can be unserialized.
+	 *
+	 * @return mixed
+	 */
+	protected static function maybe_unserialize( $data, $allowed_classes ) {
+		if ( is_serialized( $data ) ) {
+			$options = array();
+			if ( is_bool( $allowed_classes ) || is_array( $allowed_classes ) ) {
+				$options['allowed_classes'] = $allowed_classes;
+			}
+
+			return @unserialize( $data, $options ); // @phpcs:ignore
+		}
+
+		return $data;
+	}
 }

composer.json

@@ -3,7 +3,7 @@
   "description": "WP Background Processing can be used to fire off non-blocking asynchronous requests or as a background processing tool, allowing you to queue tasks.",
   "type": "library",
   "require": {
-    "php": ">=5.6"
+    "php": ">=7.0"
   },
   "suggest": {
     "coenjacobs/mozart": "Easily wrap this library with your own prefix, to prevent collisions when multiple plugins use this library"

tests/Test_WP_Background_Process.php

@@ -5,6 +5,8 @@
  * @package WP-Background-Processing
  */
 
+ require_once __DIR__ . '/fixtures/Test_Batch_Data.php';
+
 use PHPUnit\Framework\MockObject\MockObject;
 
 /**
@@ -51,6 +53,25 @@ private function getWPBPProperty( string $name ) {
 		return $property->getValue( $this->wpbp );
 	}
 
+	/**
+	 * Set a property value on WPBP regardless of accessibility.
+	 *
+	 * @param string $name
+	 * @param mixed  $value
+	 *
+	 * @return mixed
+	 */
+	private function setWPBPProperty( string $name, $value ) {
+		try {
+			$property = new ReflectionProperty( 'WP_Background_Process', $name );
+		} catch ( Exception $e ) {
+			return new WP_Error( $e->getCode(), $e->getMessage() );
+		}
+		$property->setAccessible( true );
+
+		return $property->setValue( $this->wpbp, $value );
+	}
+
 	/**
 	 * Execute a method of WPBP regardless of accessibility.
 	 *
@@ -178,6 +199,41 @@ public function test_get_batch() {
 		$this->assertInstanceOf( 'stdClass', $second_batch );
 		$this->assertNotEquals( $first_batch, $second_batch, '2nd batch returned as 1st deleted' );
 		$this->assertEquals( array( 'more wibble' ), $second_batch->data );
+
+		// Tests using a custom class for the $item.
+		$this->wpbp->delete( $second_batch->key );
+		$batch_data_object = new Test_Batch_Data();
+		$this->wpbp->push_to_queue( $batch_data_object );
+		$this->assertNotEmpty( $this->getWPBPProperty( 'data' ) );
+		$this->assertEquals( array( $batch_data_object ), $this->getWPBPProperty( 'data' ) );
+		$this->wpbp->save();
+		$third_batch = $this->executeWPBPMethod( 'get_batch' );
+		$this->assertCount( 1,  $third_batch->data );
+		$this->assertInstanceOf( Test_Batch_Data::class, $third_batch->data[0] );
+
+		// Explicitly set allowed classes to Test_Batch_Data.
+		$this->setWPBPProperty( 'allowed_batch_data_classes', array( Test_Batch_Data::class ) );
+		$third_batch = $this->executeWPBPMethod( 'get_batch' );
+		$this->assertCount( 1,  $third_batch->data );
+		$this->assertInstanceOf( Test_Batch_Data::class, $third_batch->data[0] );
+
+		// Allow a different class.
+		$this->setWPBPProperty( 'allowed_batch_data_classes', array( stdClass::class ) );
+		$third_batch = $this->executeWPBPMethod( 'get_batch' );
+		$this->assertCount( 1,  $third_batch->data );
+		$this->assertInstanceOf( __PHP_Incomplete_Class::class, $third_batch->data[0] );
+
+		// Disallow all classes.
+		$this->setWPBPProperty( 'allowed_batch_data_classes', false );
+		$third_batch = $this->executeWPBPMethod( 'get_batch' );
+		$this->assertCount( 1,  $third_batch->data );
+		$this->assertInstanceOf( __PHP_Incomplete_Class::class, $third_batch->data[0] );
+
+		// Allow everything.
+		$this->setWPBPProperty( 'allowed_batch_data_classes', true );
+		$third_batch = $this->executeWPBPMethod( 'get_batch' );
+		$this->assertCount( 1,  $third_batch->data );
+		$this->assertInstanceOf( Test_Batch_Data::class, $third_batch->data[0] );
 	}
 
 	/**

tests/fixtures/Test_Batch_Data.php

@@ -0,0 +1,5 @@
+<?php
+
+class Test_Batch_Data {
+    public $prop = "value";
+}