Implements wrapper method for a more secure unserialize with PHP 7 (m…

…atomo-org#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.
InfinityVoid · Oct 11, 2018 · 652253c · 652253c
1 parent 3077327
commit 652253c
Show file tree

Hide file tree

Showing 15 changed files with 91 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 = "<pre>" . var_export($toReturn, true) . "</pre>";
         }

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
      */