From 652253c839437defad78a94b9b8d9c96dc6ac9f8 Mon Sep 17 00:00:00 2001 From: Stefan Giehl Date: Thu, 11 Oct 2018 01:00:43 +0200 Subject: [PATCH] Implements wrapper method for a more secure unserialize with PHP 7 (#13285) * Implements wrapper method for a more secure unserialize * run AllTests on PHP 7 * trigger a debug message if unserialize fails on PHP 7 + tests * Add string to deserialize to debug log. --- .travis.yml | 4 +-- core/Common.php | 27 ++++++++++++++++ core/Concurrency/DistributedList.php | 3 +- core/CronArchive.php | 4 +-- core/DataAccess/ArchiveSelector.php | 2 +- core/DataTable.php | 6 +++- core/DataTable/Renderer/Php.php | 3 +- core/Scheduler/Timetable.php | 3 +- core/Tracker/Visit/ReferrerSpamFilter.php | 2 +- core/Updates/3.0.0-b1.php | 2 +- plugins/Annotations/AnnotationList.php | 3 +- plugins/Referrers/SearchEngine.php | 2 +- plugins/Referrers/Social.php | 3 +- plugins/UserCountryMap/Controller.php | 4 +-- tests/PHPUnit/Unit/CommonTest.php | 39 +++++++++++++++++++++++ 15 files changed, 91 insertions(+), 16 deletions(-) diff --git a/.travis.yml b/.travis.yml index a9618ccc3bc..2dd4972567e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -63,10 +63,10 @@ matrix: sudo: false addons: false # All tests after another - - php: 5.6 + - php: 7 env: TEST_SUITE=AllTests MYSQL_ADAPTER=MYSQLI ALLTEST_EXTRA_OPTIONS="--run-first-half-only" sudo: required - - php: 5.6 + - php: 7 env: TEST_SUITE=AllTests MYSQL_ADAPTER=MYSQLI ALLTEST_EXTRA_OPTIONS="--run-second-half-only" sudo: required # UITests use a specific version because the default 5.5 (== 5.5.38) is missing FreeType support diff --git a/core/Common.php b/core/Common.php index c617f6ce000..4c20559e4aa 100644 --- a/core/Common.php +++ b/core/Common.php @@ -256,6 +256,33 @@ public static function mb_strtoupper($string) return $string; } + /** + * Secure wrapper for unserialize, which by default disallows unserializing classes + * + * @param string $string String to unserialize + * @param array $allowedClasses Class names that should be allowed to unserialize + * + * @return mixed + */ + public static function safe_unserialize($string, $allowedClasses = []) + { + if (PHP_MAJOR_VERSION >= 7) { + try { + return unserialize($string, ['allowed_classes' => empty($allowedClasses) ? false : $allowedClasses]); + } catch (\Throwable $e) { + $logger = StaticContainer::get('Psr\Log\LoggerInterface'); + $logger->debug('Unable to unserialize a string: {message} (string = {string})', [ + 'message' => $e->getMessage(), + 'backtrace' => $e->getTraceAsString(), + 'string' => $string, + ]); + return false; + } + } + + return @unserialize($string); + } + /* * Escaping input */ diff --git a/core/Concurrency/DistributedList.php b/core/Concurrency/DistributedList.php index 572b25d6ff0..a01f27c1d8c 100644 --- a/core/Concurrency/DistributedList.php +++ b/core/Concurrency/DistributedList.php @@ -7,6 +7,7 @@ */ namespace Piwik\Concurrency; +use Piwik\Common; use Piwik\Container\StaticContainer; use Piwik\Option; use Psr\Log\LoggerInterface; @@ -160,7 +161,7 @@ protected function getListOptionValue() $result = array(); if ($array - && ($array = unserialize($array)) + && ($array = Common::safe_unserialize($array)) && count($array) ) { $result = $array; diff --git a/core/CronArchive.php b/core/CronArchive.php index 195ce93a7e7..49df8bab616 100644 --- a/core/CronArchive.php +++ b/core/CronArchive.php @@ -864,7 +864,7 @@ protected function processArchiveDays($idSite, $lastTimestampWebsiteProcessedDay } $content = $this->request($url); - $daysResponse = @unserialize($content); + $daysResponse = Common::safe_unserialize($content); if (empty($content) || !is_array($daysResponse) @@ -1015,7 +1015,7 @@ private function archiveReportsFor($idSite, $period, $date, $archiveSegments, Ti $success = $success && $this->checkResponse($content, $url); if ($noSegmentUrl == $url && $success) { - $stats = @unserialize($content); + $stats = Common::safe_unserialize($content); if (!is_array($stats)) { $this->logError("Error unserializing the following response from $url: " . $content); diff --git a/core/DataAccess/ArchiveSelector.php b/core/DataAccess/ArchiveSelector.php index a944f230577..54024b88ff3 100644 --- a/core/DataAccess/ArchiveSelector.php +++ b/core/DataAccess/ArchiveSelector.php @@ -316,7 +316,7 @@ public static function getArchiveData($archiveIds, $recordNames, $archiveDataTyp private static function moveChunkRowToRows(&$rows, $row, Chunk $chunk, $loadAllSubtables, $idSubtable) { // $blobs = array([subtableID] = [blob of subtableId]) - $blobs = unserialize($row['value']); + $blobs = Common::safe_unserialize($row['value']); if (!is_array($blobs)) { return; diff --git a/core/DataTable.php b/core/DataTable.php index 22b09b97ee1..b14281938c9 100644 --- a/core/DataTable.php +++ b/core/DataTable.php @@ -1336,7 +1336,11 @@ public function getSerialized($maximumRowsInDataTable = null, private function unserializeRows($serialized) { $serialized = str_replace(self::$previousRowClasses, self::$rowClassToUseForUnserialize, $serialized); - $rows = unserialize($serialized); + $rows = Common::safe_unserialize($serialized, [ + Row::class, + DataTableSummaryRow::class, + \Piwik_DataTable_SerializedRow::class + ]); if ($rows === false) { throw new Exception("The unserialization has failed!"); diff --git a/core/DataTable/Renderer/Php.php b/core/DataTable/Renderer/Php.php index 9a121bc6283..540bfb9a02c 100644 --- a/core/DataTable/Renderer/Php.php +++ b/core/DataTable/Renderer/Php.php @@ -9,6 +9,7 @@ namespace Piwik\DataTable\Renderer; use Exception; +use Piwik\Common; use Piwik\DataTable\Renderer; use Piwik\DataTable\Simple; use Piwik\DataTable; @@ -77,7 +78,7 @@ public function render($dataTable = null) if ($this->prettyDisplay) { if (!is_array($toReturn)) { - $toReturn = unserialize($toReturn); + $toReturn = Common::safe_unserialize($toReturn); } $toReturn = "
" . var_export($toReturn, true) . "
"; } diff --git a/core/Scheduler/Timetable.php b/core/Scheduler/Timetable.php index 93980b7ac0f..2970dc83ac1 100644 --- a/core/Scheduler/Timetable.php +++ b/core/Scheduler/Timetable.php @@ -9,6 +9,7 @@ namespace Piwik\Scheduler; +use Piwik\Common; use Piwik\Option; use Piwik\Date; @@ -24,7 +25,7 @@ class Timetable public function __construct() { $optionData = Option::get(self::TIMETABLE_OPTION_STRING); - $unserializedTimetable = @unserialize($optionData); + $unserializedTimetable = Common::safe_unserialize($optionData); $this->timetable = $unserializedTimetable === false ? array() : $unserializedTimetable; } diff --git a/core/Tracker/Visit/ReferrerSpamFilter.php b/core/Tracker/Visit/ReferrerSpamFilter.php index 3bca5bb6670..21d52160680 100644 --- a/core/Tracker/Visit/ReferrerSpamFilter.php +++ b/core/Tracker/Visit/ReferrerSpamFilter.php @@ -75,7 +75,7 @@ private function loadSpammerList() $list = Option::get(self::OPTION_STORAGE_NAME); if ($list) { - $this->spammerList = unserialize($list); + $this->spammerList = Common::safe_unserialize($list); } else { // Fallback to reading the bundled list $file = PIWIK_VENDOR_PATH . '/matomo/referrer-spam-blacklist/spammers.txt'; diff --git a/core/Updates/3.0.0-b1.php b/core/Updates/3.0.0-b1.php index a1561dfc164..0e3b9f2ac16 100644 --- a/core/Updates/3.0.0-b1.php +++ b/core/Updates/3.0.0-b1.php @@ -134,7 +134,7 @@ private function getPluginSettingsMigrations($queries) foreach ($options as $option) { $name = $option['option_name']; $pluginName = str_replace(array('Plugin_', '_Settings'), '', $name); - $values = @unserialize($option['option_value']); + $values = Common::safe_unserialize($option['option_value']); if (empty($values)) { continue; diff --git a/plugins/Annotations/AnnotationList.php b/plugins/Annotations/AnnotationList.php index 823c7bf1251..845cbfe9ae7 100644 --- a/plugins/Annotations/AnnotationList.php +++ b/plugins/Annotations/AnnotationList.php @@ -9,6 +9,7 @@ namespace Piwik\Plugins\Annotations; use Exception; +use Piwik\Common; use Piwik\Date; use Piwik\Option; use Piwik\Piwik; @@ -330,7 +331,7 @@ private function getAnnotationsForSite() $serialized = Option::get($optionName); if ($serialized !== false) { - $result[$id] = @unserialize($serialized); + $result[$id] = Common::safe_unserialize($serialized); if (empty($result[$id])) { // in case unserialize failed $result[$id] = array(); diff --git a/plugins/Referrers/SearchEngine.php b/plugins/Referrers/SearchEngine.php index 8f7d1742811..28498c73c61 100644 --- a/plugins/Referrers/SearchEngine.php +++ b/plugins/Referrers/SearchEngine.php @@ -54,7 +54,7 @@ private function loadDefinitions() $list = Option::get(self::OPTION_STORAGE_NAME); if ($list) { - $this->definitionList = unserialize(base64_decode($list)); + $this->definitionList = Common::safe_unserialize(base64_decode($list)); } else { // Fallback to reading the bundled list $yml = file_get_contents(PIWIK_INCLUDE_PATH . self::DEFINITION_FILE); diff --git a/plugins/Referrers/Social.php b/plugins/Referrers/Social.php index b10ae61294d..f7a0cca7db6 100644 --- a/plugins/Referrers/Social.php +++ b/plugins/Referrers/Social.php @@ -8,6 +8,7 @@ */ namespace Piwik\Plugins\Referrers; use Piwik\Cache; +use Piwik\Common; use Piwik\Option; use Piwik\Piwik; use Piwik\Singleton; @@ -51,7 +52,7 @@ private function loadDefinitions() $list = Option::get(self::OPTION_STORAGE_NAME); if ($list) { - $this->definitionList = unserialize(base64_decode($list)); + $this->definitionList = Common::safe_unserialize(base64_decode($list)); } else { // Fallback to reading the bundled list $yml = file_get_contents(PIWIK_INCLUDE_PATH . self::DEFINITION_FILE); diff --git a/plugins/UserCountryMap/Controller.php b/plugins/UserCountryMap/Controller.php index c78aa3f9713..c41ae83e666 100644 --- a/plugins/UserCountryMap/Controller.php +++ b/plugins/UserCountryMap/Controller.php @@ -75,7 +75,7 @@ public function visitorMap($fetch = false, $segmentOverride = false) . '&filter_limit=-1' ); $config = array(); - $config['visitsSummary'] = unserialize($request->process()); + $config['visitsSummary'] = Common::safe_unserialize($request->process()); $config['countryDataUrl'] = $this->_report('UserCountry', 'getCountry', $this->idSite, $period, $date, $token_auth, false, $segment); $config['regionDataUrl'] = $this->_report('UserCountry', 'getRegion', @@ -277,7 +277,7 @@ private function getMetrics($idSite, $period, $date, $token_auth) . '&token_auth=' . $token_auth . '&filter_limit=-1' ); - $metaData = unserialize($request->process()); + $metaData = Common::safe_unserialize($request->process()); $metrics = array(); if (!empty($metaData[0]['metrics']) && is_array($metaData[0]['metrics'])) { diff --git a/tests/PHPUnit/Unit/CommonTest.php b/tests/PHPUnit/Unit/CommonTest.php index 08931b6bfe2..7466a742bcb 100644 --- a/tests/PHPUnit/Unit/CommonTest.php +++ b/tests/PHPUnit/Unit/CommonTest.php @@ -10,10 +10,13 @@ use Exception; use PHPUnit_Framework_TestCase; +use Piwik\Application\Environment; use Piwik\Common; use Piwik\Container\StaticContainer; use Piwik\Filesystem; use Piwik\Intl\Data\Provider\RegionDataProvider; +use Piwik\Log; +use Piwik\Tests\Framework\Mock\FakeLogger; /** * @backupGlobals enabled @@ -279,6 +282,42 @@ public function testIsValidFilenameNotValidValues() } } + public function testSafeUnserialize() + { + if (PHP_MAJOR_VERSION < 7) { + $this->markTestSkipped('secure unserialize tests are for PHP7 only'); + return; + } + + // should unserialize an allowed class + $this->assertTrue(Common::safe_unserialize('O:12:"Piwik\Common":0:{}', ['Piwik\Common']) instanceof Common); + + // not allowed classed should result in an incomplete class + $this->assertTrue(Common::safe_unserialize('O:12:"Piwik\Common":0:{}') instanceof \__PHP_Incomplete_Class); + + // strings not unserializable should return false and trigger a debug log + $logger = $this->createFakeLogger(); + $this->assertFalse(Common::safe_unserialize('{1:somebroken}')); + $this->assertContains('Unable to unserialize a string: unserialize(): Error at offset 0 of 14 bytes', $logger->output); + } + + private function createFakeLogger() + { + $logger = new FakeLogger(); + + $newEnv = new Environment('test', array( + 'Psr\Log\LoggerInterface' => $logger, + 'Tests.log.allowAllHandlers' => true, + )); + $newEnv->init(); + + $newMonologLogger = $newEnv->getContainer()->make('Psr\Log\LoggerInterface'); + $oldLogger = new Log($newMonologLogger); + Log::setSingletonInstance($oldLogger); + + return $logger; + } + /** * Dataprovider for testGetBrowserLanguage */