From 8fb9b125d31c8f4cc4320d860658f6ae371f4918 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 28 Jul 2022 14:19:06 -0500 Subject: [PATCH 1/6] fix: multiple slash in path detection Per an [issue created for Diactoros](https://github.com/laminas/laminas-diactoros/issues/74) its [related pull request](https://github.com/laminas/laminas-diactoros/pull/77), and the discussion to that pull request, this patch does the following: - It modifies `testPathWithMultipleSlashes()` to only validate that multiple slashes _not at the beginning_ of a path are retained intact. - It adds `testProperlyTrimsLeadingSlashesToPreventXSS()`, which validates that when multiple leading slashes are present in a path, they are reduced to a single slash. This approach is done to mitigate [ZF2015-05](https://framework.zend.com/security/advisory/ZF2015-05.html) which was also reported as [CVE-2015-3257](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-3257). While RFC 3986 allows for multiple slashes anywhere in the path, when security conflicts with a specification, security concerns win. Without the mitigation, an implementation is vulnerable to XSS and open redirects if only the path portion of a URI is used within HTML content (common!) or within headers (also common). Signed-off-by: Matthew Weier O'Phinney --- src/UriIntegrationTest.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/UriIntegrationTest.php b/src/UriIntegrationTest.php index a30eba6..068f647 100644 --- a/src/UriIntegrationTest.php +++ b/src/UriIntegrationTest.php @@ -238,6 +238,13 @@ public function testPathWithMultipleSlashes() $this->assertInstanceOf(UriInterface::class, $uri); $this->assertSame($expected, (string) $uri); - $this->assertSame('//valid///path', $uri->getPath()); + $this->assertSame('/valid///path', $uri->getPath()); + } + + public function testProperlyTrimsLeadingSlashesToPreventXSS() + { + $url = 'http://example.org//zend.com'; + $uri = $this->createUri($url); + $this->assertSame('http://example.org/zend.com', (string) $uri); } } From 83bfa3938c417c80e1c9e549ad1d65506ca08f7e Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Tue, 22 Nov 2022 16:01:46 -0600 Subject: [PATCH 2/6] fix: Split multiple slashes tests into three - `testPathWithMultipleSlashes()` modified to have a path with multiple slashes only in the middle of the path. - Renames `testProperlyTrimsLeadingSlashesToPreventXSS()` to `testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS()`, and has it assert only against results of `getPath()`; on completion, it returns an associative array with the expected URI string and the URI instance. - Adds `testStringRepresentationWithMultipleSlashes()`; depends on `testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS()`, and asserts that the string representation is identical to the original URI string. Signed-off-by: Matthew Weier O'Phinney --- src/UriIntegrationTest.php | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/src/UriIntegrationTest.php b/src/UriIntegrationTest.php index 068f647..9ba4612 100644 --- a/src/UriIntegrationTest.php +++ b/src/UriIntegrationTest.php @@ -233,18 +233,38 @@ public function testPathWithMultipleSlashes() $this->markTestSkipped($this->skippedTests[__FUNCTION__]); } - $expected = 'http://example.org//valid///path'; + $expected = 'http://example.org/valid///path'; $uri = $this->createUri($expected); $this->assertInstanceOf(UriInterface::class, $uri); + $this->assertSame('/valid///path', $uri->getPath()); $this->assertSame($expected, (string) $uri); + } + + public function testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS() + { + if (isset($this->skippedTests[__FUNCTION__])) { + $this->markTestSkipped($this->skippedTests[__FUNCTION__]); + } + + $expected = 'http://example.org//valid///path'; + $uri = $this->createUri($expected); + + $this->assertInstanceOf(UriInterface::class, $uri); $this->assertSame('/valid///path', $uri->getPath()); + + return [ + 'expected' => $expected, + 'uri' => $uri, + ]; } - public function testProperlyTrimsLeadingSlashesToPreventXSS() + /** + * @depends testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS + * @psalm-param array{expected: non-empty-string, uri: UriInterface} $test + */ + public function testStringRepresentationWithMultipleSlashes(array $test) { - $url = 'http://example.org//zend.com'; - $uri = $this->createUri($url); - $this->assertSame('http://example.org/zend.com', (string) $uri); + $this->assertSame($test['expected'], (string) $test['uri']); } } From 13ef0b2490bb850713545a5dd302006f513c0edb Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Tue, 22 Nov 2022 16:14:27 -0600 Subject: [PATCH 3/6] fix: adds test for RequestInterface::getRequestTarget origin-form Adds RequestIntegrationTest::testGetRequestTargetInOriginFormNormalizesUriWithMultipleLeadingSlashesInPath(), which verifies that calling `getRequestTarget()` with a URI that contains a path with multiple leading slashes normalizes those slases to a single leading slash, in order to prevent XSS attacks. Signed-off-by: Matthew Weier O'Phinney --- src/RequestIntegrationTest.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/RequestIntegrationTest.php b/src/RequestIntegrationTest.php index 2695a00..102443c 100644 --- a/src/RequestIntegrationTest.php +++ b/src/RequestIntegrationTest.php @@ -169,4 +169,20 @@ public function testUriPreserveHost_Host_Host() $request2 = $request->withUri($this->buildUri('http://www.bar.com/foo'), true); $this->assertEquals($host, $request2->getHeaderLine('host')); } + + /** + * @see UriIntegrationTest::testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS + */ + public function testGetRequestTargetInOriginFormNormalizesUriWithMultipleLeadingSlashesInPath() + { + if (isset($this->skippedTests[__FUNCTION__])) { + $this->markTestSkipped($this->skippedTests[__FUNCTION__]); + } + + $url = 'http://example.org//valid///path'; + $request = $this->request->withUri($this->buildUri($url)); + $requestTarget = $request->getRequestTarget(); + + $this->assertSame('/valid///path', $requestTarget); + } } From 9622b8b7e71ce471e1533f91111fee0867ebfd16 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Tue, 22 Nov 2022 16:17:03 -0600 Subject: [PATCH 4/6] qa: fixes CS issue flagged by circle-ci Signed-off-by: Matthew Weier O'Phinney --- src/UriIntegrationTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/UriIntegrationTest.php b/src/UriIntegrationTest.php index 9ba4612..c5d93de 100644 --- a/src/UriIntegrationTest.php +++ b/src/UriIntegrationTest.php @@ -255,7 +255,7 @@ public function testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreven return [ 'expected' => $expected, - 'uri' => $uri, + 'uri' => $uri, ]; } From 98fff9cb382752f5fcffd8d1c997e594aa39a171 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Wed, 30 Nov 2022 11:56:35 -0600 Subject: [PATCH 5/6] docs: Documents the purpose behind the newly added tests - Documents the CVE that led to the new tests. - Documents the differences in behavior between using UriInterface::getPath(), UriInterface::__toString(), and RequestInterface::getRequestTarget(). --- src/RequestIntegrationTest.php | 5 +++++ src/UriIntegrationTest.php | 12 ++++++++++++ 2 files changed, 17 insertions(+) diff --git a/src/RequestIntegrationTest.php b/src/RequestIntegrationTest.php index 102443c..cd514eb 100644 --- a/src/RequestIntegrationTest.php +++ b/src/RequestIntegrationTest.php @@ -171,6 +171,11 @@ public function testUriPreserveHost_Host_Host() } /** + * Tests that getRequestTarget(), when using the default behavior of + * displaying the origin-form, normalizes multiple leading slashes in the + * path to a single slash. This is done to prevent URL poisoning and/or XSS + * issues. + * * @see UriIntegrationTest::testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS */ public function testGetRequestTargetInOriginFormNormalizesUriWithMultipleLeadingSlashesInPath() diff --git a/src/UriIntegrationTest.php b/src/UriIntegrationTest.php index c5d93de..ee998e0 100644 --- a/src/UriIntegrationTest.php +++ b/src/UriIntegrationTest.php @@ -241,6 +241,14 @@ public function testPathWithMultipleSlashes() $this->assertSame($expected, (string) $uri); } + /** + * Tests that getPath() normalizes multiple leading slashes to a single + * slash. This is done to ensure that when a path is used in isolation from + * the authority, it will not cause URL poisoning and/or XSS issues. + * + * @see https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-3257 + * @psalm-param array{expected: non-empty-string, uri: UriInterface} $test + */ public function testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS() { if (isset($this->skippedTests[__FUNCTION__])) { @@ -260,6 +268,10 @@ public function testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreven } /** + * Tests that the full string representation of a URI that includes multiple + * leading slashes in the path is presented verbatim (in contrast to what is + * provided when calling getPath()). + * * @depends testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS * @psalm-param array{expected: non-empty-string, uri: UriInterface} $test */ From 98fda81c65d9e51d43c58e743c7a605fbf007c4f Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Wed, 30 Nov 2022 12:02:51 -0600 Subject: [PATCH 6/6] docs: Adds CHANGELOG entries detailing new and modified tests --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 420e6f2..4855fd6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1 +1,12 @@ # Change Log + +## Added + +- Adds `UriIntegrationTest::testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS()`, `UriIntegrationTest::testStringRepresentationWithMultipleSlashes(array $test)`, and `RequestIntegrationTest::testGetRequestTargetInOriginFormNormalizesUriWithMultipleLeadingSlashesInPath()`. + These validate that a path containing multiple leading slashes is (a) represented with a single slash when calling `UriInterface::getPath()`, and (b) represented without changes when calling `UriInterface::__toString()`, including when calling `RequestInterface::getRequestTarget()` (which returns the path without the URI authority by default, to comply with origin-form). + This is done to validate mitigations for [CVE-2015-3257](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-3257). + +## Changed + +- Modifies `UriIntegrationTest::testPathWithMultipleSlashes()` to only validate multiple slashes in the middle of a path. + Multiple leading slashes are covered with the newly introduced tests.