[SECURITY] Avoid out-of-scope page access for non-matching site

This change disallows calling an URI with page-id query parameters that are not part of a particular site - for instance the following URL `https://example.org/?id=3000&L=0` has two aspects: * the site `example.org` has the root page-id 1000 * the site `internal.example.org` has the root page-id 3000 The example above allows to call a page-id for an internal site, by using a valid and public entry point. The new feature flag `security.frontend.allowInsecureSiteResolutionByQueryParameters` allows to control this behavior for backward compatibility reasons. Per default `allowInsecureSiteResolutionByQueryParameters` is disabled. Resolves: #100889 Releases: main, 12.4, 11.5 Change-Id: I88d565b5d9bea556b4f754c3069d56124cea98bd Security-Bulletin: TYPO3-CORE-SA-2023-003 Security-References: CVE-2023-38499 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/80161 Tested-by: Oliver Hader <oliver.hader@typo3.org> Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
TYPO3 · Jul 25, 2023 · 37ab012 · 37ab012
1 parent 800f673
commit 37ab012
Show file tree

Hide file tree

Showing 9 changed files with 416 additions and 66 deletions.
diff --git a/typo3/sysext/core/Classes/Routing/SiteMatcher.php b/typo3/sysext/core/Classes/Routing/SiteMatcher.php
@@ -18,9 +18,11 @@
 namespace TYPO3\CMS\Core\Routing;
 
 use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Message\UriInterface;
 use Symfony\Component\Routing\Exception\NoConfigurationException;
 use Symfony\Component\Routing\Exception\ResourceNotFoundException;
 use TYPO3\CMS\Core\Cache\CacheManager;
+use TYPO3\CMS\Core\Configuration\Features;
 use TYPO3\CMS\Core\Exception\SiteNotFoundException;
 use TYPO3\CMS\Core\Http\NormalizedParams;
 use TYPO3\CMS\Core\SingletonInterface;
