Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[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/+/77087
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
  • Loading branch information
ohader committed Dec 13, 2022
1 parent edf344b commit 091735d
Show file tree
Hide file tree
Showing 8 changed files with 215 additions and 5 deletions.
Expand Up @@ -347,7 +347,7 @@ protected function saveAction(ServerRequestInterface $request): ResponseInterfac
if (!$isNewConfiguration && $currentIdentifier !== $siteIdentifier) {
$siteConfigurationManager->rename($currentIdentifier, $siteIdentifier);
}
$siteConfigurationManager->write($siteIdentifier, $newSiteConfiguration);
$siteConfigurationManager->write($siteIdentifier, $newSiteConfiguration, true);
} catch (SiteValidationErrorException $e) {
// Do not store new config if a validation error is thrown, but redirect only to show a generated flash message
}
Expand Down
@@ -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
{
}
Expand Up @@ -48,6 +48,7 @@ class YamlFileLoader implements LoggerAwareInterface
{
use LoggerAwareTrait;

public const PATTERN_PARTS = '%[^(%]+?\([\'"]?([^(]*?)[\'"]?\)%|%([^%()]*?)%';
public const PROCESS_PLACEHOLDERS = 1;
public const PROCESS_IMPORTS = 2;

Expand Down Expand Up @@ -259,7 +260,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
Expand Down
@@ -0,0 +1,111 @@
<?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
{
/**
* @var array<string, mixed>
*/
protected $existingConfiguration;

/**
* @var StringFragmentSplitter
*/
protected $fragmentSplitter;

public function __construct(array $existingConfiguration)
{
$this->existingConfiguration = $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;
}
}
30 changes: 29 additions & 1 deletion typo3/sysext/core/Classes/Configuration/SiteConfiguration.php
Expand Up @@ -21,7 +21,9 @@
use Symfony\Component\Yaml\Yaml;
use TYPO3\CMS\Core\Cache\CacheManager;
use TYPO3\CMS\Core\Cache\Frontend\PhpFrontend;
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;
Expand Down Expand Up @@ -196,15 +198,19 @@ public function load(string $siteIdentifier): array
*
* @param string $siteIdentifier
* @param array $configuration
* @param bool $protectPlaceholders whether to disallow introducing new placeholders
* @throws \TYPO3\CMS\Core\Cache\Exception\NoSuchCacheException
*/
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
Expand All @@ -216,6 +222,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);
}
Expand Down Expand Up @@ -265,6 +274,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 \RuntimeException
*/
protected function protectPlaceholders(array $existingConfiguration, array $modifiedConfiguration): array
{
try {
return GeneralUtility::makeInstance(YamlPlaceholderGuard::class, $existingConfiguration)
->process($modifiedConfiguration);
} catch (YamlPlaceholderException $exception) {
throw new \RuntimeException($exception->getMessage(), 1670361271, $exception);
}
}

/**
* Short-hand function for the cache
*
Expand Down
Expand Up @@ -3,6 +3,7 @@ baseVariants:
-
base: foo123
condition: bar
customProperty: 'Using %env("existing")% variable'
errorHandling:
-
errorCode: '404'
Expand Down
Expand Up @@ -3,6 +3,7 @@ baseVariants:
-
base: foo123
condition: bar
customProperty: 'Using %env("existing")% variable'
errorHandling:
-
errorCode: '404'
Expand Down
Expand Up @@ -116,7 +116,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);
Expand Down Expand Up @@ -155,9 +155,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(\RuntimeException::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);
}
}

0 comments on commit 091735d

Please sign in to comment.