From c324ad928dbdd54ce5b09eb0dabe60ef9de1ea38 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 26 Aug 2023 15:28:29 +0100 Subject: [PATCH 1/2] Security: Added new SSR allow list and validator Included unit tests to cover validator functionality. Added to webhooks. Still need to do testing specifically for webhooks. --- app/Activity/DispatchWebhookJob.php | 3 ++ app/Config/app.php | 9 ++++ app/Util/SsrUrlValidator.php | 64 +++++++++++++++++++++++++++++ lang/en/errors.php | 2 + tests/Unit/SsrUrlValidatorTest.php | 59 ++++++++++++++++++++++++++ 5 files changed, 137 insertions(+) create mode 100644 app/Util/SsrUrlValidator.php create mode 100644 tests/Unit/SsrUrlValidatorTest.php diff --git a/app/Activity/DispatchWebhookJob.php b/app/Activity/DispatchWebhookJob.php index f2330c4fa..405bca49c 100644 --- a/app/Activity/DispatchWebhookJob.php +++ b/app/Activity/DispatchWebhookJob.php @@ -8,6 +8,7 @@ use BookStack\Activity\Tools\WebhookFormatter; use BookStack\Facades\Theme; use BookStack\Theming\ThemeEvents; use BookStack\Users\Models\User; +use BookStack\Util\SsrUrlValidator; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; @@ -53,6 +54,8 @@ class DispatchWebhookJob implements ShouldQueue $lastError = null; try { + (new SsrUrlValidator())->ensureAllowed($this->webhook->endpoint); + $response = Http::asJson() ->withOptions(['allow_redirects' => ['strict' => true]]) ->timeout($this->webhook->timeout) diff --git a/app/Config/app.php b/app/Config/app.php index 29ab9c6dc..3a843c512 100644 --- a/app/Config/app.php +++ b/app/Config/app.php @@ -66,6 +66,15 @@ return [ // Current host and source for the "DRAWIO" setting will be auto-appended to the sources configured. 'iframe_sources' => env('ALLOWED_IFRAME_SOURCES', 'https://*.draw.io https://*.youtube.com https://*.youtube-nocookie.com https://*.vimeo.com'), + // A list of the sources/hostnames that can be reached by application SSR calls. + // This is used wherever users can provide URLs/hosts in-platform, like for webhooks. + // Host-specific functionality (usually controlled via other options) like auth + // or user avatars for example, won't use this list. + // Space seperated if multiple. Can use '*' as a wildcard. + // Values will be compared prefix-matched, case-insensitive, against called SSR urls. + // Defaults to allow all hosts. + 'ssr_hosts' => env('ALLOWED_SSR_HOSTS', '*'), + // Alter the precision of IP addresses stored by BookStack. // Integer value between 0 (IP hidden) to 4 (Full IP usage) 'ip_address_precision' => env('IP_ADDRESS_PRECISION', 4), diff --git a/app/Util/SsrUrlValidator.php b/app/Util/SsrUrlValidator.php new file mode 100644 index 000000000..722a45f7b --- /dev/null +++ b/app/Util/SsrUrlValidator.php @@ -0,0 +1,64 @@ +config = $config ?? config('app.ssr_hosts') ?? ''; + } + + /** + * @throws HttpFetchException + */ + public function ensureAllowed(string $url): void + { + if (!$this->allowed($url)) { + throw new HttpFetchException(trans('errors.http_ssr_url_no_match')); + } + } + + /** + * Check if the given URL is allowed by the configured SSR host values. + */ + public function allowed(string $url): bool + { + $allowed = $this->getHostPatterns(); + + foreach ($allowed as $pattern) { + if ($this->urlMatchesPattern($url, $pattern)) { + return true; + } + } + + return false; + } + + protected function urlMatchesPattern($url, $pattern): bool + { + $pattern = trim($pattern); + $url = trim($url); + + if (empty($pattern) || empty($url)) { + return false; + } + + $quoted = preg_quote($pattern, '/'); + $regexPattern = str_replace('\*', '.*', $quoted); + + return preg_match('/^' . $regexPattern . '.*$/i', $url); + } + + /** + * @return string[] + */ + protected function getHostPatterns(): array + { + return explode(' ', strtolower($this->config)); + } +} diff --git a/lang/en/errors.php b/lang/en/errors.php index 23c326f9e..4cde4cea3 100644 --- a/lang/en/errors.php +++ b/lang/en/errors.php @@ -111,4 +111,6 @@ return [ // Settings & Maintenance 'maintenance_test_email_failure' => 'Error thrown when sending a test email:', + // HTTP errors + 'http_ssr_url_no_match' => 'The URL does not match the configured allowed SSR hosts', ]; diff --git a/tests/Unit/SsrUrlValidatorTest.php b/tests/Unit/SsrUrlValidatorTest.php new file mode 100644 index 000000000..1443eedd7 --- /dev/null +++ b/tests/Unit/SsrUrlValidatorTest.php @@ -0,0 +1,59 @@ + '', 'url' => '', 'result' => false], + ['config' => '', 'url' => 'https://example.com', 'result' => false], + ['config' => ' ', 'url' => 'https://example.com', 'result' => false], + ['config' => '*', 'url' => '', 'result' => false], + ['config' => '*', 'url' => 'https://example.com', 'result' => true], + ['config' => 'https://*', 'url' => 'https://example.com', 'result' => true], + ['config' => 'http://*', 'url' => 'https://example.com', 'result' => false], + ['config' => 'https://*example.com', 'url' => 'https://example.com', 'result' => true], + ['config' => 'https://*ample.com', 'url' => 'https://example.com', 'result' => true], + ['config' => 'https://*.example.com', 'url' => 'https://example.com', 'result' => false], + ['config' => 'https://*.example.com', 'url' => 'https://test.example.com', 'result' => true], + ['config' => '*//example.com', 'url' => 'https://example.com', 'result' => true], + ['config' => '*//example.com', 'url' => 'http://example.com', 'result' => true], + ['config' => 'https://example.com', 'url' => 'https://example.com/a/b/c?test=cat', 'result' => true], + ['config' => 'https://example.com', 'url' => 'https://example.co.uk', 'result' => false], + + // Escapes + ['config' => 'https://(.*?).com', 'url' => 'https://example.com', 'result' => false], + ['config' => 'https://example.com', 'url' => 'https://example.co.uk#https://example.com', 'result' => false], + + // Multi values + ['config' => '*//example.org *//example.com', 'url' => 'https://example.com', 'result' => true], + ['config' => '*//example.org *//example.com', 'url' => 'https://example.com/a/b/c?test=cat#hello', 'result' => true], + ['config' => '*.example.org *.example.com', 'url' => 'https://example.co.uk', 'result' => false], + ['config' => ' *.example.org *.example.com ', 'url' => 'https://example.co.uk', 'result' => false], + ['config' => '* *.example.com', 'url' => 'https://example.co.uk', 'result' => true], + ['config' => '*//example.org *//example.com *//example.co.uk', 'url' => 'https://example.co.uk', 'result' => true], + ['config' => '*//example.org *//example.com *//example.co.uk', 'url' => 'https://example.net', 'result' => false], + ]; + + foreach ($testMap as $test) { + $result = (new SsrUrlValidator($test['config']))->allowed($test['url']); + $this->assertEquals($test['result'], $result, "Failed asserting url '{$test['url']}' with config '{$test['config']}' results " . ($test['result'] ? 'true' : 'false')); + } + } + + public function test_enssure_allowed() + { + $result = (new SsrUrlValidator('https://example.com'))->ensureAllowed('https://example.com'); + $this->assertNull($result); + + $this->expectException(HttpFetchException::class); + (new SsrUrlValidator('https://example.com'))->ensureAllowed('https://test.example.com'); + } +} From 903895814ace77cd30c22b57de16f9e22daf21e4 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 26 Aug 2023 20:13:37 +0100 Subject: [PATCH 2/2] SSR: Updated allow list handling & covered webhook usage - Covered webhook SSR allow list useage via test. - Updated allow list handling to use trailing slash, or hash, or end of line as late anchor for better handling for hosts (prevent .co.uk passing for .co domain host) --- app/Util/SsrUrlValidator.php | 4 ++-- tests/Actions/WebhookCallTest.php | 14 ++++++++++++++ tests/Unit/SsrUrlValidatorTest.php | 3 +++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/app/Util/SsrUrlValidator.php b/app/Util/SsrUrlValidator.php index 722a45f7b..0b3a6a31d 100644 --- a/app/Util/SsrUrlValidator.php +++ b/app/Util/SsrUrlValidator.php @@ -41,7 +41,7 @@ class SsrUrlValidator protected function urlMatchesPattern($url, $pattern): bool { - $pattern = trim($pattern); + $pattern = rtrim(trim($pattern), '/'); $url = trim($url); if (empty($pattern) || empty($url)) { @@ -51,7 +51,7 @@ class SsrUrlValidator $quoted = preg_quote($pattern, '/'); $regexPattern = str_replace('\*', '.*', $quoted); - return preg_match('/^' . $regexPattern . '.*$/i', $url); + return preg_match('/^' . $regexPattern . '($|\/.*$|#.*$)/i', $url); } /** diff --git a/tests/Actions/WebhookCallTest.php b/tests/Actions/WebhookCallTest.php index fc49a524e..0746aa3a1 100644 --- a/tests/Actions/WebhookCallTest.php +++ b/tests/Actions/WebhookCallTest.php @@ -101,6 +101,20 @@ class WebhookCallTest extends TestCase $this->assertNotNull($webhook->last_errored_at); } + public function test_webhook_uses_ssr_hosts_option_if_set() + { + config()->set('app.ssr_hosts', 'https://*.example.com'); + $http = Http::fake(); + + $webhook = $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.co.uk'], ['all']); + $this->runEvent(ActivityType::ROLE_CREATE); + $http->assertNothingSent(); + + $webhook->refresh(); + $this->assertEquals('The URL does not match the configured allowed SSR hosts', $webhook->last_error); + $this->assertNotNull($webhook->last_errored_at); + } + public function test_webhook_call_data_format() { Http::fake([ diff --git a/tests/Unit/SsrUrlValidatorTest.php b/tests/Unit/SsrUrlValidatorTest.php index 1443eedd7..8fb538916 100644 --- a/tests/Unit/SsrUrlValidatorTest.php +++ b/tests/Unit/SsrUrlValidatorTest.php @@ -25,6 +25,9 @@ class SsrUrlValidatorTest extends TestCase ['config' => 'https://*.example.com', 'url' => 'https://test.example.com', 'result' => true], ['config' => '*//example.com', 'url' => 'https://example.com', 'result' => true], ['config' => '*//example.com', 'url' => 'http://example.com', 'result' => true], + ['config' => '*//example.co', 'url' => 'http://example.co.uk', 'result' => false], + ['config' => '*//example.co/bookstack', 'url' => 'https://example.co/bookstack/a/path', 'result' => true], + ['config' => '*//example.co*', 'url' => 'https://example.co.uk/bookstack/a/path', 'result' => true], ['config' => 'https://example.com', 'url' => 'https://example.com/a/b/c?test=cat', 'result' => true], ['config' => 'https://example.com', 'url' => 'https://example.co.uk', 'result' => false],