[!!!][SECURITY] Mitigate potential cache flooding 21/49921/2
authorHelmut Hummel <info@helhum.io>
Tue, 13 Sep 2016 09:52:49 +0000 (11:52 +0200)
committerOliver Hader <oliver.hader@typo3.org>
Tue, 13 Sep 2016 09:52:51 +0000 (11:52 +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: d67099a5e5dd387fa3fb8a9847933fbeb377d99f
Security-Bulletins: TYPO3-CORE-SA-2016-020, 021
Change-Id: Ie9753536dad5cae60e607a286e1ebb08efc3c85a
Reviewed-on: https://review.typo3.org/49921
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 9b6d951..bc0998a 100644 (file)
@@ -598,7 +598,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 8cda7eb..e9577fc 100644 (file)
@@ -1134,6 +1134,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 1d59842..6e71d8e 100644 (file)
@@ -6612,8 +6612,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=' . $linkParameter;
                         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 0106e90..177fbfd 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->getMock(\TYPO3\CMS\Frontend\Page\CacheHashCalculator::class, ['foo']);
         $this->subject->setConfiguration([
@@ -41,12 +33,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)
@@ -57,7 +50,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' => [
@@ -100,26 +93,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)
@@ -128,22 +125,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']
         ];
     }
 
@@ -174,7 +181,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)
@@ -188,21 +195,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)
@@ -215,28 +222,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 22283ef..36c75b3 100644 (file)
@@ -1113,6 +1113,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 bb4506a..a19dacc 100644 (file)
@@ -37,12 +37,29 @@ class SecurityStatus implements \TYPO3\CMS\Reports\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 = $GLOBALS['LANG']->getLL('status_ok');
+        $message = '';
+        $severity = \TYPO3\CMS\Reports\Status::OK;
+        if (empty($GLOBALS['TYPO3_CONF_VARS']['FE']['cHashIncludePageId'])) {
+            $value = $GLOBALS['LANG']->getLL('status_insecure');
+            $severity = \TYPO3\CMS\Reports\Status::ERROR;
+            $message = $GLOBALS['LANG']->sL('LLL:EXT:lang/locallang_core.xlf:warning.install_cache_flooding');
+        }
+        return GeneralUtility::makeInstance(\TYPO3\CMS\Reports\Status::class, $GLOBALS['LANG']->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 d574adc..c6b0326 100644 (file)
@@ -99,6 +99,9 @@
                        <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>