[SECURITY] Prevent XSS with fe_users data in felogin/TSFE 02/59102/2
authorBenni Mack <benni@typo3.org>
Tue, 11 Dec 2018 09:57:11 +0000 (10:57 +0100)
committerOliver Hader <oliver.hader@typo3.org>
Tue, 11 Dec 2018 09:57:13 +0000 (10:57 +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: 1cc57f4aa7dfb5b1e3e4db581c57aacd69dd4d9d
Security-Bulletin: TYPO3-CORE-SA-2018-008
Change-Id: I72a1a4ea60f23c81016b87cbbd1ba63161c52df0
Reviewed-on: https://review.typo3.org/59102
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 5095f3a..e48fc15 100644 (file)
@@ -998,7 +998,9 @@ class FrontendLoginController extends AbstractPlugin implements LoggerAwareInter
         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 48e8e99..1a246d8 100644 (file)
@@ -27,6 +27,7 @@ use TYPO3\CMS\Core\Exception\SiteNotFoundException;
 use TYPO3\CMS\Core\Site\SiteFinder;
 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;
 use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
 
@@ -546,4 +547,93 @@ class FrontendLoginControllerTest extends UnitTestCase
         $this->accessibleFixture->_set('userIsLoggedIn', 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 d19ace3..dad00cb 100644 (file)
@@ -4153,7 +4153,7 @@ class TypoScriptFrontendController implements LoggerAwareInterface
             // 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) {