TYPO3 contains Sensitive Information Disclosure via YAML Placeholder Expressions in Site Configuration

@@ -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 {

@@ -0,0 +1,22 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+namespace TYPO3\CMS\Core\Configuration\Loader\Exception;
+
+class YamlPlaceholderException extends \RuntimeException
+{
+}

@@ -47,6 +47,7 @@ class YamlFileLoader implements LoggerAwareInterface
 {
     use LoggerAwareTrait;
 
+    public const PATTERN_PARTS = '%[^(%]+?\([\'"]?([^(]*?)[\'"]?\)%|%([^%()]*?)%';
     public const PROCESS_PLACEHOLDERS = 1;
     public const PROCESS_IMPORTS = 2;
 
@@ -252,7 +253,7 @@ protected function getParts(string $placeholders): array
         // find occurrences of placeholders like %some()% and %array.access%.
         // Only find the innermost ones, so we can nest them.
         preg_match_all(
-            '/%[^(%]+?\([\'"]?([^(]*?)[\'"]?\)%|%([^%()]*?)%/',
+            '/' . self::PATTERN_PARTS . '/',
             $placeholders,
             $parts,
             PREG_UNMATCHED_AS_NULL

@@ -0,0 +1,102 @@
+<?php
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+namespace TYPO3\CMS\Core\Configuration\Loader;
+
+use TYPO3\CMS\Core\Configuration\Loader\Exception\YamlPlaceholderException;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
+use TYPO3\CMS\Core\Utility\String\StringFragmentPattern;
+use TYPO3\CMS\Core\Utility\String\StringFragmentSplitter;
+
+/**
+ * A guard for protecting YAML placeholders - keeps existing, but escalates on adding new placeholders
+ */
+class YamlPlaceholderGuard
+{
+    protected StringFragmentSplitter $fragmentSplitter;
+
+    public function __construct(protected array $existingConfiguration)
+    {
+        $fragmentPattern = GeneralUtility::makeInstance(
+            StringFragmentPattern::class,
+            StringFragmentSplitter::TYPE_EXPRESSION,
+            YamlFileLoader::PATTERN_PARTS
+        );
+        $this->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<string, mixed> $current
+     * @param array<string, mixed> $modified
+     * @param list<string> $steps configuration keys traversed so far
+     * @return array<string, mixed> 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;
+    }
+}

@@ -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<string, mixed> $existingConfiguration
+     * @param array<string, mixed> $modifiedConfiguration
+     * @return array<string, mixed> 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);

@@ -3,6 +3,7 @@ baseVariants:
   -
     base: foo123
     condition: bar
+customProperty: 'Using %env("existing")% variable'
 errorHandling:
   -
     errorCode: '404'

@@ -3,6 +3,7 @@ baseVariants:
   -
     base: foo123
     condition: bar
+customProperty: 'Using %env("existing")% variable'
 errorHandling:
   -
     errorCode: '404'

@@ -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);
+    }
 }