@@ -48,6 +50,7 @@
 class SiteMatcher implements SingletonInterface
 {
     public function __construct(
+        protected readonly Features $features,
         protected readonly SiteFinder $finder,
         protected readonly RequestContextFactory $requestContextFactory
     ) {
@@ -79,75 +82,42 @@ public function refresh()
      */
     public function matchRequest(ServerRequestInterface $request): RouteResultInterface
     {
-        $site = new NullSite();
-        $language = null;
-        $defaultLanguage = null;
+        // Remove script file name (index.php) from request uri
+        $uri = $this->canonicalizeUri($request->getUri(), $request);
+        $pageId = $this->resolvePageIdQueryParam($request);
+        $languageId = $this->resolveLanguageIdQueryParam($request);
 
-        $pageId = $request->getQueryParams()['id'] ?? $request->getParsedBody()['id'] ?? 0;
+        $routeResult = $this->matchSiteByUri($uri, $request);
 
-        // First, check if we have a _GET/_POST parameter for "id", then a site information can be resolved based.
-        if ($pageId > 0) {
-            // Loop over the whole rootline without permissions to get the actual site information
-            try {
-                $site = $this->finder->getSiteByPageId((int)$pageId);
-                // If a "L" parameter is given, we take that one into account.
-                $languageId = $request->getQueryParams()['L'] ?? $request->getParsedBody()['L'] ?? null;
-                if ($languageId !== null) {
-                    $language = $site->getLanguageById((int)$languageId);
-                } else {
-                    // Use this later below
-                    $defaultLanguage = $site->getDefaultLanguage();
-                }
-            } catch (SiteNotFoundException $e) {
-                // No site found by the given page
-            } catch (\InvalidArgumentException $e) {
-                // The language fetched by getLanguageById() was not available, now the PSR-15 middleware
-                // redirects to the default page.
-            }
+        // Allow insecure pageId based site resolution if explicitly enabled and only if both, ?id= and ?L= are defined
+        // (pageId based site resolution without L parameter has always been prohibited, so we do not support that)
+        if (
+            $this->features->isFeatureEnabled('security.frontend.allowInsecureSiteResolutionByQueryParameters') &&
+            $pageId !== null && $languageId !== null
+        ) {
+            return $this->matchSiteByQueryParams($pageId, $languageId, $routeResult, $uri);
         }
 
-        $uri = $request->getUri();
-        if (!empty($uri->getPath())) {
-            $normalizedParams = $request->getAttribute('normalizedParams');
-            if ($normalizedParams instanceof NormalizedParams) {
-                $urlPath = ltrim($uri->getPath(), '/');
-                $scriptName = ltrim($normalizedParams->getScriptName(), '/');
-                $scriptPath = ltrim($normalizedParams->getSitePath(), '/');
-                if ($scriptName !== '' && str_starts_with($urlPath, $scriptName)) {
-                    $urlPath = '/' . $scriptPath . substr($urlPath, mb_strlen($scriptName));
-                    $uri = $uri->withPath($urlPath);
-                }
-            }
+        // Allow the default language to be resolved in case all languages use a prefix
+        // and therefore did not match based on path if an explicit pageId is given,
+        // (example "https://www.example.com/?id=.." was entered, but all languages have "https://www.example.com/lang-key/")
+        // @todo remove this fallback, in order for SiteBaseRedirectResolver to produce a redirect instead (requires functionals to be adapted)
+        if ($pageId !== null && $routeResult->getLanguage() === null) {
+            $routeResult = $routeResult->withLanguage($routeResult->getSite()->getDefaultLanguage());
         }
 
-        // No language found at this point means that the URL was not used with a valid "?id=1&L=2" parameter
-        // which resulted in a site / language combination that was found. Now, the matching is done
-        // on the incoming URL.
-        if (!($language instanceof SiteLanguage)) {
-            $collection = $this->getRouteCollectionForAllSites();
-            $requestContext = $this->requestContextFactory->fromUri($uri, $request->getMethod());
-            $matcher = new BestUrlMatcher($collection, $requestContext);
+        // adjust the language aspect if it was given by query param `&L` (and ?id is given)
+        // @todo remove, this is added for backwards (and functional tests) compatibility reasons
+        if ($languageId !== null && $pageId !== null) {
             try {
-                $result = $matcher->match($uri->getPath());
-                return new SiteRouteResult(
-                    $uri,
-                    $result['site'],
-                    // if no language is found, this usually results due to "/" called instead of "/fr/"
-                    // but it could also be the reason that "/index.php?id=23" was called, so the default
-                    // language is used as a fallback here then.
-                    $result['language'] ?? $defaultLanguage,
-                    $result['tail']
-                );
-            } catch (NoConfigurationException | ResourceNotFoundException $e) {
-                // At this point we discard a possible found site via ?id=123
-                // Because ?id=123 _can_ only work if the actual domain/site base works
-                // so www.domain-without-site-configuration/index.php?id=123 (where 123 is a page referring
-                // to a page within a site configuration will never be resolved here) properly
-                $site = new NullSite();
+                // override/set language by `&L=` query param
+                $routeResult = $routeResult->withLanguage($routeResult->getSite()->getLanguageById($languageId));
+            } catch (\InvalidArgumentException) {
+                // ignore; language id not available
             }
         }
 
-        return new SiteRouteResult($uri, $site, $language);
+        return $routeResult;
     }
 
     /**
@@ -206,4 +176,94 @@ protected function getRouteCollectionForAllSites(): RouteCollection
         }
         return $collection;
     }
+
+    /**
+     * @return ?positive-int
+     */
+    protected function resolvePageIdQueryParam(ServerRequestInterface $request): ?int
+    {
+        $pageId = $request->getQueryParams()['id'] ?? $request->getParsedBody()['id'] ?? null;
+        if ($pageId === null) {
+            return null;
+        }
+        return (int)$pageId <= 0 ? null : (int)$pageId;
+    }
+
+    /**
+     * @return ?positive-int
+     */
+    protected function resolveLanguageIdQueryParam(ServerRequestInterface $request): ?int
+    {
+        $languageId = $request->getQueryParams()['L'] ?? $request->getParsedBody()['L'] ?? null;
+        if ($languageId === null) {
+            return null;
+        }
+        return (int)$languageId < 0 ? null : (int)$languageId;
+    }
+
+    /**
+     * Remove script file name (index.php) from request uri
+     */
+    protected function canonicalizeUri(UriInterface $uri, ServerRequestInterface $request): UriInterface
+    {
+        if ($uri->getPath() === '') {
+            return $uri;
+        }
+
+        $normalizedParams = $request->getAttribute('normalizedParams');
+        if (!$normalizedParams instanceof NormalizedParams) {
+            return $uri;
+        }
+
+        $urlPath = ltrim($uri->getPath(), '/');
+        $scriptName = ltrim($normalizedParams->getScriptName(), '/');
+        $scriptPath = ltrim($normalizedParams->getSitePath(), '/');
+        if ($scriptName !== '' && str_starts_with($urlPath, $scriptName)) {
+            $urlPath = '/' . $scriptPath . substr($urlPath, mb_strlen($scriptName));
+            $uri = $uri->withPath($urlPath);
+        }
+
+        return $uri;
+    }
+
+    protected function matchSiteByUri(UriInterface $uri, ServerRequestInterface $request): SiteRouteResult
+    {
+        $collection = $this->getRouteCollectionForAllSites();
+        $requestContext = $this->requestContextFactory->fromUri($uri, $request->getMethod());
+        $matcher = new BestUrlMatcher($collection, $requestContext);
+        try {
+            /** @var array{site: SiteInterface, language: ?SiteLanguage, tail: string} $match */
+            $match = $matcher->match($uri->getPath());
+            return new SiteRouteResult(
+                $uri,
+                $match['site'],
+                $match['language'],
+                $match['tail']
+            );
+        } catch (NoConfigurationException | ResourceNotFoundException) {
+            return new SiteRouteResult($uri, new NullSite(), null, '');
+        }
+    }
+
+    protected function matchSiteByQueryParams(
+        int $pageId,
+        int $languageId,
+        SiteRouteResult $fallback,
+        UriInterface $uri,
+    ): SiteRouteResult {
+        try {
+            $site = $this->finder->getSiteByPageId($pageId);
+        } catch (SiteNotFoundException) {
+            return $fallback;
+        }
+
+        try {
+            // override/set language by `&L=` query param
+            $language = $site->getLanguageById($languageId);
+        } catch (\InvalidArgumentException) {
+            return $fallback;
+        }
+
+        return new SiteRouteResult($uri, $site, $language);
+    }
 }
diff --git a/typo3/sysext/core/Classes/Routing/SiteRouteResult.php b/typo3/sysext/core/Classes/Routing/SiteRouteResult.php
@@ -95,6 +95,17 @@ public function offsetExists($offset): bool
         return in_array($offset, $this->validProperties, true) || isset($this->data[$offset]);
     }
 
+    /**
+     * @internal
+     */
+    public function withLanguage(SiteLanguage $language): self
+    {
+        $clone = clone $this;
+        $clone->language = $language;
+
+        return $clone;
+    }
+
     /**
      * @param mixed $offset
      */

diff --git a/typo3/sysext/core/Configuration/DefaultConfiguration.php b/typo3/sysext/core/Configuration/DefaultConfiguration.php
@@ -75,6 +75,7 @@
             'security.backend.htmlSanitizeRte' => false,
             'security.backend.enforceReferrer' => true,
             'security.frontend.enforceContentSecurityPolicy' => false,
+            'security.frontend.allowInsecureSiteResolutionByQueryParameters' => false,
             'security.usePasswordPolicyForFrontendUsers' => false,
         ],
         'createGroup' => '',

diff --git a/typo3/sysext/core/Configuration/DefaultConfigurationDescription.yaml b/typo3/sysext/core/Configuration/DefaultConfigurationDescription.yaml
@@ -207,6 +207,9 @@ SYS:
               security.frontend.enforceContentSecurityPolicy:
                 type: bool
                 description: 'If on, HTTP Content-Security-Policy header will be applied for each HTTP frontend request.'
+              security.frontend.allowInsecureSiteResolutionByQueryParameters:
+                type: bool
+                description: 'If on, site resolution can be overwritten by `&id=...&L=...` parameters, URI path & host are just used as default.'
               security.usePasswordPolicyForFrontendUsers:
                 type: bool
                 description: 'If on, the configured password policy in `$GLOBALS[TYPO3_CONF_VARS][FE][passwordPolicy]`

diff --git a/typo3/sysext/core/Tests/Unit/Routing/SiteMatcherTest.php b/typo3/sysext/core/Tests/Unit/Routing/SiteMatcherTest.php
@@ -17,6 +17,8 @@
 
 namespace TYPO3\CMS\Core\Tests\Unit\Routing;
 
+use PHPUnit\Framework\MockObject\MockObject;
+use TYPO3\CMS\Core\Configuration\Features;
 use TYPO3\CMS\Core\Configuration\SiteConfiguration;
 use TYPO3\CMS\Core\Http\ServerRequest;
 use TYPO3\CMS\Core\Routing\BackendEntryPointResolver;
@@ -74,9 +76,10 @@ public function fullUrlMatchesSpecificLanguageWithSubdomainsAndDomainSuffixes():
                 ],
             ],
         ]);
