[!!!][SECURITY] Mitigate potential cache flooding 25/49925/2
authorHelmut Hummel <info@helhum.io>
Tue, 13 Sep 2016 09:53:12 +0000 (11:53 +0200)
committerOliver Hader <oliver.hader@typo3.org>
Tue, 13 Sep 2016 09:53:18 +0000 (11:53 +0200)
Bind cHash to the page id it was generated for, to avoid
an attacker to be able to call multiple pages with the same
cHash arguments and thus create unnecessary cache entries.

We now add the id argument to the cHash calculation, but only
if there are other arguments in the URI which would require a cHash.
This avoids multiple cache entries for one page
(one with and one without cHash).

We ignore other core parameters like "type" and "MP", because the possibility
to create unnecessary cache entries by manipulating these is very limited
and thus an attack not feasible.

Adapted tests to show our new expectations for cHash calculations.

The new behavior is default for new installations, but not for on for existing
installations, as an update would break the site with a high probability.

By adding the configuration option, we'll give users the chance to
pull the trigger once everything is prepared, but still get other
security issues fixed with the release.

Resolves: #76462
Releases: master, 8.3, 7.6, 6.2
Security-Commit: d97bb1cc1aea4a616db0665b60366a1a23f5ee50
Security-Bulletins: TYPO3-CORE-SA-2016-020, 021
Change-Id: Id8801b7b7d86b65ecaa2ca5e9bb14c27d4268aee
Reviewed-on: https://review.typo3.org/49925
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
typo3/sysext/core/Classes/Core/Bootstrap.php
typo3/sysext/core/Configuration/DefaultConfiguration.php
typo3/sysext/core/Configuration/FactoryConfiguration.php
typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php
typo3/sysext/frontend/Classes/Page/CacheHashCalculator.php
typo3/sysext/frontend/Tests/Unit/Page/CacheHashCalculatorTest.php
typo3/sysext/lang/locallang_core.xlf
typo3/sysext/reports/Classes/Report/Status/SecurityStatus.php
typo3/sysext/reports/Resources/Private/Language/locallang_reports.xlf

