Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion classes/Visualizer/Gutenberg/Block.php
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ public function get_visualizer_data( $post ) {
$data['visualizer-settings'] = apply_filters( Visualizer_Plugin::FILTER_GET_CHART_SETTINGS, $data['visualizer-settings'], $post_id, $data['visualizer-chart-type'] );

// handle data filter hooks
$data['visualizer-data'] = apply_filters( Visualizer_Plugin::FILTER_GET_CHART_DATA, unserialize( html_entity_decode( get_the_content( $post_id ) ) ), $post_id, $data['visualizer-chart-type'] );
$data['visualizer-data'] = apply_filters( Visualizer_Plugin::FILTER_GET_CHART_DATA, Visualizer_Module::decode_content( html_entity_decode( get_the_content( $post_id ) ) ), $post_id, $data['visualizer-chart-type'] );

Comment on lines 309 to 311
// we are going to format only for tabular charts, because we are not sure of the effect on others.
// this is to solve the case where boolean data shows up as all-ticks on gutenberg.
Expand Down
15 changes: 14 additions & 1 deletion classes/Visualizer/Module.php
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,19 @@ final public static function get_features_for_license( $plan ) {
}
}

/**
* Safely unserialize chart/source content, blocking PHP object injection.
*
* Single guarded chokepoint shared by every unserialize() sink so the
* allowed_classes guard cannot be dropped from one call site independently.
*
* @param mixed $content The serialized content (only strings are decoded).
* @return mixed The decoded value (array for valid chart data), or false.
*/
public static function decode_content( $content ) {
return is_string( $content ) ? unserialize( $content, array( 'allowed_classes' => false ) ) : false;
}