+        $featuresMock = $this->createFeaturesMock();
         $finderMock = $this->createSiteFinder($site, $secondSite);
         $requestContextFactory = new RequestContextFactory(new BackendEntryPointResolver());
-        $subject = new SiteMatcher($finderMock, $requestContextFactory);
+        $subject = new SiteMatcher($featuresMock, $finderMock, $requestContextFactory);
 
         $request = new ServerRequest('http://9-5.typo3.test/da/my-page/');
         /** @var SiteRouteResult $result */
@@ -171,9 +174,10 @@ public function fullUrlMatchesSpecificLanguageWithSubdomainsAndPathSuffixes(): v
                 ],
             ],
         ]);
+        $featuresMock = $this->createFeaturesMock();
         $finderMock = $this->createSiteFinder($site, $secondSite);
         $requestContextFactory = new RequestContextFactory(new BackendEntryPointResolver());
-        $subject = new SiteMatcher($finderMock, $requestContextFactory);
+        $subject = new SiteMatcher($featuresMock, $finderMock, $requestContextFactory);
 
         $request = new ServerRequest('https://www.example.com/de');
         /** @var SiteRouteResult $result */
@@ -253,9 +257,10 @@ public function bestMatchingUrlIsUsed(string $requestUri, string $expectedSite,
             ],
         ]);
 
