PB-39045 Fix fullBaseUrl should not fallback to Host header if PASSBO…

…LT_SECURITY_PREVENT_HOST_HEADER_FALLBACK is set to true
passbolt · Feb 12, 2025 · 09a528d · 09a528d
1 parent d9bb992
commit 09a528d
Show file tree

Hide file tree

Showing 12 changed files with 272 additions and 123 deletions.
diff --git a/config/bootstrap.php b/config/bootstrap.php
@@ -154,6 +154,8 @@
  * This URL is used as the base of all absolute links.
  */
 $fullBaseUrl = Configure::read('App.fullBaseUrl');
+// Store original full base url from config before it's been modified
+Configure::write('passbolt.originalFullBaseUrl', $fullBaseUrl);
 if (!$fullBaseUrl) {
     /*
      * When using proxies or load balancers, SSL/TLS connections might

diff --git a/config/default.php b/config/default.php
@@ -40,6 +40,8 @@
         // Edition.
         'edition' => 'ce',
         'featurePluginAdder' => \App\BaseSolutionBootstrapper::class,
+        // set in bootstrap.php
+        'originalFullBaseUrl' => '',
         'v5' => [
             'enabled' => filter_var(env('PASSBOLT_V5_ENABLED', false), FILTER_VALIDATE_BOOLEAN),
         ],
@@ -328,7 +330,9 @@
                 'secure' => filter_var(env('PASSBOLT_SECURITY_COOKIE_SECURE', true), FILTER_VALIDATE_BOOLEAN)
             ],
             'setHeaders' => filter_var(env('PASSBOLT_SECURITY_SET_HEADERS', true), FILTER_VALIDATE_BOOLEAN),
-            'preventHostHeaderFallback' => filter_var(env('PASSBOLT_SECURITY_PREVENT_HOST_HEADER_FALLBACK', false), FILTER_VALIDATE_BOOLEAN),
+            // By default, false (unsafe) for BC, will be true in v5.0
+            'fullBaseUrlEnforce' => filter_var(env('PASSBOLT_SECURITY_FULLBASEURL_ENFORCE', false), FILTER_VALIDATE_BOOLEAN),
+            'emptyFullBaseUrlWarn' => filter_var(env('PASSBOLT_SECURITY_EMPTY_FULLBASEURL_WARN', true), FILTER_VALIDATE_BOOLEAN),
             'csrfProtection' => [
                 'active' => true,
                 'unlockedActions' => [

diff --git a/src/Application.php b/src/Application.php
@@ -22,12 +22,12 @@
 use App\Command\PassboltBuildCommandsListener;
 use App\Command\SqlExportCommand;
 use App\Middleware\ApiVersionMiddleware;
+use App\Middleware\AssertFullBaseUrlMiddleware;
 use App\Middleware\ContainerInjectorMiddleware;
 use App\Middleware\ContentSecurityPolicyMiddleware;
 use App\Middleware\CsrfProtectionMiddleware;
 use App\Middleware\GpgAuthHeadersMiddleware;
 use App\Middleware\HttpProxyMiddleware;
-use App\Middleware\PreventHostHeaderFallbackMiddleware;
 use App\Middleware\SessionAuthPreventDeletedOrDisabledUsersMiddleware;
 use App\Middleware\SessionPreventExtensionMiddleware;
 use App\Middleware\SslForceMiddleware;
@@ -107,7 +107,7 @@ public function middleware(MiddlewareQueue $middlewareQueue): MiddlewareQueue
             ->prepend(new ContainerInjectorMiddleware($this->getContainer()))
             ->add(new ContentSecurityPolicyMiddleware())
             ->add(new ErrorHandlerMiddleware(Configure::read('Error')))
-            ->add(new PreventHostHeaderFallbackMiddleware())
+            ->add(new AssertFullBaseUrlMiddleware())
             ->add(SslForceMiddleware::class)
             ->add(new AssetMiddleware(['cacheTime' => Configure::read('Asset.cacheTime')]))
             ->add(new RoutingMiddleware($this))

diff --git a/...e/PreventHostHeaderFallbackMiddleware.php → ...iddleware/AssertFullBaseUrlMiddleware.php b/...e/PreventHostHeaderFallbackMiddleware.php → ...iddleware/AssertFullBaseUrlMiddleware.php
@@ -12,45 +12,45 @@
  * @copyright     Copyright (c) Passbolt SA (https://www.passbolt.com)
  * @license       https://opensource.org/licenses/AGPL-3.0 AGPL License
  * @link          https://www.passbolt.com Passbolt(tm)
- * @since         4.11.0
+ * @since         4.11.1
  */
 namespace App\Middleware;
 
 use Cake\Core\Configure;
-use Cake\Http\Exception\BadRequestException;
+use Cake\Http\Exception\InternalErrorException;
+use Cake\Log\Log;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Server\MiddlewareInterface;
 use Psr\Http\Server\RequestHandlerInterface;
 
-class PreventHostHeaderFallbackMiddleware implements MiddlewareInterface
+class AssertFullBaseUrlMiddleware implements MiddlewareInterface
 {
     /**
-     * Throws a bad request if the full base url is not set, host header is set and header fallback config is set to true.
-     *
      * @param \Psr\Http\Message\ServerRequestInterface $request The request.
      * @param \Psr\Http\Server\RequestHandlerInterface $handler The request handler.
      * @return \Psr\Http\Message\ResponseInterface A response.
-     * @throws \Cake\Http\Exception\BadRequestException if the API version provided is deprecated
+     * @throws \Cake\Http\Exception\InternalErrorException If config value is invalid
      */
     public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
     {
-        $configPreventHostHeaderFallback = Configure::read('passbolt.security.preventHostHeaderFallback', false);
-        if (!$configPreventHostHeaderFallback) {
-            return $handler->handle($request);
-        }
+        $enforceFullBaseUrl = Configure::read('passbolt.security.fullBaseUrlEnforce', false);
+        $originalFullBaseUrl = Configure::read('passbolt.originalFullBaseUrl', '');
+        $isValidFullBaseUrl = is_string($originalFullBaseUrl) && $originalFullBaseUrl !== '';
+
+        if (!$enforceFullBaseUrl) {
+            $warnEmptyFullBaseUrl = Configure::read('passbolt.security.emptyFullBaseUrlWarn', true);
+            if ($warnEmptyFullBaseUrl && !$isValidFullBaseUrl) {
+                Log::warning('Your FullBaseUrl configuration is not safe. See healthcheck for more information.');
+            }
 
-        $fullBaseUrl = Configure::read('App.fullBaseUrl');
-        if (is_string($fullBaseUrl) && $fullBaseUrl !== '') {
             return $handler->handle($request);
         }
 
-        /** @var \Cake\Http\ServerRequest $request */
-        $hostHeader = $request->getHeader('Host');
-        if (empty($hostHeader)) {
+        if ($isValidFullBaseUrl) {
             return $handler->handle($request);
         }
 
-        throw new BadRequestException('Setting host header is not allowed when full base URL is not set.');
+        throw new InternalErrorException(__('The `{0}` configuration must be a valid non-empty string.', 'App.fullBaseUrl')); // phpcs:ignore
     }
 }
diff --git a/src/Service/Healthcheck/Core/FullBaseUrlCoreHealthcheck.php b/src/Service/Healthcheck/Core/FullBaseUrlCoreHealthcheck.php
@@ -31,12 +31,20 @@ class FullBaseUrlCoreHealthcheck implements HealthcheckServiceInterface, Healthc
      */
     private bool $status = false;
 
+    /**
+     * Type of full base url if not string.
+     *
+     * @var string
+     */
+    private string $fullBaseUrlType = '';
+
     /**
      * @inheritDoc
      */
     public function check(): HealthcheckServiceInterface
     {
         $this->status = (Configure::read('App.fullBaseUrl') !== null);
+        $this->fullBaseUrlType = gettype(Configure::read('App.fullBaseUrl'));
 
         return $this;
     }
@@ -70,7 +78,12 @@ public function level(): string
      */
     public function getSuccessMessage(): string
     {
-        return __('Full base url is set to {0}', Configure::read('App.fullBaseUrl'));
+        $fullBaseUrl = Configure::read('App.fullBaseUrl');
+        if (!is_string($fullBaseUrl)) {
+            $fullBaseUrl = sprintf('"%s"', $this->fullBaseUrlType);
+        }
+
+        return __('Full base url is set to {0}', $fullBaseUrl);
     }
 
     /**

diff --git a/src/Service/Healthcheck/Core/ValidFullBaseUrlCoreHealthcheck.php b/src/Service/Healthcheck/Core/ValidFullBaseUrlCoreHealthcheck.php
@@ -41,7 +41,12 @@ class ValidFullBaseUrlCoreHealthcheck implements HealthcheckServiceInterface, He
      */
     public function __construct($url = null)
     {
-        $this->url = $url ?? Configure::read('App.fullBaseUrl');
+        $fullBaseUrl = $url ?? Configure::read('App.fullBaseUrl');
+        if (!is_string($fullBaseUrl)) {
+            $fullBaseUrl = gettype($fullBaseUrl);
+        }
+
+        $this->url = $fullBaseUrl;
     }
 
     /**
@@ -94,7 +99,7 @@ public function getSuccessMessage(): string
      */
     public function getFailureMessage(): string
     {
-        return __('App.fullBaseUrl does not validate. {0}.', Configure::read('App.fullBaseUrl'));
+        return __('App.fullBaseUrl does not validate. A valid URL/IP is accepted, but found "{0}".', $this->url);
     }
 
     /**
@@ -105,6 +110,7 @@ public function getHelpMessage()
         return [
             __('Edit App.fullBaseUrl in {0}', CONFIG . 'passbolt.php'),
             __('Select a valid domain name as defined by section 2.3.1 of http://www.ietf.org/rfc/rfc1035.txt'),
+            __('IMPORTANT: Using an empty App.fullBaseUrl can lead to host header injection attack: https://owasp.org/www-project-web-security-testing-guide/v42/4-Web_Application_Security_Testing/07-Input_Validation_Testing/17-Testing_for_Host_Header_Injection'), // phpcs:ignore
         ];
     }
 

diff --git a/tests/TestCase/ApplicationTest.php b/tests/TestCase/ApplicationTest.php
@@ -18,12 +18,12 @@
 
 use App\Application;
 use App\Middleware\ApiVersionMiddleware;
+use App\Middleware\AssertFullBaseUrlMiddleware;
 use App\Middleware\ContainerInjectorMiddleware;
 use App\Middleware\ContentSecurityPolicyMiddleware;
 use App\Middleware\CsrfProtectionMiddleware;
 use App\Middleware\GpgAuthHeadersMiddleware;
 use App\Middleware\HttpProxyMiddleware;
-use App\Middleware\PreventHostHeaderFallbackMiddleware;
 use App\Middleware\SessionAuthPreventDeletedOrDisabledUsersMiddleware;
 use App\Middleware\SessionPreventExtensionMiddleware;
 use App\Middleware\SslForceMiddleware;
@@ -57,7 +57,7 @@ public function testApplication_Middleware()
             ValidCookieNameMiddleware::class,
             ContentSecurityPolicyMiddleware::class,
             ErrorHandlerMiddleware::class,
-            PreventHostHeaderFallbackMiddleware::class,
+            AssertFullBaseUrlMiddleware::class,
             SslForceMiddleware::class,
             AssetMiddleware::class,
             RoutingMiddleware::class,

diff --git a/tests/TestCase/Command/InstallCommandTest.php b/tests/TestCase/Command/InstallCommandTest.php
@@ -162,7 +162,7 @@ public function testInstallCommandForce_Will_Fail_If_BaseUrlIsNotValid()
         Configure::write('App.fullBaseUrl', 'foo');
         $this->exec('passbolt install --force -d test');
         $this->assertExitError();
-        $this->assertOutputContains('<error>App.fullBaseUrl does not validate. foo.</error>');
+        $this->assertOutputContains('<error>App.fullBaseUrl does not validate. A valid URL/IP is accepted, but found "foo".</error>');
     }
 
     /**

diff --git a/tests/TestCase/Middleware/AssertFullBaseUrlMiddlewareTest.php b/tests/TestCase/Middleware/AssertFullBaseUrlMiddlewareTest.php
@@ -0,0 +1,82 @@
+<?php
+declare(strict_types=1);
+
+/**
+ * Passbolt ~ Open source password manager for teams
+ * Copyright (c) Passbolt SA (https://www.passbolt.com)
+ *
+ * Licensed under GNU Affero General Public License version 3 of the or any later version.
+ * For full copyright and license information, please see the LICENSE.txt
+ * Redistributions of files must retain the above copyright notice.
+ *
+ * @copyright     Copyright (c) Passbolt SA (https://www.passbolt.com)
+ * @license       https://opensource.org/licenses/AGPL-3.0 AGPL License
+ * @link          https://www.passbolt.com Passbolt(tm)
+ * @since         4.11.1
+ */
+
+namespace App\Test\TestCase\Middleware;
+
+use App\Middleware\AssertFullBaseUrlMiddleware;
+use App\Test\Lib\AppIntegrationTestCase;
+use App\Test\Lib\Http\TestRequestHandler;
+use Cake\Core\Configure;
+use Cake\Http\Exception\InternalErrorException;
+use Cake\Http\Response;
+use Cake\Http\ServerRequest;
+
+/**
+ * @covers \App\Middleware\AssertFullBaseUrlMiddleware
+ */
+class AssertFullBaseUrlMiddlewareTest extends AppIntegrationTestCase
+{
+    /**
+     * @dataProvider invalidFullBaseUrlValuesProvider
+     * @param mixed $invalidFullBaseUrl Invalid values.
+     * @return void
+     */
+    public function testAssertFullBaseUrlMiddleware_DisallowInvalidFullBaseUrl_FlagIsTrue($invalidFullBaseUrl): void
+    {
+        Configure::write('passbolt.originalFullBaseUrl', $invalidFullBaseUrl);
+        Configure::write('passbolt.security.fullBaseUrlEnforce', true);
+
+        $this->expectException(InternalErrorException::class);
+
+        $middleware = new AssertFullBaseUrlMiddleware();
+        $middleware->process((new ServerRequest()), new TestRequestHandler());
+    }
+
+    public function invalidFullBaseUrlValuesProvider(): array
+    {
+        return [
+            [false],
+            [true],
+            [null],
+            [''],
+            [[]],
+            [new \stdClass()],
+        ];
+    }
+
+    public function testAssertFullBaseUrlMiddleware_ValidFullBaseUrl_FlagIsTrue(): void
+    {
+        Configure::write('passbolt.originalFullBaseUrl', 'https://passbolt.test');
+        Configure::write('passbolt.security.fullBaseUrlEnforce', true);
+
+        $middleware = new AssertFullBaseUrlMiddleware();
+        $response = $middleware->process((new ServerRequest()), new TestRequestHandler());
+
+        $this->assertInstanceOf(Response::class, $response);
+    }
+
+    public function testAssertFullBaseUrlMiddleware_AllowInvalidFullBaseUrl_FlagIsFalse(): void
+    {
+        Configure::write('passbolt.originalFullBaseUrl', false);
+        Configure::write('passbolt.security.fullBaseUrlEnforce', false);
+
+        $middleware = new AssertFullBaseUrlMiddleware();
+        $response = $middleware->process(new ServerRequest(), new TestRequestHandler());
+
+        $this->assertInstanceOf(Response::class, $response);
+    }
+}
diff --git a/tests/TestCase/Middleware/PreventHostHeaderFallbackMiddlewareTest.php b/tests/TestCase/Middleware/PreventHostHeaderFallbackMiddlewareTest.php