Modify multiple slash in URI path detection #54
Conversation
Per an [issue created for Diactoros](laminas/laminas-diactoros#74) its [related pull request](laminas/laminas-diactoros#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 <matthew@weierophinney.net>
src/UriIntegrationTest.php
Outdated
| $this->assertSame('/valid///path', $uri->getPath()); | ||
| } | ||
|
|
||
| public function testProperlyTrimsLeadingSlashesToPreventXSS() |
There was a problem hiding this comment.
should we add a comment here with the reference to the vulnerability warnings?
you did a very good commit message explaining the change, but this seems relevant enough to also have a doc comment explaining why we want this.
|
I have a genuine question about this PR. If I understand correctly the issue is not when using or calling so the correct fix should be to apply the fix on the If we follow RFC3986 if the authority and the scheme is removed an URI will be invalid if it starts with a path with multiple TL;DR: Using a UriFactory class I believe the following should be true for all implementing URI class: $uri = $uriFactory->createUri('http://example.com///google.com');
echo $uri, PHP_EOL; // returns 'http://example.com///google.com'
echo $uri->getPath(); // returns '/google.com'; (multiple leading slashes strip for security
//and
$uri->withScheme('')->withHost(''); // should throw and InvalidArgumentException because
//If there is no authority the path `///google.com` can not start with a `//`Is my interpretation correct ? |
|
@nyamsprod I don't see why |
|
@kelunik the issue is that Most HTTP Clients that I see when redirecting on the same base URI will only do echo "GET ". $uri->getPath(); //pseudo codeRFC3986 allows: IMO to avoid redirecting the client to the wrong website you only need to strip multiple leading slashes on Removing the scheme and the host is already cover by the RFC which states that a URI without authority and without scheme can not have its path starting with multiple forward slashes. To me at least, normalizing the full URI only to secure Hope this is more clear. last but not least why removing the scheme or the host should affect the path. the issue is with the path and no other URI component. |
|
The approach to only normalize on HOWEVER, that said, the PSR-7 spec does account for this in the docblock of the request
Regarding this:
The URI is considered valid per RFC-3986, but currently triggers the CVE noted in the patch description when emitting only the path without normalizing the leading slashes. That's what we're trying to avoid here. If we go the route you are suggesting, As such, I'd be willing to update this patch as follows:
If everyone agrees to this, I'll make the changes for the final review. |
IMO manually querying the path should have the same serialization as making the URL relative by removing host and scheme and then serializing the whole URL. |
|
@weierophinney Sounds good to me. |
@weierophinney according to RFC3986 that instance is already invalid see section https://www.rfc-editor.org/rfc/rfc3986#section-3
So to my understanding this is already covered (ie the instance should throw or be invalid). Either way I am OK with your suggestions. |
- `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 <matthew@weierophinney.net>
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 <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
|
Finally finished up this patch. I've covered all test cases discussed previously for URIs containing paths with multiple leading slashes:
I modified the original test, |
- Documents the CVE that led to the new tests. - Documents the differences in behavior between using UriInterface::getPath(), UriInterface::__toString(), and RequestInterface::getRequestTarget().
Done! |
|
should i do a new release with this change? |
Yes, please. When you do, I'll run laminas-diactoros against it. |
Updates to 1.2.0, which adds the tests we wrote for mitigating ZF2015-05, with a few changes: - When creating the string representation of the URL, we DO NOT normalize the path to remove multiple leading slashes. In its absolute form, this is not necessary. - All normalization is done via `getPath()`; this mitigates the common XSS scenario. - It adds a test to validate that when using origin-form during a `RequestInterface::getRequestTarget()` call, it will use the results of `getPath()`, as this is a scenario where the XSS could also occur. I have removed one test from `UriTest`, as it contradicts the first point above. Since the scenario is covered in the PSR-7 integration tests, we are covered. See php-http/psr7-integration-tests#54 for more details. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
What's in this PR?
Per an issue created for Diactoros its related pull request, and the discussion to that pull request, this patch does the following:
testPathWithMultipleSlashes()to only validate that multiple slashes not at the beginning of a path are retained intact.testProperlyTrimsLeadingSlashesToPreventXSS(), which validates that when multiple leading slashes are present in a path, they are reduced to a single slash.Why?
This approach is done to mitigate ZF2015-05 which was also reported as 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).
Example Usage
When testing a PSR-7 imlementation, ensure that URLs with multiple leading slashes in the path are rewritten to a single leading slash in order to pass the integration suite tests.
Checklist