Skip to content

Commit

Permalink
[SECURITY] Prevent XSS due to wrong PATH_INFO evaluation
Browse files Browse the repository at this point in the history
As already started in #88304 (but only for NormalizedParams)
and later reverted in #89312 (because of cgi-bin problems),
PATH_INFO is no longer considered as a preferable SCRIPT_NAME
alternative. All known server configurations set SCRIPT_NAME
these days to a proper value when cgi.fix_pathinfo is set.

The fallback to PATH_INFO has been introduced with
the initial revision of TYPO3 and isn't needed at all nowadays,
it's actually wrong, as a REQUEST_URI like /index.php/foo/bar
would incorrectly be interpreted as $scriptName == "/foo/bar",
which let's all calculations on $scriptName fail and
even leads to XSS where values derived from $scriptName are
printed without being escaped.

Also any ORIG_SCRIPT_NAME evaluation is dropped, as this variable
contains the SCRIPT_NAME that was set by the webserver configuration
before PHP applied cgi.fix_pathinfo. Using ORIG_SCRIPT_NAME
effectively meant bypassing PHP's pathinfo fix. It usually contains
the cgi-wrapper paths, which is why PATH_INFO was used to overrule
wrong ORIG_SCRIPT_NAME values.

GeneralUtility::getIndpEnv('PATH_INFO') is adapted to trust the
servers PATH_INFO information, now that we no longer allow
servers to send SCRIPT_NAME as PATH_INFO (we enforce
cgi.fix_pathinfo=1 for CGI installations).

The normalized SCRIPT_NAME is now adapted to be encoded as a URL
path by default, as all TYPO3 usages expect this to be an URL path.
Note that $_SERVER['SCRIPT_NAME'] refers to the servers file
system path, not the URL encoded value.

This SCRIPT_NAME sanitization actually enables:

a) TYPO3 to be run in a subfolder that contains characters
that need URL encoding
e.g. `/test:site/` – url encoded that'd be `/test3Asite/`.

b) prevention of XSS in case third party extensions
missed to escape any URL that is derived from SCRIPT_NAME
(while making sure that properly escaped
output is not double escaped)

Resolves: #99651
Related: #88304
Related: #89312
Releases: main, 11.5, 10.4
Change-Id: Ief95253d764665db5182a15ce8ffd02ea02ee61e
Security-Bulletin: TYPO3-CORE-SA-2023-001
Security-References: CVE-2023-24814
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/77739
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
  • Loading branch information
bnf authored and ohader committed Feb 7, 2023
1 parent db06e68 commit 0005a6f
Show file tree
Hide file tree
Showing 8 changed files with 55 additions and 119 deletions.
16 changes: 5 additions & 11 deletions typo3/sysext/core/Classes/Core/SystemEnvironmentBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -255,23 +255,17 @@ protected static function getPathThisScript(bool $isCli)
}