/**
* Gets the chart content after common manipulations.
*/
Expand All @@ -793,7 +806,7 @@ function ( $matches ) {
},
$post_content
);
$data = unserialize( $post_content );
$data = self::decode_content( $post_content );
$altered = array();
if ( ! empty( $data ) ) {
foreach ( $data as $index => $array ) {
Expand Down
4 changes: 2 additions & 2 deletions classes/Visualizer/Module/Chart.php
Original file line number Diff line number Diff line change
Expand Up @@ -1336,8 +1336,8 @@ public function uploadData() {
if ( $source->fetch() ) {
$content = $source->getData( get_post_meta( $chart_id, Visualizer_Plugin::CF_EDITABLE_TABLE, true ) );
$populate = true;
if ( is_string( $content ) && is_array( unserialize( $content ) ) ) {
$json = unserialize( $content );
$json = self::decode_content( $content );
if ( is_array( $json ) ) {
// if source exists, so should data. if source exists but data is blank, do not populate the chart.
// if we populate the data even if it is empty, the chart will show "Table has no columns".
if ( array_key_exists( 'source', $json ) && ! empty( $json['source'] ) && ( ! array_key_exists( 'data', $json ) || empty( $json['data'] ) ) ) {
Expand Down
7 changes: 5 additions & 2 deletions classes/Visualizer/Module/Utility.php
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,10 @@ private static function apply_chartjs_palette( array $settings, string $type, ar
// fall through.
case 'pie':
$chart = get_post( $chart_id );
$data = $chart instanceof WP_Post ? maybe_unserialize( $chart->post_content ) : array();
$raw = $chart instanceof WP_Post ? $chart->post_content : array();
// Object-injection-safe replacement for maybe_unserialize(); keeps
// the "return as-is when not serialized" behavior.
$data = is_serialized( $raw ) ? self::decode_content( $raw ) : $raw;
$name = 'slices';
$max = is_array( $data ) ? count( $data ) : $count;
// fall through.
Expand Down Expand Up @@ -434,7 +437,7 @@ private static function set_defaults_chartjs( $chart, $post_status ) {
case 'polarArea':
// fall through.
case 'pie':
$data = unserialize( $chart->post_content );
$data = self::decode_content( $chart->post_content );
$name = 'slices';
$max = count( $data );
// fall through.
Expand Down
2 changes: 1 addition & 1 deletion classes/Visualizer/Source/Csv/Remote.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ private function _repopulate( $chart_id ) {
// if filename is empty, extract it from chart content
if ( empty( $this->_filename ) ) {
$chart = get_post( $chart_id );
$data = unserialize( html_entity_decode( $chart->post_content ) );
$data = Visualizer_Module::decode_content( html_entity_decode( $chart->post_content ) );
if ( ! isset( $data['source'] ) ) {
return false;
}
Expand Down
161 changes: 161 additions & 0 deletions tests/test-security-object-injection.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
<?php
/**
* Regression test for PHP Object Injection via unsafe unserialize().
*
* Chart post_content is unserialized in several places. With the
* array( 'allowed_classes' => false ) guard, a serialized object in the
* content must NOT be instantiated. The canary below flips a static flag
* from its __wakeup(); if the guard is removed, unserialize() instantiates
* the canary, __wakeup() fires, and the assertion fails.
*
* @package Visualizer
* @subpackage Tests
*/

if ( ! class_exists( 'Visualizer_POI_Canary' ) ) {
/**
* Canary "gadget": records whether it was ever instantiated by unserialize().
*/
class Visualizer_POI_Canary {
/**
* Flag to track whether the canary was instantiated.
*
* @var bool
*/
public static $awoke = false;

/**
* Records that this object was instantiated by unserialize().
*/
public function __wakeup() {
self::$awoke = true;
}
}
}

/**
* Security tests for object injection vulnerabilities.
*/
class Test_Security_Object_Injection extends WP_UnitTestCase {

/**
* Serialized payload carrying a Visualizer_POI_Canary object.
*
* @return string
*/
private function object_payload() {
Visualizer_POI_Canary::$awoke = false;
return serialize( new Visualizer_POI_Canary() );
}

/**
* Visualizer_Module::get_chart_data() must not instantiate objects from content.
*/
public function test_get_chart_data_does_not_instantiate_objects() {
$chart = new stdClass();
$chart->post_content = $this->object_payload();

try {
Visualizer_Module::get_chart_data( $chart, 'line', false );
} catch ( \Throwable $e ) {
// Downstream handling of the neutralized payload is irrelevant here;
// we only assert that no object was instantiated during unserialize().
}

$this->assertFalse(
Visualizer_POI_Canary::$awoke,
'unserialize() must not instantiate objects from chart post_content.'
);
}

/**
* The remote CSV source (_repopulate) must not instantiate objects from content.
*/
public function test_remote_csv_source_does_not_instantiate_objects() {
$chart_id = self::factory()->post->create(
array(
'post_type' => Visualizer_Plugin::CPT_VISUALIZER,
'post_content' => wp_slash( $this->object_payload() ),
)
);
Comment on lines +77 to +82

$source = new Visualizer_Source_Csv_Remote();
$method = new ReflectionMethod( 'Visualizer_Source_Csv_Remote', '_repopulate' );
$method->setAccessible( true );

try {
$method->invoke( $source, $chart_id );
} catch ( \Throwable $e ) {
// The neutralized payload has no 'source' key and returns false; only the canary matters.
}

$this->assertFalse(
Visualizer_POI_Canary::$awoke,
'Remote CSV source must not instantiate objects from chart post_content.'
);
}

/**
* The shared decode_content() chokepoint must not instantiate objects.
*
* The business/scheduled JSON sink (updateBusinessJson) reads
* serialize()'d source data via getData() and decodes it through
* Visualizer_Module::decode_content(). This feeds a serialized canary the
* same way and asserts the object is never instantiated.
*/
public function test_decode_content_does_not_instantiate_objects() {
Visualizer_POI_Canary::$awoke = false;

// Reproduce the content the sink reads: serialize() of the source data
// (getData()), here carrying a canary object.
$source = new Visualizer_Source_Json( array( 'url' => '', 'root' => '', 'paging' => '' ) );
$data_prop = new ReflectionProperty( 'Visualizer_Source', '_data' );
$data_prop->setAccessible( true );
$data_prop->setValue( $source, array( new Visualizer_POI_Canary() ) );
$content = $source->getData();

$result = Visualizer_Module::decode_content( $content );

$this->assertFalse(
Visualizer_POI_Canary::$awoke,
'decode_content() must not instantiate objects from source content.'
);
$this->assertIsArray(
$result,
'decode_content() should still return the (neutralized) array.'
);
}

/**
* The Utility pie/polarArea render palette call site must not instantiate objects.
*
* Drives the real sink (Visualizer_Module_Utility::apply_chartjs_palette),
* which previously used maybe_unserialize() on post_content, with a chart
* whose content is a serialized canary. Reverting that call site to an
* unrestricted (maybe_)unserialize() would fail this test.
*/
public function test_utility_pie_palette_call_site_is_guarded() {
Visualizer_POI_Canary::$awoke = false;

$chart_id = self::factory()->post->create(
array(
'post_type' => Visualizer_Plugin::CPT_VISUALIZER,
'post_content' => wp_slash( serialize( array( new Visualizer_POI_Canary() ) ) ),
)
);

$method = new ReflectionMethod( 'Visualizer_Module_Utility', 'apply_chartjs_palette' );
$method->setAccessible( true );

try {
$method->invoke( null, array(), 'pie', array(), $chart_id );
} catch ( \Throwable $e ) {
// Only the canary matters here.
}

$this->assertFalse(
Visualizer_POI_Canary::$awoke,
'Utility pie palette call site must not instantiate objects from post_content.'
);
}
}
Loading