[SECURITY] Prevent XSS with fe_users data in felogin/TSFE 94/59094/2
authorBenni Mack <benni@typo3.org>
Tue, 11 Dec 2018 09:56:17 +0000 (10:56 +0100)
committerOliver Hader <oliver.hader@typo3.org>
Tue, 11 Dec 2018 09:56:19 +0000 (10:56 +0100)
Two occurrences allow to render data of the currently logged in
frontend user that is not sanitized and thus allow XSS attacks
by frontend users.

1. EXT:fe_login adds ###FEUSER_{fieldname}### for each
field that exists in the fe_users DB table, which CAN be processed
by TypoScript but is insecure by default.

2. config.USERNAME_substToken = <!--###USERNAME###-->
sets the username dynamically, which is then insecure.

Adding htmlspecialchars as a default configuration
solves this problem.

Resolves: #87053
Releases: master, 8.7, 7.6
Security-Commit: 3ef6a5c97381742eb6699923e9ed44224ab1e72e
Security-Bulletin: TYPO3-CORE-SA-2018-008
Change-Id: Ic0a48a36d1e5b394b6e829c5e209bdd2321b654e
Reviewed-on: https://review.typo3.org/59094
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
typo3/sysext/felogin/Classes/Controller/FrontendLoginController.php
typo3/sysext/felogin/Tests/Unit/Controller/FrontendLoginControllerTest.php
typo3/sysext/frontend/Classes/Controller/TypoScriptFrontendController.php

index b0c23c5..567062a 100644 (file)
@@ -1006,7 +1006,9 @@ class FrontendLoginController extends \TYPO3\CMS\Frontend\Plugin\AbstractPlugin
         if ($this->frontendController->fe_user->user) {
             // All fields of fe_user will be replaced, scheme is ###FEUSER_FIELDNAME###
             foreach ($this->frontendController->fe_user->user as $field => $value) {
-                $marker['###FEUSER_' . strtoupper($field) . '###'] = $this->cObj->stdWrap($value, $this->conf['userfields.'][$field . '.']);
+                $conf = $this->conf['userfields.'][$field . '.'] ?? [];
+                $conf = array_replace_recursive(['htmlSpecialChars' => '1'], $conf);
+                $marker['###FEUSER_' . strtoupper($field) . '###'] = $this->cObj->stdWrap($value, $conf);
             }
             // Add ###USER### for compatibility
             $marker['###USER###'] = $marker['###FEUSER_USERNAME###'];
index 2b6841e..e4e3484 100644 (file)
@@ -22,6 +22,7 @@ use TYPO3\CMS\Core\Database\Query\Expression\ExpressionBuilder;
 use TYPO3\CMS\Core\Database\Query\QueryBuilder;
 use TYPO3\CMS\Core\Tests\Unit\Database\Mocks\MockPlatform;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
+use TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer;
 use TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController;
 
 /**
@@ -516,4 +517,93 @@ class FrontendLoginControllerTest extends \TYPO3\TestingFramework\Core\Unit\Unit
         $tsfe->loginUser = true;
         $this->assertSame(['http://www.example.com/snafu'], $this->accessibleFixture->_call('processRedirect'));
     }
+
+    /**
+     *
+     */
+    public function processUserFieldsRespectsDefaultConfigurationForStdWrapDataProvider()
+    {
+        return [
+            'Simple casing' => [
+                [
+                    'username' => 'Holy',
+                    'lastname' => 'Wood',
+                ],
+                [
+                    'username.' => [
+                        'case' => 'upper'
+                    ]
+                ],
+                [
+                    '###FEUSER_USERNAME###' => 'HOLY',
+                    '###FEUSER_LASTNAME###' => 'Wood',
+                    '###USER###' => 'HOLY'
+                ]
+            ],
+            'Default config applies' => [
+                [
+                    'username' => 'Holy',
+                    'lastname' => 'O" Mally',
+                ],
+                [
+                    'username.' => [
+                        'case' => 'upper'
+                    ]
+                ],
+                [
+                    '###FEUSER_USERNAME###' => 'HOLY',
+                    '###FEUSER_LASTNAME###' => 'O&quot; Mally',
+                    '###USER###' => 'HOLY'
+                ]
+            ],
+            'Specific config overrides default config' => [
+                [
+                    'username' => 'Holy',
+                    'lastname' => 'O" Mally',
+                ],
+                [
+                    'username.' => [
+                        'case' => 'upper'
+                    ],
+                    'lastname.' => [
+                        'htmlSpecialChars' => '0'
+                    ]
+                ],
+                [
+                    '###FEUSER_USERNAME###' => 'HOLY',
+                    '###FEUSER_LASTNAME###' => 'O" Mally',
+                    '###USER###' => 'HOLY'
+                ]
+            ],
+            'No given user returns empty array' => [
+                null,
+                [
+                    'username.' => [
+                        'case' => 'upper'
+                    ],
+                    'lastname.' => [
+                        'htmlSpecialChars' => '0'
+                    ]
+                ],
+                []
+            ],
+        ];
+    }
+
+    /**
+     * @test
+     * @dataProvider processUserFieldsRespectsDefaultConfigurationForStdWrapDataProvider
+     */
+    public function processUserFieldsRespectsDefaultConfigurationForStdWrap($userRecord, $fieldConf, $expectedMarkers)
+    {
+        $tsfe = new \stdClass();
+        $tsfe->fe_user = new \stdClass();
+        $tsfe->fe_user->user = $userRecord;
+        $conf = ['userfields.' => $fieldConf];
+        $this->accessibleFixture->_set('cObj', new ContentObjectRenderer());
+        $this->accessibleFixture->_set('frontendController', $tsfe);
+        $this->accessibleFixture->_set('conf', $conf);
+        $actualResult = $this->accessibleFixture->_call('getUserFieldMarkers');
+        $this->assertEquals($expectedMarkers, $actualResult);
+    }
 }
index c2217e3..86b4863 100644 (file)
@@ -3748,7 +3748,7 @@ class TypoScriptFrontendController
             // User name:
             $token = isset($this->config['config']['USERNAME_substToken']) ? trim($this->config['config']['USERNAME_substToken']) : '';
             $search[] = $token ? $token : '<!--###USERNAME###-->';
-            $replace[] = $this->fe_user->user['username'];
+            $replace[] = htmlspecialchars($this->fe_user->user['username']);
             // User uid (if configured):
             $token = isset($this->config['config']['USERUID_substToken']) ? trim($this->config['config']['USERUID_substToken']) : '';
             if ($token) {