/**
* Calculate path to entry script if not in cli mode.
*
* Depending on the environment, the script path is found in different $_SERVER variables.
* Return path to entry script if not in cli mode.
*
* @return string Absolute path to entry script
*/
protected static function getPathThisScriptNonCli()
{
$isCgi = Environment::isRunningOnCgiServer();
if ($isCgi && Environment::usesCgiFixPathInfo()) {
return $_SERVER['SCRIPT_FILENAME'];
}
$cgiPath = $_SERVER['ORIG_PATH_TRANSLATED'] ?? $_SERVER['PATH_TRANSLATED'] ?? '';
if ($cgiPath && $isCgi) {
return $cgiPath;
if (Environment::isRunningOnCgiServer() && !Environment::usesCgiFixPathInfo()) {
throw new \Exception('TYPO3 does only support being used with cgi.fix_pathinfo=1 on CGI server APIs.', 1675108421);
}
return $_SERVER['ORIG_SCRIPT_FILENAME'] ?? $_SERVER['SCRIPT_FILENAME'];

return $_SERVER['SCRIPT_FILENAME'];
}

/**
Expand Down
28 changes: 12 additions & 16 deletions typo3/sysext/core/Classes/Http/NormalizedParams.php
Original file line number Diff line number Diff line change
Expand Up @@ -311,12 +311,13 @@ public function __construct(array $serverParams, array $configuration, string $p
$requestHost = $this->requestHost = ($isHttps ? 'https://' : 'http://') . $httpHost;
$requestHostOnly = $this->requestHostOnly = self::determineRequestHostOnly($httpHost);
$this->requestPort = self::determineRequestPort($httpHost, $requestHostOnly);
$scriptName = $this->scriptName = self::determineScriptName(
$scriptNameOnFileSystem = self::determineScriptName(
$serverParams,
$configuration,
$isHttps,
$isBehindReverseProxy
);
$scriptName = $this->scriptName = self::encodeFileSystemPathComponentForUrlPath($scriptNameOnFileSystem);
$requestUri = $this->requestUri = self::determineRequestUri(
$serverParams,
$configuration,
Expand All @@ -329,7 +330,7 @@ public function __construct(array $serverParams, array $configuration, string $p
$requestDir = $this->requestDir = $requestHost . GeneralUtility::dirname($scriptName) . '/';
$this->remoteAddress = self::determineRemoteAddress($serverParams, $configuration, $isBehindReverseProxy);
$scriptFilename = $this->scriptFilename = $pathThisScript;
$this->documentRoot = self::determineDocumentRoot($scriptName, $scriptFilename);
$this->documentRoot = self::determineDocumentRoot($scriptNameOnFileSystem, $scriptFilename);
$siteUrl = $this->siteUrl = self::determineSiteUrl($requestDir, $pathThisScript, $pathSite . '/');
$this->sitePath = self::determineSitePath($requestHost, $siteUrl);
$this->siteScript = self::determineSiteScript($requestUrl, $siteUrl);
Expand All @@ -344,6 +345,11 @@ public function __construct(array $serverParams, array $configuration, string $p
$this->queryString = $serverParams['QUERY_STRING'] ?? '';
}

private static function encodeFileSystemPathComponentForUrlPath(string $path): string
{
return implode('/', array_map('rawurlencode', explode('/', $path)));
}

/**
* @return string Sanitized HTTP_HOST value host[:port]
*/
Expand Down Expand Up @@ -632,17 +638,7 @@ protected static function determineScriptName(
bool $isHttps,
bool $isBehindReverseProxy
): string {
// see https://forge.typo3.org/issues/89312
// When using a CGI wrapper to dispatch the PHP process `ORIG_SCRIPT_NAME`
// contains the name of the wrapper script (which is most probably outside
// the TYPO3's project root) and leads to invalid prefixes, e.g. resolving
// the `siteUrl` incorrectly as `http://ip10.local/fcgi/` instead of
// actual `http://ip10.local/`
$possiblePathInfo = ($serverParams['ORIG_PATH_INFO'] ?? '') ?: ($serverParams['PATH_INFO'] ?? '');
$possibleScriptName = ($serverParams['ORIG_SCRIPT_NAME'] ?? '') ?: ($serverParams['SCRIPT_NAME'] ?? '');
$scriptName = Environment::isRunningOnCgiServer() && $possiblePathInfo
? $possiblePathInfo
: $possibleScriptName;
$scriptName = $serverParams['SCRIPT_NAME'] ?? '';
if ($isBehindReverseProxy) {
// Add a prefix if TYPO3 is behind a proxy: ext-domain.com => int-server.com/prefix
if ($isHttps && !empty($configuration['reverseProxyPrefixSSL'])) {
Expand Down Expand Up @@ -778,19 +774,19 @@ protected static function determineRequestPort(string $httpHost, string $httpHos
/**
* Calculate absolute path to web document root
*
* @param string $scriptName Entry script path of URI, without domain and without query parameters, with leading /
* @param string $scriptNameOnFileSystem Entry script path of URI on file system, without domain and without query parameters, with leading /
* @param string $scriptFilename Absolute path to entry script on server filesystem
* @return string Path to document root with trailing slash
*/
protected static function determineDocumentRoot(string $scriptName, string $scriptFilename): string
protected static function determineDocumentRoot(string $scriptNameOnFileSystem, string $scriptFilename): string
{
// Get the web root (it is not the root of the TYPO3 installation)
// Some CGI-versions (LA13CGI) and mod-rewrite rules on MODULE versions will deliver a 'wrong'
// DOCUMENT_ROOT (according to our description). Further various aliases/mod_rewrite rules can
// disturb this as well. Therefore the DOCUMENT_ROOT is always calculated as the SCRIPT_FILENAME
// minus the end part shared with SCRIPT_NAME.
$webDocRoot = '';
$scriptNameArray = explode('/', strrev($scriptName));
$scriptNameArray = explode('/', strrev($scriptNameOnFileSystem));
$scriptFilenameArray = explode('/', strrev($scriptFilename));
$path = [];
foreach ($scriptNameArray as $segmentNumber => $segment) {
Expand Down
28 changes: 12 additions & 16 deletions typo3/sysext/core/Classes/Utility/GeneralUtility.php
Original file line number Diff line number Diff line change
Expand Up @@ -2310,10 +2310,7 @@ public static function getIndpEnv($getEnvName)
$retVal = '';
switch ((string)$getEnvName) {
case 'SCRIPT_NAME':
$retVal = Environment::isRunningOnCgiServer()
&& (($_SERVER['ORIG_PATH_INFO'] ?? false) ?: ($_SERVER['PATH_INFO'] ?? false))
? (($_SERVER['ORIG_PATH_INFO'] ?? '') ?: ($_SERVER['PATH_INFO'] ?? ''))
: (($_SERVER['ORIG_SCRIPT_NAME'] ?? '') ?: ($_SERVER['SCRIPT_NAME'] ?? ''));
$retVal = $_SERVER['SCRIPT_NAME'] ?? '';
// Add a prefix if TYPO3 is behind a proxy: ext-domain.com => int-server.com/prefix
if (self::cmpIP($_SERVER['REMOTE_ADDR'] ?? '', $GLOBALS['TYPO3_CONF_VARS']['SYS']['reverseProxyIP'] ?? '')) {
if (self::getIndpEnv('TYPO3_SSL') && $GLOBALS['TYPO3_CONF_VARS']['SYS']['reverseProxyPrefixSSL']) {
Expand All @@ -2322,6 +2319,7 @@ public static function getIndpEnv($getEnvName)
$retVal = $GLOBALS['TYPO3_CONF_VARS']['SYS']['reverseProxyPrefix'] . $retVal;
}
}
$retVal = self::encodeFileSystemPathComponentForUrlPath($retVal);
break;
case 'SCRIPT_FILENAME':
$retVal = Environment::getCurrentScript();
Expand Down Expand Up @@ -2350,17 +2348,7 @@ public static function getIndpEnv($getEnvName)
}
break;
case 'PATH_INFO':
// $_SERVER['PATH_INFO'] != $_SERVER['SCRIPT_NAME'] is necessary because some servers (Windows/CGI)
// are seen to set PATH_INFO equal to script_name
// Further, there must be at least one '/' in the path - else the PATH_INFO value does not make sense.
// IF 'PATH_INFO' never works for our purpose in TYPO3 with CGI-servers,
// then 'PHP_SAPI=='cgi'' might be a better check.
// Right now strcmp($_SERVER['PATH_INFO'], GeneralUtility::getIndpEnv('SCRIPT_NAME')) will always
// return FALSE for CGI-versions, but that is only as long as SCRIPT_NAME is set equal to PATH_INFO
// because of PHP_SAPI=='cgi' (see above)
if (!Environment::isRunningOnCgiServer()) {
$retVal = $_SERVER['PATH_INFO'] ?? '';
}
$retVal = $_SERVER['PATH_INFO'] ?? '';
break;
case 'TYPO3_REV_PROXY':
$retVal = self::cmpIP($_SERVER['REMOTE_ADDR'] ?? '', $GLOBALS['TYPO3_CONF_VARS']['SYS']['reverseProxyIP']);
Expand Down Expand Up @@ -2433,7 +2421,10 @@ public static function getIndpEnv($getEnvName)
// Some CGI-versions (LA13CGI) and mod-rewrite rules on MODULE versions will deliver a 'wrong' DOCUMENT_ROOT (according to our description). Further various aliases/mod_rewrite rules can disturb this as well.
// Therefore the DOCUMENT_ROOT is now always calculated as the SCRIPT_FILENAME minus the end part shared with SCRIPT_NAME.
$SFN = self::getIndpEnv('SCRIPT_FILENAME');
$SN_A = explode('/', strrev(self::getIndpEnv('SCRIPT_NAME')));
// Use rawurldecode to reverse the result of self::encodeFileSystemPathComponentForUrlPath()
// which has been applied to getIndpEnv(SCRIPT_NAME) for web URI usage.
// We compare with a file system path (SCRIPT_FILENAME) in here and therefore need to undo the encoding.
$SN_A = array_map('rawurldecode', explode('/', strrev(self::getIndpEnv('SCRIPT_NAME'))));
$SFN_A = explode('/', strrev($SFN));
$acc = [];
foreach ($SN_A as $kk => $vv) {
Expand Down Expand Up @@ -2558,6 +2549,11 @@ protected static function webserverUsesHttps()
return !empty($_SERVER['HTTPS']) && strtolower($_SERVER['HTTPS']) !== 'off';
}

protected static function encodeFileSystemPathComponentForUrlPath(string $path): string
{
return implode('/', array_map('rawurlencode', explode('/', $path)));
}

/*************************
*
* TYPO3 SPECIFIC FUNCTIONS
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ private function createServerRequest(string $url, string $method = 'GET'): Serve
'SCRIPT_NAME' => '/typo3/index.php',
'PHP_SELF' => '/typo3/index.php',
'SCRIPT_FILENAME' => $docRoot . '/index.php',
'PATH_TRANSLATED' => $docRoot . '/index.php',
'QUERY_STRING' => $requestUrlParts['query'] ?? '',
'REQUEST_URI' => $requestUrlParts['path'] . (isset($requestUrlParts['query']) ? '?' . $requestUrlParts['query'] : ''),
'REQUEST_METHOD' => $method,
Expand Down
85 changes: 18 additions & 67 deletions typo3/sysext/core/Tests/Unit/Http/NormalizedParamsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -360,77 +360,19 @@ public function getScriptNameReturnsExpectedValueDataProvider(): array
[],
'',
],
'use ORIG_SCRIPT_NAME if ORIG_PATH_INFO is set but empty' => [
[
'ORIG_PATH_INFO' => '',
'PATH_INFO' => '',
'ORIG_SCRIPT_NAME' => '/orig/script/name.php',
'SCRIPT_NAME' => '/script/name.php',
],
[],
'/orig/script/name.php',
],
'use ORIG_SCRIPT_NAME if PATH_INFO is set but empty' => [
[
'PATH_INFO' => '',
'ORIG_SCRIPT_NAME' => '/orig/script/name.php',
'SCRIPT_NAME' => '/script/name.php',
],
[],
'/orig/script/name.php',
],
'use SCRIPT_NAME if ORIG_PATH_INFO is set but empty' => [
[
'ORIG_PATH_INFO' => '',
'PATH_INFO' => '',
'ORIG_SCRIPT_NAME' => '',
'SCRIPT_NAME' => '/script/name.php',
],
[],
'/script/name.php',
],
'use SCRIPT_NAME if PATH_INFO is set but empty' => [
[
'PATH_INFO' => '',
'ORIG_SCRIPT_NAME' => '',
'SCRIPT_NAME' => '/script/name.php',
],
[],
'/script/name.php',
],
'use SCRIPT_NAME if ORIG_PATH_INFO is set' => [
[
'ORIG_PATH_INFO' => '/foo/bar',
'PATH_INFO' => '',
'ORIG_SCRIPT_NAME' => '',
'SCRIPT_NAME' => '/script/name.php',
],
[],
'/script/name.php',
],
'use SCRIPT_NAME if PATH_INFO is set' => [
'use SCRIPT_NAME' => [
[
'PATH_INFO' => '/foo/bar',
'ORIG_SCRIPT_NAME' => '',
'SCRIPT_NAME' => '/script/name.php',
],
[],
'/script/name.php',
],
'use ORIG_SCRIPT_NAME' => [
[
'ORIG_SCRIPT_NAME' => '/orig/script/name.php',
'SCRIPT_NAME' => '/script/name.php',
],
[],
'/orig/script/name.php',
],
'use SCRIPT_NAME' => [
'apply URL encoding to SCRIPT_NAME' => [
[
'SCRIPT_NAME' => '/script/name.php',
'SCRIPT_NAME' => '/test:site/script/name.php',
],
[],
'/script/name.php',
'/test%3Asite/script/name.php',
],
'add proxy ssl prefix' => [
[
Expand Down Expand Up @@ -497,6 +439,14 @@ public function getRequestUriReturnsExpectedValueDataProvider(): array
[],
'/typo3/index.php?parameter=foo/bar&id=42',
],
'use query string and script name in special subdirectory if REQUEST_URI is not set' => [
[
'QUERY_STRING' => 'parameter=foo/bar&id=42',
'SCRIPT_NAME' => '/sub:dir/typo3/index.php',
],
[],
'/sub%3Adir/typo3/index.php?parameter=foo/bar&id=42',
],
'prefix with proxy prefix with ssl if using REQUEST_URI' => [
[
'HTTP_HOST' => 'www.domain.com',
Expand Down Expand Up @@ -905,7 +855,6 @@ public function getSiteUrlReturnsExpectedUrl(): void
$serverParams = [
'SCRIPT_NAME' => '/typo3/index.php',
'HTTP_HOST' => 'www.domain.com',
'PATH_INFO' => '/typo3/index.php',
];
$pathThisScript = '/var/www/myInstance/Web/typo3/index.php';
$pathSite = '/var/www/myInstance/Web';
Expand Down Expand Up @@ -978,7 +927,8 @@ public function getSiteScriptReturnsExpectedPathDataProvider(): array
return [
'not in a sub directory' => [
[
'SCRIPT_NAME' => '/typo3/index.php?id=42&foo=bar',
'SCRIPT_NAME' => '/typo3/index.php',
'REQUEST_URI' => '/typo3/index.php?id=42&foo=bar',
'HTTP_HOST' => 'www.domain.com',
],
'/var/www/myInstance/Web/typo3/index.php',
Expand All @@ -987,7 +937,8 @@ public function getSiteScriptReturnsExpectedPathDataProvider(): array
],
'in a sub directory' => [
[
'SCRIPT_NAME' => '/some/sub/dir/typo3/index.php?id=42&foo=bar',
'SCRIPT_NAME' => '/some/sub/dir/typo3/index.php',
'REQUEST_URI' => '/some/sub/dir/typo3/index.php?id=42&foo=bar',
'HTTP_HOST' => 'www.domain.com',
],
'/var/www/myInstance/Web/typo3/index.php',
Expand Down Expand Up @@ -1023,9 +974,9 @@ public function getSiteScriptReturnsExpectedPath(array $serverParams, string $pa
public function getPathInfoReturnsExpectedValue(): void
{
$serverParams = [
'PATH_INFO' => '/typo3/index.php',
'PATH_INFO' => '/foo/bar',
];
$expected = '/typo3/index.php';
$expected = '/foo/bar';
$serverRequestParameters = new NormalizedParams($serverParams, [], '', '');
self::assertSame($expected, $serverRequestParameters->getPathInfo());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,8 @@ class WebProcessorTest extends UnitTestCase
*/
public function webProcessorAddsWebDataToLogRecord(): void
{
$_SERVER['PATH_INFO'] = '';
$_SERVER['REQUEST_URI'] = '';
$_SERVER['ORIG_SCRIPT_NAME'] = '';
$_SERVER['SCRIPT_NAME'] = '';
$_SERVER['REMOTE_ADDR'] = '';
$_SERVER['QUERY_STRING'] = '';
$_SERVER['SSL_SESSION_ID'] = '';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ protected function setUp(): void
*/
protected function setUpFakeSitePathAndHost(): void
{
$_SERVER['ORIG_PATH_INFO'] = $_SERVER['PATH_INFO'] = $_SERVER['ORIG_SCRIPT_NAME'] = $_SERVER['SCRIPT_NAME'] = $this->testSitePath . 'index.php';
$_SERVER['SCRIPT_NAME'] = $this->testSitePath . 'index.php';
$_SERVER['HTTP_HOST'] = $this->testHostName;

$request = ServerRequestFactory::fromGlobals();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2423,23 +2423,24 @@ protected function setAbsRefPrefix()
if (!$this->absRefPrefix) {
return;
}
$encodedAbsRefPrefix = htmlspecialchars($this->absRefPrefix, ENT_QUOTES | ENT_HTML5);
$search = [
'"_assets/',
'"typo3temp/',
'"' . PathUtility::stripPathSitePrefix(Environment::getExtensionsPath()) . '/',
'"' . PathUtility::stripPathSitePrefix(Environment::getFrameworkBasePath()) . '/',
];
$replace = [
'"' . $this->absRefPrefix . '_assets/',
'"' . $this->absRefPrefix . 'typo3temp/',
'"' . $this->absRefPrefix . PathUtility::stripPathSitePrefix(Environment::getExtensionsPath()) . '/',
'"' . $this->absRefPrefix . PathUtility::stripPathSitePrefix(Environment::getFrameworkBasePath()) . '/',
'"' . $encodedAbsRefPrefix . '_assets/',
'"' . $encodedAbsRefPrefix . 'typo3temp/',
'"' . $encodedAbsRefPrefix . PathUtility::stripPathSitePrefix(Environment::getExtensionsPath()) . '/',
'"' . $encodedAbsRefPrefix . PathUtility::stripPathSitePrefix(Environment::getFrameworkBasePath()) . '/',
];
// Process additional directories
$directories = GeneralUtility::trimExplode(',', $GLOBALS['TYPO3_CONF_VARS']['FE']['additionalAbsRefPrefixDirectories'], true);
foreach ($directories as $directory) {
$search[] = '"' . $directory;
$replace[] = '"' . $this->absRefPrefix . $directory;
$replace[] = '"' . $encodedAbsRefPrefix . $directory;
}
$this->content = str_replace(
$search,
Expand Down

0 comments on commit 0005a6f

Please # to comment.