From 7303604550e90ba2e35134273db7bc7c1bb6523d Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Sat, 5 Jul 2025 12:01:50 +0200 Subject: [PATCH 1/9] Verify websocket origin --- src/main/php/web/handler/WebSocket.class.php | 42 ++++++++++++++++- .../unittest/handler/WebSocketTest.class.php | 45 +++++++++++++++++-- 2 files changed, 82 insertions(+), 5 deletions(-) diff --git a/src/main/php/web/handler/WebSocket.class.php b/src/main/php/web/handler/WebSocket.class.php index 8d3cdb1a..82f695f6 100755 --- a/src/main/php/web/handler/WebSocket.class.php +++ b/src/main/php/web/handler/WebSocket.class.php @@ -14,10 +14,44 @@ class WebSocket implements Handler { const GUID= '258EAFA5-E914-47DA-95CA-C5AB0DC85B11'; private $listener; + private $allowed= []; - /** @param function(websocket.protocol.Connection, string|util.Bytes): var|websocket.Listener $listener */ - public function __construct($listener) { + /** + * Creates a new websocket handler + * + * @param function(websocket.protocol.Connection, string|util.Bytes): var|websocket.Listener $listener + * @param string[] $origins + */ + public function __construct($listener, $origins= ['*']) { $this->listener= Listeners::cast($listener); + foreach ($origins as $allowed) { + $this->allowed[]= '#'.strtr(preg_quote($allowed), ['\\*' => '.+']).'#'; + } + } + + /** + * Verifies request `Origin` header matches the allowed origins. This + * header cannot be set by client-side JavaScript in browsers! + * + * @param web.Request $request + * @param web.Response $response + * @return bool + */ + public function verify($request, $response) { + static $ports= ['http' => 80, 'https' => 443]; + + if ($origin= $request->header('Origin')) { + $url= parse_url($origin); + $canonical= $url['scheme'].'://'.$url['host'].':'.($url['port'] ?? $ports[$url['scheme']]); + + foreach ($this->allowed as $pattern) { + if (preg_match($pattern, $canonical)) return true; + } + } + + $response->answer(403); + $response->send('Origin not allowed', 'text/plain'); + return false; } /** @@ -30,6 +64,8 @@ public function __construct($listener) { public function handle($request, $response) { switch ($version= (int)$request->header('Sec-WebSocket-Version')) { case 13: // RFC 6455 + if (!$this->verify($request, $response)) return; + $key= $request->header('Sec-WebSocket-Key'); $response->answer(101); $response->header('Sec-WebSocket-Accept', base64_encode(sha1($key.self::GUID, true))); @@ -39,6 +75,8 @@ public function handle($request, $response) { break; case 9: // Reserved version, use for WS <-> SSE translation + if (!$this->verify($request, $response)) return; + $response->answer(200); $response->header('Content-Type', 'text/event-stream'); $response->header('Transfer-Encoding', 'chunked'); diff --git a/src/test/php/web/unittest/handler/WebSocketTest.class.php b/src/test/php/web/unittest/handler/WebSocketTest.class.php index 392ec832..4c1b942e 100755 --- a/src/test/php/web/unittest/handler/WebSocketTest.class.php +++ b/src/test/php/web/unittest/handler/WebSocketTest.class.php @@ -1,6 +1,6 @@ send('Re: '.$payload); } }; - (new WebSocket($echo))->handle($request, $response)->next(); + (new WebSocket($echo, $origins))->handle($request, $response)->next(); return $response; } + /** @return iterable */ + private function origins() { + + // We allow all ports and schemes on localhost + yield ['http://localhost', 101]; + yield ['https://localhost', 101]; + yield ['http://localhost:8080', 101]; + yield ['https://localhost:8443', 101]; + + // Not allowed: localhost subdomains and unrelated domains + yield ['http://example.localhost', 403]; + yield ['http://localhost.example.com', 403]; + yield ['http://evil.example.com', 403]; + } + #[Test] public function can_create() { new WebSocket(function($conn, $payload) { }); @@ -30,6 +45,7 @@ public function can_create() { #[Test] public function switching_protocols() { $response= $this->handle(new Request(new TestInput('GET', '/ws', [ + 'Origin' => 'http://localhost:8080', 'Sec-WebSocket-Version' => 13, 'Sec-WebSocket-Key' => 'test', ]))); @@ -37,9 +53,31 @@ public function switching_protocols() { Assert::equals('tNpbgC8ZQDOcSkHAWopKzQjJ1hI=', $response->headers()['Sec-WebSocket-Accept']); } + #[Test] + public function missing_origin() { + $request= new Request(new TestInput('GET', '/ws', [ + 'Sec-WebSocket-Version' => 13, + 'Sec-WebSocket-Key' => 'test', + ])); + + Assert::equals(403, $this->handle($request)->status()); + } + + #[Test, Values(from: 'origins')] + public function verify_localhost_origin($origin, $expected) { + $request= new Request(new TestInput('GET', '/ws', [ + 'Origin' => $origin, + 'Sec-WebSocket-Version' => 13, + 'Sec-WebSocket-Key' => 'test', + ])); + + Assert::equals($expected, $this->handle($request, ['*://localhost:*'])->status()); + } + #[Test] public function translate_text_message() { $response= $this->handle(new Request(new TestInput('POST', '/ws', [ + 'Origin' => 'http://localhost:8080', 'Sec-WebSocket-Version' => 9, 'Sec-WebSocket-Id' => 123, 'Content-Type' => 'text/plain', @@ -53,6 +91,7 @@ public function translate_text_message() { #[Test] public function translate_binary_message() { $response= $this->handle(new Request(new TestInput('POST', '/ws', [ + 'Origin' => 'http://localhost:8080', 'Sec-WebSocket-Version' => 9, 'Sec-WebSocket-Id' => 123, 'Content-Type' => 'application/octet-stream', From e77ec8281fc032facc3e540feadfdc94841c4136 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Sat, 5 Jul 2025 12:10:14 +0200 Subject: [PATCH 2/9] Pass origin header for websocket integration tests --- src/it/php/web/unittest/IntegrationTest.class.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/it/php/web/unittest/IntegrationTest.class.php b/src/it/php/web/unittest/IntegrationTest.class.php index 3096af75..4486d936 100755 --- a/src/it/php/web/unittest/IntegrationTest.class.php +++ b/src/it/php/web/unittest/IntegrationTest.class.php @@ -170,7 +170,7 @@ public function with_large_cookie($length) { public function websocket_message($input, $output) { try { $ws= new WebSocket($this->server->connection, '/ws'); - $ws->connect(); + $ws->connect(['Origin' => 'http://localhost']); $ws->send($input); $result= $ws->receive(); } finally { @@ -183,7 +183,7 @@ public function websocket_message($input, $output) { public function invalid_utf8_passed_to_websocket_text_message() { try { $ws= new WebSocket($this->server->connection, '/ws'); - $ws->connect(); + $ws->connect(['Origin' => 'http://localhost']); $ws->send("\xfc"); $ws->receive(); } finally { From 3501edcbdfd56a74dd88b22831094f0b94714b89 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Sat, 5 Jul 2025 12:31:18 +0200 Subject: [PATCH 3/9] Implement same-origin policy by default See https://github.com/xp-forge/web/pull/126#pullrequestreview-2989729906 --- src/main/php/web/handler/WebSocket.class.php | 28 +++++++++++++------ .../unittest/handler/WebSocketTest.class.php | 27 ++++++++++++++++++ 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/src/main/php/web/handler/WebSocket.class.php b/src/main/php/web/handler/WebSocket.class.php index 82f695f6..86ac3a30 100755 --- a/src/main/php/web/handler/WebSocket.class.php +++ b/src/main/php/web/handler/WebSocket.class.php @@ -1,5 +1,6 @@ listener= Listeners::cast($listener); foreach ($origins as $allowed) { - $this->allowed[]= '#'.strtr(preg_quote($allowed), ['\\*' => '.+']).'#'; + $this->allowed[]= '#'.strtr(preg_quote($allowed, '#'), ['\\*' => '.+']).'#i'; } } + /** + * Returns canonicalized base URI + * + * @param util.URI $uri + * @return string + */ + private function base($uri) { + static $ports= ['http' => 80, 'https' => 443]; + + return $uri->scheme().'://'.$uri->host().':'.($uri->port() ?? $ports[$uri->scheme()] ?? 0); + } + /** * Verifies request `Origin` header matches the allowed origins. This * header cannot be set by client-side JavaScript in browsers! @@ -38,15 +51,14 @@ public function __construct($listener, $origins= ['*']) { * @return bool */ public function verify($request, $response) { - static $ports= ['http' => 80, 'https' => 443]; - if ($origin= $request->header('Origin')) { - $url= parse_url($origin); - $canonical= $url['scheme'].'://'.$url['host'].':'.($url['port'] ?? $ports[$url['scheme']]); - + $base= $this->base(new URI($origin)); foreach ($this->allowed as $pattern) { - if (preg_match($pattern, $canonical)) return true; + if (preg_match($pattern, $base)) return true; } + + // Same-origin policy + if (0 === strcasecmp($this->base($request->uri()), $base)) return true; } $response->answer(403); diff --git a/src/test/php/web/unittest/handler/WebSocketTest.class.php b/src/test/php/web/unittest/handler/WebSocketTest.class.php index 4c1b942e..3c7538d4 100755 --- a/src/test/php/web/unittest/handler/WebSocketTest.class.php +++ b/src/test/php/web/unittest/handler/WebSocketTest.class.php @@ -22,6 +22,19 @@ private function handle(Request $request, $origins= ['*']): Response { return $response; } + /** @return iterable */ + private function same() { + + // By default, enforces same-origin policy + yield ['http://localhost:80', 101]; + yield ['http://localhost', 101]; + yield ['http://Localhost', 101]; + + // Not allowed: Differing ports or scheme + yield ['http://localhost:81', 403]; + yield ['https://localhost', 403]; + } + /** @return iterable */ private function origins() { @@ -30,6 +43,7 @@ private function origins() { yield ['https://localhost', 101]; yield ['http://localhost:8080', 101]; yield ['https://localhost:8443', 101]; + yield ['http://Localhost', 101]; // Not allowed: localhost subdomains and unrelated domains yield ['http://example.localhost', 403]; @@ -63,10 +77,23 @@ public function missing_origin() { Assert::equals(403, $this->handle($request)->status()); } + #[Test, Values(from: 'same')] + public function verify_same_origin($origin, $expected) { + $request= new Request(new TestInput('GET', '/ws', [ + 'Origin' => $origin, + 'Host' => 'localhost:80', + 'Sec-WebSocket-Version' => 13, + 'Sec-WebSocket-Key' => 'test', + ])); + + Assert::equals($expected, $this->handle($request, [])->status()); + } + #[Test, Values(from: 'origins')] public function verify_localhost_origin($origin, $expected) { $request= new Request(new TestInput('GET', '/ws', [ 'Origin' => $origin, + 'Host' => 'localhost:8080', 'Sec-WebSocket-Version' => 13, 'Sec-WebSocket-Key' => 'test', ])); From f792299a072d4ab143542b53a784f32977789158 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Sat, 5 Jul 2025 12:35:01 +0200 Subject: [PATCH 4/9] Pass host header --- src/it/php/web/unittest/IntegrationTest.class.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/it/php/web/unittest/IntegrationTest.class.php b/src/it/php/web/unittest/IntegrationTest.class.php index 4486d936..945d0e6e 100755 --- a/src/it/php/web/unittest/IntegrationTest.class.php +++ b/src/it/php/web/unittest/IntegrationTest.class.php @@ -170,7 +170,7 @@ public function with_large_cookie($length) { public function websocket_message($input, $output) { try { $ws= new WebSocket($this->server->connection, '/ws'); - $ws->connect(['Origin' => 'http://localhost']); + $ws->connect(['Origin' => 'http://localhost', 'Host' => 'localhost:80']); $ws->send($input); $result= $ws->receive(); } finally { @@ -183,7 +183,7 @@ public function websocket_message($input, $output) { public function invalid_utf8_passed_to_websocket_text_message() { try { $ws= new WebSocket($this->server->connection, '/ws'); - $ws->connect(['Origin' => 'http://localhost']); + $ws->connect(['Origin' => 'http://localhost', 'Host' => 'localhost:80']); $ws->send("\xfc"); $ws->receive(); } finally { From 34f82cdb3cc3dec9df5dd417dd55346340b6eee2 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Sat, 5 Jul 2025 12:38:04 +0200 Subject: [PATCH 5/9] Use array type hint --- src/main/php/web/handler/WebSocket.class.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/php/web/handler/WebSocket.class.php b/src/main/php/web/handler/WebSocket.class.php index 86ac3a30..05935210 100755 --- a/src/main/php/web/handler/WebSocket.class.php +++ b/src/main/php/web/handler/WebSocket.class.php @@ -23,7 +23,7 @@ class WebSocket implements Handler { * @param function(websocket.protocol.Connection, string|util.Bytes): var|websocket.Listener $listener * @param string[] $origins */ - public function __construct($listener, $origins= []) { + public function __construct($listener, array $origins= []) { $this->listener= Listeners::cast($listener); foreach ($origins as $allowed) { $this->allowed[]= '#'.strtr(preg_quote($allowed, '#'), ['\\*' => '.+']).'#i'; From d6cc0dd7ea3301ed1e7c5bb7b62c04cac66d5742 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Sat, 5 Jul 2025 12:38:17 +0200 Subject: [PATCH 6/9] Add more tests --- src/test/php/web/unittest/handler/WebSocketTest.class.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/php/web/unittest/handler/WebSocketTest.class.php b/src/test/php/web/unittest/handler/WebSocketTest.class.php index 3c7538d4..d5f569fd 100755 --- a/src/test/php/web/unittest/handler/WebSocketTest.class.php +++ b/src/test/php/web/unittest/handler/WebSocketTest.class.php @@ -30,8 +30,10 @@ private function same() { yield ['http://localhost', 101]; yield ['http://Localhost', 101]; - // Not allowed: Differing ports or scheme + // Not allowed: Differing hosts, ports or scheme yield ['http://localhost:81', 403]; + yield ['http://example.localhost', 403]; + yield ['http://localhost.example.com', 403]; yield ['https://localhost', 403]; } From ba6381d734c15dd04cc90ee30b19748056b2388d Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Sat, 5 Jul 2025 12:43:14 +0200 Subject: [PATCH 7/9] QA: Simplify code in WebSocket handler --- src/main/php/web/handler/WebSocket.class.php | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/main/php/web/handler/WebSocket.class.php b/src/main/php/web/handler/WebSocket.class.php index 05935210..7222cd6d 100755 --- a/src/main/php/web/handler/WebSocket.class.php +++ b/src/main/php/web/handler/WebSocket.class.php @@ -84,7 +84,14 @@ public function handle($request, $response) { foreach ($this->listener->protocols ?? [] as $protocol) { $response->header('Sec-WebSocket-Protocol', $protocol, true); } - break; + + // Signal server implementation to switch protocols + yield 'connection' => ['websocket', [ + 'path' => $request->uri()->resource(), + 'headers' => $request->headers(), + 'listener' => $this->listener, + ]]; + return; case 9: // Reserved version, use for WS <-> SSE translation if (!$this->verify($request, $response)) return; @@ -110,11 +117,5 @@ public function handle($request, $response) { $response->send('This service does not support WebSocket version '.$version, 'text/plain'); return; } - - yield 'connection' => ['websocket', [ - 'path' => $request->uri()->resource(), - 'headers' => $request->headers(), - 'listener' => $this->listener, - ]]; } } \ No newline at end of file From eaf00432e5de8bed540bbda85107762a6bca386a Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Sat, 5 Jul 2025 12:58:37 +0200 Subject: [PATCH 8/9] Verify subdomains --- src/test/php/web/unittest/handler/WebSocketTest.class.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/php/web/unittest/handler/WebSocketTest.class.php b/src/test/php/web/unittest/handler/WebSocketTest.class.php index d5f569fd..939aa45f 100755 --- a/src/test/php/web/unittest/handler/WebSocketTest.class.php +++ b/src/test/php/web/unittest/handler/WebSocketTest.class.php @@ -46,8 +46,9 @@ private function origins() { yield ['http://localhost:8080', 101]; yield ['https://localhost:8443', 101]; yield ['http://Localhost', 101]; + yield ['http://api.localhost', 101]; - // Not allowed: localhost subdomains and unrelated domains + // Not allowed: Other localhost subdomains and unrelated domains yield ['http://example.localhost', 403]; yield ['http://localhost.example.com', 403]; yield ['http://evil.example.com', 403]; @@ -100,7 +101,7 @@ public function verify_localhost_origin($origin, $expected) { 'Sec-WebSocket-Key' => 'test', ])); - Assert::equals($expected, $this->handle($request, ['*://localhost:*'])->status()); + Assert::equals($expected, $this->handle($request, ['*://localhost:*', '*://api.localhost:*'])->status()); } #[Test] From 1c5378927f8e3d4a7b87bcf17d325b7577fa43e0 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Sat, 5 Jul 2025 14:24:37 +0200 Subject: [PATCH 9/9] Do not allow substring matches --- src/main/php/web/handler/WebSocket.class.php | 2 +- src/test/php/web/unittest/handler/WebSocketTest.class.php | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/main/php/web/handler/WebSocket.class.php b/src/main/php/web/handler/WebSocket.class.php index 7222cd6d..43be3610 100755 --- a/src/main/php/web/handler/WebSocket.class.php +++ b/src/main/php/web/handler/WebSocket.class.php @@ -26,7 +26,7 @@ class WebSocket implements Handler { public function __construct($listener, array $origins= []) { $this->listener= Listeners::cast($listener); foreach ($origins as $allowed) { - $this->allowed[]= '#'.strtr(preg_quote($allowed, '#'), ['\\*' => '.+']).'#i'; + $this->allowed[]= '#^'.strtr(preg_quote($allowed, '#'), ['\\*' => '.+']).'$#i'; } } diff --git a/src/test/php/web/unittest/handler/WebSocketTest.class.php b/src/test/php/web/unittest/handler/WebSocketTest.class.php index 939aa45f..8b2ca030 100755 --- a/src/test/php/web/unittest/handler/WebSocketTest.class.php +++ b/src/test/php/web/unittest/handler/WebSocketTest.class.php @@ -35,6 +35,9 @@ private function same() { yield ['http://example.localhost', 403]; yield ['http://localhost.example.com', 403]; yield ['https://localhost', 403]; + yield ['http://localhost:81@evil.example.com', 403]; + yield ['http://evil.example.com/#http://localhost', 403]; + yield ['http://evil.example.com/?page=http://localhost', 403]; } /** @return iterable */ @@ -51,7 +54,9 @@ private function origins() { // Not allowed: Other localhost subdomains and unrelated domains yield ['http://example.localhost', 403]; yield ['http://localhost.example.com', 403]; - yield ['http://evil.example.com', 403]; + yield ['http://localhost:81@evil.example.com', 403]; + yield ['http://evil.example.com/#http://localhost', 403]; + yield ['http://evil.example.com/?page=http://localhost', 403]; } #[Test]