index c410eff..94ee4af 100644 (file)
@@ -577,7 +577,8 @@ class Bootstrap
         $GLOBALS['TYPO3_CONF_VARS']['FE']['cacheHash'] = [
             'cachedParametersWhiteList' => GeneralUtility::trimExplode(',', $GLOBALS['TYPO3_CONF_VARS']['FE']['cHashOnlyForParameters'], true),
             'excludedParameters' => GeneralUtility::trimExplode(',', $GLOBALS['TYPO3_CONF_VARS']['FE']['cHashExcludedParameters'], true),
-            'requireCacheHashPresenceParameters' => GeneralUtility::trimExplode(',', $GLOBALS['TYPO3_CONF_VARS']['FE']['cHashRequiredParameters'], true)
+            'requireCacheHashPresenceParameters' => GeneralUtility::trimExplode(',', $GLOBALS['TYPO3_CONF_VARS']['FE']['cHashRequiredParameters'], true),
+            'includePageId' => $GLOBALS['TYPO3_CONF_VARS']['FE']['cHashIncludePageId']
         ];
         if (trim($GLOBALS['TYPO3_CONF_VARS']['FE']['cHashExcludedParametersIfEmpty']) === '*') {
             $GLOBALS['TYPO3_CONF_VARS']['FE']['cacheHash']['excludeAllEmptyParameters'] = true;
index c28252d..862b470 100644 (file)
@@ -1122,6 +1122,7 @@ return [
         'eID_include' => [],        // Array of key/value pairs where key is "tx_[ext]_[optional suffix]" and value is relative filename of class to include. Key is used as "?eID=" for \TYPO3\CMS\Frontend\Http\RequestHandlerRequestHandler to include the code file which renders the page from that point. (Useful for functionality that requires a low initialization footprint, eg. frontend ajax applications)
         'disableNoCacheParameter' => false,        // Boolean: If set, the no_cache request parameter will become ineffective. This is currently still an experimental feature and will require a website only with plugins that don't use this parameter. However, using "&amp;no_cache=1" should be avoided anyway because there are better ways to disable caching for a certain part of the website (see COA_INT/USER_INT documentation in TSref).
         'cacheHash' => [],        // Array: Processed values of the cHash* parameters, handled by core bootstrap internally
+        'cHashIncludePageId' => false,        // Boolean: If enabled the cHash calculation is bound to the current page ID. This is recommended to avoid generation of not required page cache entries. Changing this value will void all links that have a cHash argument, as the cHash will change.
         'cHashExcludedParameters' => 'L, pk_campaign, pk_kwd, utm_source, utm_medium, utm_campaign, utm_term, utm_content',        // String: The the given parameters will be ignored in the cHash calculation. Example: L,tx_search_pi1[query]
         'cHashOnlyForParameters' => '',        // String: Only the given parameters will be evaluated in the cHash calculation. Example: tx_news_pi1[uid]
         'cHashRequiredParameters' => '',        // Optional: Configure Parameters that require a cHash. If no cHash is given but one of the parameters are set, then TYPO3 triggers the configured cHash Error behaviour
index 4c90eea..98f4c38 100644 (file)
@@ -32,6 +32,7 @@ return [
     ],
     'FE' => [
         'loginSecurityLevel' => 'rsa',
+        'cHashIncludePageId' => true,
     ],
     'GFX' => [
         'jpg_quality' => '80',
index 640962d..781704c 100644 (file)
@@ -5931,8 +5931,7 @@ class ContentObjectRenderer
                         $addQueryParams = '';
                     }
                     if ($conf['useCacheHash']) {
-                        // Mind the order below! See http://forge.typo3.org/issues/17070
-                        $params = $tsfe->linkVars . $addQueryParams;
+                        $params = $tsfe->linkVars . $addQueryParams . '&id=' . $linkDetails['pageuid'];
                         if (trim($params, '& ') != '') {
                             /** @var $cacheHash CacheHashCalculator */
                             $cacheHash = GeneralUtility::makeInstance(CacheHashCalculator::class);
index 02c8fbd..28cdc96 100644 (file)
@@ -45,6 +45,11 @@ class CacheHashCalculator implements \TYPO3\CMS\Core\SingletonInterface
     protected $excludeAllEmptyParameters = false;
 
     /**
+     * @var bool
+     */
+    protected $includePageId = false;
+
+    /**
      * Initialise class properties by using the relevant TYPO3 configuration
      */
     public function __construct()
@@ -121,6 +126,12 @@ class CacheHashCalculator implements \TYPO3\CMS\Core\SingletonInterface
             $relevantParameters[$parameterName] = $parameterValue;
         }
         if (!empty($relevantParameters)) {
+            if ($this->includePageId) {
+                if (empty($parameters['id'])) {
+                    throw new \RuntimeException('ID parameter needs to be passed for the cHash calculation! As a temporary not recommended workaround, you can set $GLOBALS[\'TYPO3_CONF_VARS\'][\'FE\'][\'cHashIncludePageId\'] to false to avoid this error.', 1467983513);
+                }
+                $relevantParameters['id'] = $parameters['id'];
+            }
             // Finish and sort parameters array by keys:
             $relevantParameters['encryptionKey'] = $GLOBALS['TYPO3_CONF_VARS']['SYS']['encryptionKey'];
             ksort($relevantParameters);
@@ -218,9 +229,9 @@ class CacheHashCalculator implements \TYPO3\CMS\Core\SingletonInterface
      * Loops through the configuration array and calls the accordant
      * getters with the value.
      *
-     * @param $configuration
+     * @param array $configuration
      */
-    public function setConfiguration($configuration)
+    public function setConfiguration(array $configuration)
     {
         foreach ($configuration as $name => $value) {
             $setterName = 'set' . ucfirst($name);
@@ -239,6 +250,14 @@ class CacheHashCalculator implements \TYPO3\CMS\Core\SingletonInterface
     }
 
     /**
+     * @param bool $includePageId
+     */
+    protected function setIncludePageId($includePageId)
+    {
+        $this->includePageId = $includePageId;
+    }
+
+    /**
      * @param bool $excludeAllEmptyParameters
      */
     protected function setExcludeAllEmptyParameters($excludeAllEmptyParameters)
index 39fce70..fa5f4d7 100644 (file)
@@ -24,16 +24,8 @@ class CacheHashCalculatorTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
      */
     protected $subject;
 
-    /**
-     * @var array
-     */
-    protected $confCache = [];
-
     protected function setUp()
     {
-        $this->confCache = [
-            'encryptionKey' => $GLOBALS['TYPO3_CONF_VARS']['SYS']['encryptionKey']
-        ];
         $GLOBALS['TYPO3_CONF_VARS']['SYS']['encryptionKey'] = 't3lib_cacheHashTest';
         $this->subject = $this->getMockBuilder(\TYPO3\CMS\Frontend\Page\CacheHashCalculator::class)
             ->setMethods(['foo'])
@@ -43,12 +35,13 @@ class CacheHashCalculatorTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
             'cachedParametersWhiteList' => [],
             'requireCacheHashPresenceParameters' => ['req1', 'req2'],
             'excludedParametersIfEmpty' => [],
+            'includePageId' => true,
             'excludeAllEmptyParameters' => false
         ]);
     }
 
     /**
-     * @dataProvider cacheHashCalculationDataprovider
+     * @dataProvider cacheHashCalculationDataProvider
      * @test
      */
     public function cacheHashCalculationWorks($params, $expected)
@@ -59,7 +52,7 @@ class CacheHashCalculatorTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     /**
      * @return array
      */
-    public function cacheHashCalculationDataprovider()
+    public function cacheHashCalculationDataProvider()
     {
         return [
             'Empty parameters should not return a hash' => [
@@ -102,26 +95,30 @@ class CacheHashCalculatorTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
         return [
             'Empty list should be passed through' => ['', []],
             'Simple parameter should be passed through and the encryptionKey should be added' => [
-                'key=v',
-                ['encryptionKey', 'key']
+                'key=v&id=42',
+                ['encryptionKey', 'id', 'key']
             ],
             'Simple parameter should be passed through' => [
-                'key1=v&key2=v',
-                ['encryptionKey', 'key1', 'key2']
+                'key1=v&key2=v&id=42',
+                ['encryptionKey', 'id', 'key1', 'key2']
             ],
             'System and exclude parameters should be omitted' => [
                 'id=1&type=3&exclude1=x&no_cache=1',
                 []
             ],
-            'System and exclude parameters should be omitted, others should stay' => [
+            'System and exclude parameters (except id) should be omitted, others should stay' => [
                 'id=1&type=3&key=x&no_cache=1',
-                ['encryptionKey', 'key']
+                ['encryptionKey', 'id', 'key']
+            ],
+            'System and exclude parameters should be omitted and id is not required to be specified' => [
+                '&type=3&no_cache=1',
+                []
             ]
         ];
     }
 
     /**
-     * @dataProvider canGenerateForParametersDataprovider
+     * @dataProvider canGenerateForParametersDataProvider
      * @test
      */
     public function canGenerateForParameters($params, $expected)
@@ -130,22 +127,32 @@ class CacheHashCalculatorTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     }
 
     /**
+     * @test
+     * @expectedException \RuntimeException
+     * @expectedExceptionCode 1467983513
+     */
+    public function generateForParametersThrowsExceptionWhenIdIsNotSpecified()
+    {
+        $this->subject->generateForParameters('&key=x');
+    }
+
+    /**
      * @return array
      */
-    public function canGenerateForParametersDataprovider()
+    public function canGenerateForParametersDataProvider()
     {
-        $knowHash = '5cfdcf826275558b3613dd51714a0a17';
+        $knowHash = 'fac112f7e662c83c19b57142c3a921f5';
         return [
-            'Empty parameters should not return an hash' => ['', ''],
+            'Empty parameters should not return an hash' => ['&id=42', ''],
             'Querystring has no relevant parameters so we should not have a cacheHash' => ['&exclude1=val', ''],
-            'Querystring has only system parameters so we should not have a cacheHash' => ['id=1&type=val', ''],
-            'Trivial key value combination should generate hash' => ['&key=value', $knowHash],
-            'Only the relevant parts should be taken into account' => ['&key=value&exclude1=val', $knowHash],
-            'Only the relevant parts should be taken into account(exclude2 before key)' => ['&exclude2=val&key=value', $knowHash],
-            'System parameters should not be taken into account' => ['&id=1&key=value', $knowHash],
-            'Admin panel parameters should not be taken into account' => ['&TSFE_ADMIN_PANEL[display]=7&key=value', $knowHash],
-            'Trivial hash for sorted parameters should be right' => ['a=v&b=v', '0f40b089cdad149aea99e9bf4badaa93'],
-            'Parameters should be sorted before  is created' => ['b=v&a=v', '0f40b089cdad149aea99e9bf4badaa93']
+            'Querystring has only system parameters so we should not have a cacheHash' => ['&id=42&type=val', ''],
+            'Trivial key value combination should generate hash' => ['&id=42&key=value', $knowHash],
+            'Only the relevant parts should be taken into account' => ['&id=42&key=value&exclude1=val', $knowHash],
+            'Only the relevant parts should be taken into account(exclude2 before key)' => ['&id=42&exclude2=val&key=value', $knowHash],
+            'System parameters should not be taken into account (except id)' => ['&id=42&type=23&key=value', $knowHash],
+            'Admin panel parameters should not be taken into account' => ['&id=42&TSFE_ADMIN_PANEL[display]=7&key=value', $knowHash],
+            'Trivial hash for sorted parameters should be right' => ['&id=42&a=v&b=v', '52c8a1299e20324f90377c43153c4987'],
+            'Parameters should be sorted before cHash is created' => ['&id=42&b=v&a=v', '52c8a1299e20324f90377c43153c4987']
         ];
     }
 
@@ -176,7 +183,7 @@ class CacheHashCalculatorTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
      * In case the cHashOnlyForParameters is set, other parameters should not
      * incluence the cHash (except the encryption key of course)
      *
-     * @dataProvider canWhitelistParametersDataprovider
+     * @dataProvider canWhitelistParametersDataProvider
      * @test
      */
     public function canWhitelistParameters($params, $expected)
@@ -190,21 +197,21 @@ class CacheHashCalculatorTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     /**
      * @return array
      */
-    public function canWhitelistParametersDataprovider()
+    public function canWhitelistParametersDataProvider()
     {
-        $oneParamHash = 'e2c0f2edf08be18bcff2f4272e11f66b';
-        $twoParamHash = 'f6f08c2e10a97d91b6ec61a6e2ddd0e7';
+        $oneParamHash = 'eae50a13101afd53a9d2c543230eb5bb';
+        $twoParamHash = '701e2d2f1becc9d1b71d327e5cb1c3ed';
         return [
             'Even with the whitelist enabled, empty parameters should not return an hash.' => ['', ''],
-            'Whitelisted parameters should have a hash.' => ['whitep1=value', $oneParamHash],
-            'Blacklisted parameter should not influence hash.' => ['whitep1=value&black=value', $oneParamHash],
-            'Multiple whitelisted parameters should work' => ['&whitep1=value&whitep2=value', $twoParamHash],
-            'The order should not influce the hash.' => ['whitep2=value&black=value&whitep1=value', $twoParamHash]
+            'Whitelisted parameters should have a hash.' => ['&id=42&whitep1=value', $oneParamHash],
+            'Blacklisted parameter should not influence hash.' => ['&id=42&whitep1=value&black=value', $oneParamHash],
+            'Multiple whitelisted parameters should work' => ['&id=42&whitep1=value&whitep2=value', $twoParamHash],
+            'The order should not influce the hash.' => ['&id=42&whitep2=value&black=value&whitep1=value', $twoParamHash]
         ];
     }
 
     /**
-     * @dataProvider canSkipParametersWithEmptyValuesDataprovider
+     * @dataProvider canSkipParametersWithEmptyValuesDataProvider
      * @test
      */
     public function canSkipParametersWithEmptyValues($params, $settings, $expected)
@@ -217,28 +224,28 @@ class CacheHashCalculatorTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     /**
      * @return array
      */
-    public function canSkipParametersWithEmptyValuesDataprovider()
+    public function canSkipParametersWithEmptyValuesDataProvider()
     {
         return [
             'The default configuration does not allow to skip an empty key.' => [
-                'key1=v&key2=&key3=',
+                '&id=42&key1=v&key2=&key3=',
                 ['excludedParametersIfEmpty' => [], 'excludeAllEmptyParameters' => false],
-                ['encryptionKey', 'key1', 'key2', 'key3']
+                ['encryptionKey', 'id', 'key1', 'key2', 'key3']
             ],
             'Due to the empty value, "key2" should be skipped(with equals sign' => [
-                'key1=v&key2=&key3=',
+                '&id=42&key1=v&key2=&key3=',
                 ['excludedParametersIfEmpty' => ['key2'], 'excludeAllEmptyParameters' => false],
-                ['encryptionKey', 'key1', 'key3']
+                ['encryptionKey', 'id', 'key1', 'key3']
             ],
             'Due to the empty value, "key2" should be skipped(without equals sign)' => [
-                'key1=v&key2&key3',
+                '&id=42&key1=v&key2&key3',
                 ['excludedParametersIfEmpty' => ['key2'], 'excludeAllEmptyParameters' => false],
-                ['encryptionKey', 'key1', 'key3']
+                ['encryptionKey', 'id', 'key1', 'key3']
             ],
             'Due to the empty value, "key2" and "key3" should be skipped' => [
-                'key1=v&key2=&key3=',
+                '&id=42&key1=v&key2=&key3=',
                 ['excludedParametersIfEmpty' => [], 'excludeAllEmptyParameters' => true],
-                ['encryptionKey', 'key1']
+                ['encryptionKey', 'id', 'key1']
             ]
         ];
     }
index 4b132aa..1da3c52 100644 (file)
@@ -1116,6 +1116,9 @@ Do you want to refresh it now?</source>
                        <trans-unit id="warning.install_trustedhosts">
                                <source>The trusted hosts pattern check is disabled. Please define the allowed hosts in the [SYS][trustedHostsPattern] section of the Install Tool.</source>
                        </trans-unit>
+                       <trans-unit id="warning.install_cache_flooding">
+                               <source>Cache Flooding Protection is disabled. Please enable [FE][cHashIncludePageId] in the Install Tool.</source>
+                       </trans-unit>
                        <trans-unit id="warning.install_encryption">
                                <source>The encryption key is not set. Set it in the %sBasic Configuration section%s of the Install Tool.</source>
                        </trans-unit>
index e0182f9..c6807e8 100644 (file)
@@ -45,12 +45,29 @@ class SecurityStatus implements StatusProviderInterface
             'encryptionKeyEmpty' => $this->getEncryptionKeyStatus(),
             'fileDenyPattern' => $this->getFileDenyPatternStatus(),
             'htaccessUpload' => $this->getHtaccessUploadStatus(),
-            'saltedpasswords' => $this->getSaltedPasswordsStatus()
+            'saltedpasswords' => $this->getSaltedPasswordsStatus(),
+            'cacheFloodingProtection' => $this->getCacheFloodingProtectionStatus()
         ];
         return $statuses;
     }
 
     /**
+     * @return \TYPO3\CMS\Reports\Status An object representing whether the check is disabled
+     */
+    protected function getCacheFloodingProtectionStatus()
+    {
+        $value = $this->getLanguageService()->getLL('status_ok');
+        $message = '';
+        $severity = ReportStatus::OK;
+        if (empty($GLOBALS['TYPO3_CONF_VARS']['FE']['cHashIncludePageId'])) {
+            $value = $this->getLanguageService()->getLL('status_insecure');
+            $severity = ReportStatus::ERROR;
+            $message = $this->getLanguageService()->sL('LLL:EXT:lang/locallang_core.xlf:warning.install_cache_flooding');
+        }
+        return GeneralUtility::makeInstance(ReportStatus::class, $this->getLanguageService()->getLL('status_cacheFloodingProtection'), $value, $message, $severity);
+    }
+
+    /**
      * Checks if the trusted hosts pattern check is disabled.
      *
      * @return \TYPO3\CMS\Reports\Status An object representing whether the check is disabled
index 10b5ea6..4a5f656 100644 (file)
                        <trans-unit id="status_trustedHostsPattern">
                                <source>Trusted Hosts Pattern</source>
                        </trans-unit>
+                       <trans-unit id="status_cacheFloodingProtection">
+                               <source>Cache Flooding Protection</source>
+                       </trans-unit>
                        <trans-unit id="status_adminUserAccount">
                                <source>Admin User Account</source>
                        </trans-unit>