From d1e627ff7eef07bd94c53db861e85977b203900a Mon Sep 17 00:00:00 2001 From: Oliver Hader Date: Tue, 13 Dec 2022 10:21:54 +0100 Subject: [PATCH] [SECURITY] Disallow introducing Yaml placeholders in user interface Introducing Yaml placeholders in backend user interface can lead to information disclosure and denial-of-service senarios. This change disallows adding new placeholders and throws an exception - existing placeholders are kept. Resolves: #89401 Releases: main, 11.5, 10.4 Change-Id: I69e24de07b5327507e1bf8de990f84402078f7d4 Security-Bulletin: TYPO3-CORE-SA-2022-016 Security-References: CVE-2022-23504 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/77103 Reviewed-by: Oliver Hader Tested-by: Oliver Hader --- .../SiteConfigurationController.php | 2 +- .../Exception/YamlPlaceholderException.php | 22 ++++ .../Configuration/Loader/YamlFileLoader.php | 3 +- .../Loader/YamlPlaceholderGuard.php | 102 ++++++++++++++++++ .../Configuration/SiteConfiguration.php | 31 +++++- .../Fixtures/SiteConfigs/config2.yaml | 1 + .../SiteConfigs/config2_expected.yaml | 1 + .../Configuration/SiteConfigurationTest.php | 51 ++++++++- 8 files changed, 208 insertions(+), 5 deletions(-) create mode 100644 typo3/sysext/core/Classes/Configuration/Loader/Exception/YamlPlaceholderException.php create mode 100644 typo3/sysext/core/Classes/Configuration/Loader/YamlPlaceholderGuard.php diff --git a/typo3/sysext/backend/Classes/Controller/SiteConfigurationController.php b/typo3/sysext/backend/Classes/Controller/SiteConfigurationController.php index 96745e424e99..fdf6d6271cb4 100644 --- a/typo3/sysext/backend/Classes/Controller/SiteConfigurationController.php +++ b/typo3/sysext/backend/Classes/Controller/SiteConfigurationController.php @@ -411,7 +411,7 @@ protected function saveAction(ServerRequestInterface $request): ResponseInterfac $siteConfigurationManager->rename($currentIdentifier, $siteIdentifier); $this->getBackendUser()->writelog(Type::SITE, SiteAction::RENAME, SystemLogErrorClassification::MESSAGE, 0, 'Site configuration \'%s\' was renamed to \'%s\'.', [$currentIdentifier, $siteIdentifier], 'site'); } - $siteConfigurationManager->write($siteIdentifier, $newSiteConfiguration); + $siteConfigurationManager->write($siteIdentifier, $newSiteConfiguration, true); if ($isNewConfiguration) { $this->getBackendUser()->writelog(Type::SITE, SiteAction::CREATE, SystemLogErrorClassification::MESSAGE, 0, 'Site configuration \'%s\' was created.', [$siteIdentifier], 'site'); } else { diff --git a/typo3/sysext/core/Classes/Configuration/Loader/Exception/YamlPlaceholderException.php b/typo3/sysext/core/Classes/Configuration/Loader/Exception/YamlPlaceholderException.php new file mode 100644 index 000000000000..3457557b2c10 --- /dev/null +++ b/typo3/sysext/core/Classes/Configuration/Loader/Exception/YamlPlaceholderException.php @@ -0,0 +1,22 @@ +fragmentSplitter = GeneralUtility::makeInstance( + StringFragmentSplitter::class, + $fragmentPattern + ); + } + + /** + * Modifies existing configuration. + */ + public function process(array $modified): array + { + return $this->protectPlaceholders($this->existingConfiguration, $modified); + } + + /** + * Detects placeholders that have been introduced and handles* them. + * (*) currently throws an exception, but could be purged or escaped as well + * + * @param array $current + * @param array $modified + * @param list $steps configuration keys traversed so far + * @return array sanitized configuration (currently not used, exception thrown before) + * @throws YamlPlaceholderException + */ + protected function protectPlaceholders(array $current, array $modified, array $steps = []): array + { + foreach ($modified as $key => $value) { + $currentSteps = array_merge($steps, [$key]); + if (is_array($value)) { + $modified[$key] = $this->protectPlaceholders( + $current[$key] ?? [], + $value, + $currentSteps + ); + } elseif (is_string($value)) { + $splitFlags = StringFragmentSplitter::FLAG_UNMATCHED_AS_NULL; + $newFragments = $this->fragmentSplitter->split($value, $splitFlags); + if (is_string($current[$key] ?? null)) { + $currentFragments = $this->fragmentSplitter->split($current[$key] ?? '', $splitFlags); + } else { + $currentFragments = null; + } + // in case there are new fragments (at least one matching the pattern) + if ($newFragments !== null) { + // compares differences in `expression` fragments only + $differences = $currentFragments === null + ? $newFragments->withOnlyType(StringFragmentSplitter::TYPE_EXPRESSION) + : $newFragments->withOnlyType(StringFragmentSplitter::TYPE_EXPRESSION) + ->diff($currentFragments->withOnlyType(StringFragmentSplitter::TYPE_EXPRESSION)); + if (count($differences) > 0) { + throw new YamlPlaceholderException( + sprintf( + 'Introducing placeholder%s %s for %s is not allowed', + count($differences) !== 1 ? 's' : '', + implode(', ', $differences->getFragments()), + implode('.', $currentSteps) + ), + 1651690534 + ); + } + } + } + } + return $modified; + } +} diff --git a/typo3/sysext/core/Classes/Configuration/SiteConfiguration.php b/typo3/sysext/core/Classes/Configuration/SiteConfiguration.php index 82534ccdc15d..7e2ae4587141 100644 --- a/typo3/sysext/core/Classes/Configuration/SiteConfiguration.php +++ b/typo3/sysext/core/Classes/Configuration/SiteConfiguration.php @@ -26,7 +26,9 @@ use TYPO3\CMS\Core\Configuration\Event\SiteConfigurationBeforeWriteEvent; use TYPO3\CMS\Core\Configuration\Event\SiteConfigurationLoadedEvent; use TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException; +use TYPO3\CMS\Core\Configuration\Loader\Exception\YamlPlaceholderException; use TYPO3\CMS\Core\Configuration\Loader\YamlFileLoader; +use TYPO3\CMS\Core\Configuration\Loader\YamlPlaceholderGuard; use TYPO3\CMS\Core\Exception\SiteNotFoundException; use TYPO3\CMS\Core\SingletonInterface; use TYPO3\CMS\Core\Site\Entity\Site; @@ -257,15 +259,20 @@ public function writeSettings(string $siteIdentifier, array $settings): void /** * Add or update a site configuration * + * @param bool $protectPlaceholders whether to disallow introducing new placeholders + * @todo enforce $protectPlaceholders with TYPO3 v13.0 * @throws SiteConfigurationWriteException */ - public function write(string $siteIdentifier, array $configuration): void + public function write(string $siteIdentifier, array $configuration, bool $protectPlaceholders = false): void { $folder = $this->configPath . '/' . $siteIdentifier; $fileName = $folder . '/' . $this->configFileName; $newConfiguration = $configuration; if (!file_exists($folder)) { GeneralUtility::mkdir_deep($folder); + if ($protectPlaceholders && $newConfiguration !== []) { + $newConfiguration = $this->protectPlaceholders([], $newConfiguration); + } } elseif (file_exists($fileName)) { $loader = GeneralUtility::makeInstance(YamlFileLoader::class); // load without any processing to have the unprocessed base to modify @@ -277,6 +284,9 @@ public function write(string $siteIdentifier, array $configuration): void self::findRemoved($processed, $configuration), self::findModified($processed, $configuration) ); + if ($protectPlaceholders && $newModified !== []) { + $newModified = $this->protectPlaceholders($newConfiguration, $newModified); + } // change _only_ the modified keys, leave the original non-changed areas alone ArrayUtility::mergeRecursiveWithOverrule($newConfiguration, $newModified); } @@ -327,6 +337,25 @@ public function delete(string $siteIdentifier): void $this->firstLevelCache = null; } + /** + * Detects placeholders that have been introduced and handles* them. + * (*) currently throws an exception, but could be purged or escaped as well + * + * @param array $existingConfiguration + * @param array $modifiedConfiguration + * @return array sanitized configuration (currently not used, exception thrown before) + * @throws SiteConfigurationWriteException + */ + protected function protectPlaceholders(array $existingConfiguration, array $modifiedConfiguration): array + { + try { + return GeneralUtility::makeInstance(YamlPlaceholderGuard::class, $existingConfiguration) + ->process($modifiedConfiguration); + } catch (YamlPlaceholderException $exception) { + throw new SiteConfigurationWriteException($exception->getMessage(), 1670361271, $exception); + } + } + protected function sortConfiguration(array $newConfiguration): array { ksort($newConfiguration); diff --git a/typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config2.yaml b/typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config2.yaml index b612292bca6e..eee4d05dceaa 100644 --- a/typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config2.yaml +++ b/typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config2.yaml @@ -3,6 +3,7 @@ baseVariants: - base: foo123 condition: bar +customProperty: 'Using %env("existing")% variable' errorHandling: - errorCode: '404' diff --git a/typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config2_expected.yaml b/typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config2_expected.yaml index 305e894edd5f..e72ab3957907 100644 --- a/typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config2_expected.yaml +++ b/typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config2_expected.yaml @@ -3,6 +3,7 @@ baseVariants: - base: foo123 condition: bar +customProperty: 'Using %env("existing")% variable' errorHandling: - errorCode: '404' diff --git a/typo3/sysext/core/Tests/Unit/Configuration/SiteConfigurationTest.php b/typo3/sysext/core/Tests/Unit/Configuration/SiteConfigurationTest.php index 0a2b10450e15..6a19b2fb85e7 100644 --- a/typo3/sysext/core/Tests/Unit/Configuration/SiteConfigurationTest.php +++ b/typo3/sysext/core/Tests/Unit/Configuration/SiteConfigurationTest.php @@ -19,6 +19,7 @@ use Symfony\Component\Yaml\Yaml; use TYPO3\CMS\Core\Cache\Frontend\NullFrontend; +use TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException; use TYPO3\CMS\Core\Configuration\Loader\YamlFileLoader; use TYPO3\CMS\Core\Configuration\SiteConfiguration; use TYPO3\CMS\Core\Core\Environment; @@ -107,7 +108,7 @@ public function writeOnlyWritesModifiedKeys(): void // delete values unset($configuration['someOtherValue'], $configuration['languages'][1]); - $this->siteConfiguration->write($identifier, $configuration); + $this->siteConfiguration->write($identifier, $configuration, true); // expect modified base but intact imports self::assertFileEquals($expected, $siteConfig); @@ -146,9 +147,55 @@ public function writingOfNestedStructuresPreservesOrder(): void 'navigationTitle' => 'English', ]; array_unshift($configuration['languages'], $languageConfig); - $this->siteConfiguration->write($identifier, $configuration); + $this->siteConfiguration->write($identifier, $configuration, true); // expect modified base but intact imports self::assertFileEquals($expected, $siteConfig); } + + public static function writingPlaceholdersIsHandledDataProvider(): \Generator + { + yield 'unchanged' => [ + ['customProperty' => 'Using %env("existing")% variable'], + false, + ]; + yield 'removed placeholder variable' => [ + ['customProperty' => 'Not using any variable'], + false, + ]; + yield 'changed raw text only' => [ + ['customProperty' => 'Using %env("existing")% variable from system environment'], + false, + ]; + yield 'added new placeholder variable' => [ + ['customProperty' => 'Using %env("existing")% and %env("secret")% variable'], + true, + ]; + } + + /** + * @test + * @dataProvider writingPlaceholdersIsHandledDataProvider + */ + public function writingPlaceholdersIsHandled(array $changes, bool $expectedException): void + { + if ($expectedException) { + $this->expectException(SiteConfigurationWriteException::class); + $this->expectExceptionCode(1670361271); + } + + $identifier = 'testsite'; + GeneralUtility::mkdir_deep($this->fixturePath . '/' . $identifier); + $configFixture = __DIR__ . '/Fixtures/SiteConfigs/config2.yaml'; + $siteConfig = $this->fixturePath . '/' . $identifier . '/config.yaml'; + copy($configFixture, $siteConfig); + // load with resolved imports as the module does + $configuration = GeneralUtility::makeInstance(YamlFileLoader::class) + ->load( + GeneralUtility::fixWindowsFilePath($siteConfig), + YamlFileLoader::PROCESS_IMPORTS + ); + $configuration = array_merge($configuration, $changes); + $this->siteConfiguration->write($identifier, $configuration, true); + } }