+        $featuresMock = $this->createFeaturesMock();
         $finderMock = $this->createSiteFinder($mainSite, $dkSite, $frSite);
         $requestContextFactory = new RequestContextFactory(new BackendEntryPointResolver());
-        $subject = new SiteMatcher($finderMock, $requestContextFactory);
+        $subject = new SiteMatcher($featuresMock, $finderMock, $requestContextFactory);
 
         $request = new ServerRequest($requestUri);
         /** @var SiteRouteResult $result */
@@ -265,6 +270,18 @@ public function bestMatchingUrlIsUsed(string $requestUri, string $expectedSite,
         self::assertSame($expectedLocale, (string)$result->getLanguage()->getLocale());
     }
 
+    private function createFeaturesMock(): MockObject&Features
+    {
+        $mock = $this->getMockBuilder(Features::class)
+            ->onlyMethods(['isFeatureEnabled'])
+            ->getMock();
+        $mock->expects(self::any())
+            ->method('isFeatureEnabled')
+            ->with('security.frontend.allowInsecureSiteResolutionByQueryParameters')
+            ->willReturn(false);
+        return $mock;
+    }
+
     private function createSiteFinder(Site ...$sites): SiteFinder
     {
         $siteConfiguration = new class ($sites) extends SiteConfiguration {