From 1f7f8b8aee67181bd91a0c0dba7786ac9da1c1e6 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Fri, 4 May 2012 11:30:45 +0200 Subject: [PATCH 1/2] BUGFIX Don't' set 'Referer' header in FunctionalTest->get()/post() if its explicitly passed to the method --- dev/TestSession.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev/TestSession.php b/dev/TestSession.php index cdd49c916..1f654d58e 100644 --- a/dev/TestSession.php +++ b/dev/TestSession.php @@ -43,7 +43,7 @@ class TestSession { */ function get($url, $session = null, $headers = null, $cookies = null) { $headers = (array) $headers; - if($this->lastUrl) $headers['Referer'] = $this->lastUrl; + if($this->lastUrl && !isset($headers['Referer'])) $headers['Referer'] = $this->lastUrl; $this->lastResponse = Director::test($url, null, $session ? $session : $this->session, null, null, $headers, $cookies); $this->lastUrl = $url; if(!$this->lastResponse) user_error("Director::test($url) returned null", E_USER_WARNING); @@ -56,7 +56,7 @@ class TestSession { */ function post($url, $data, $headers = null, $session = null, $body = null, $cookies = null) { $headers = (array) $headers; - if($this->lastUrl) $headers['Referer'] = $this->lastUrl; + if($this->lastUrl && !isset($headers['Referer'])) $headers['Referer'] = $this->lastUrl; $this->lastResponse = Director::test($url, $data, $session ? $session : $this->session, null, $body, $headers, $cookies); $this->lastUrl = $url; if(!$this->lastResponse) user_error("Director::test($url) returned null", E_USER_WARNING); From d5b3dbc6fbfec2eac201805ca0dc33f8b78ca993 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Fri, 4 May 2012 11:55:40 +0200 Subject: [PATCH 2/2] SECURITY Return true for Director::is_absolute_url() checks if they're prefixed with two or more slashes (as browsers interpret this as a valid URL) SECURITY More solid URL checks in Director::is_site_url(), using a conservative parse_url() hostname comparison rather than Director::makeRelative(), which is not designed for security purposes --- control/Director.php | 50 ++++++++++++++++++++++++------- tests/control/ControllerTest.php | 51 ++++++++++++++++++++++++++++++++ tests/control/DirectorTest.php | 38 ++++++++++++------------ 3 files changed, 109 insertions(+), 30 deletions(-) diff --git a/control/Director.php b/control/Director.php index 36cc29f53..b9c06a469 100644 --- a/control/Director.php +++ b/control/Director.php @@ -509,21 +509,42 @@ class Director implements TemplateGlobalProvider { /** * Checks if a given URL is absolute (e.g. starts with 'http://' etc.). + * URLs beginning with "//" are treated as absolute, as browsers take this to mean + * the same protocol as currently being used. + * + * Useful to check before redirecting based on a URL from user submissions + * through $_GET or $_POST, and avoid phishing attacks by redirecting + * to an attackers server. + * + * Note: Can't solely rely on PHP's parse_url() , since it is not intended to work with relative URLs + * or for security purposes. filter_var($url, FILTER_VALIDATE_URL) has similar problems. * * @param string $url * @return boolean */ public static function is_absolute_url($url) { - $url = trim($url); - // remove all query strings to avoid parse_url choking on URLs like 'test.com?url=http://test.com' - $url = preg_replace('/(.*)\?.*/', '$1', $url); - $parsed = parse_url($url); - return (isset($parsed['scheme'])); + $colonPosition = strpos($url, ':'); + return ( + // Base check for existence of a host on a compliant URL + parse_url($url, PHP_URL_HOST) + // Check for more than one leading slash without a protocol. + // While not a RFC compliant absolute URL, it is completed to a valid URL by some browsers, + // and hence a potential security risk. Single leading slashes are not an issue though. + || preg_match('/\s*[\/]{2,}/', $url) + || ( + // If a colon is found, check if it's part of a valid scheme definition + // (meaning its not preceded by a slash, hash or questionmark). + // URLs in query parameters are assumed to be correctly urlencoded based on RFC3986, + // in which case no colon should be present in the parameters. + $colonPosition !== FALSE + && !preg_match('![/?#]!', substr($url, 0, $colonPosition)) + ) + + ); } /** - * Checks if a given URL is relative by checking - * {@link is_absolute_url()}. + * Checks if a given URL is relative by checking {@link is_absolute_url()}. * * @param string $url * @return boolean @@ -533,8 +554,10 @@ class Director implements TemplateGlobalProvider { } /** - * Checks if the given URL is belonging to this "site", - * as defined by {@link makeRelative()} and {@link absoluteBaseUrl()}. + * Checks if the given URL is belonging to this "site" (not an external link). + * That's the case if the URL is relative, as defined by {@link is_relative_url()}, + * or if the host matches {@link protocolAndHost()}. + * * Useful to check before redirecting based on a URL from user submissions * through $_GET or $_POST, and avoid phishing attacks by redirecting * to an attackers server. @@ -543,8 +566,13 @@ class Director implements TemplateGlobalProvider { * @return boolean */ public static function is_site_url($url) { - $relativeUrl = Director::makeRelative($url); - return (bool)self::is_relative_url($relativeUrl); + $urlHost = parse_url($url, PHP_URL_HOST); + $actualHost = parse_url(self::protocolAndHost(), PHP_URL_HOST); + if($urlHost && $actualHost && $urlHost == $actualHost) { + return true; + } else { + return self::is_relative_url($url); + } } /** diff --git a/tests/control/ControllerTest.php b/tests/control/ControllerTest.php index 6808c9501..0c47f08db 100644 --- a/tests/control/ControllerTest.php +++ b/tests/control/ControllerTest.php @@ -1,7 +1,10 @@ assertEquals(Controller::BaseURL(), Director::BaseURL()); } */ + + function testRedirectBackByReferer() { + $internalRelativeUrl = '/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest', null, array('Referer' => $internalRelativeUrl)); + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals($internalRelativeUrl, $response->getHeader('Location'), + "Redirects on internal relative URLs" + ); + + $internalAbsoluteUrl = Director::absoluteBaseURL() . '/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest', null, array('Referer' => $internalAbsoluteUrl)); + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals($internalAbsoluteUrl, $response->getHeader('Location'), + "Redirects on internal absolute URLs" + ); + + $externalAbsoluteUrl = 'http://myhost.com/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest', null, array('Referer' => $externalAbsoluteUrl)); + $this->assertEquals(200, $response->getStatusCode(), + "Doesn't redirect on external URLs" + ); + } + + function testRedirectBackByBackUrl() { + $internalRelativeUrl = '/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest?BackURL=' . urlencode($internalRelativeUrl)); + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals($internalRelativeUrl, $response->getHeader('Location'), + "Redirects on internal relative URLs" + ); + + $internalAbsoluteUrl = Director::absoluteBaseURL() . '/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest?BackURL=' . urlencode($internalAbsoluteUrl)); + $this->assertEquals($internalAbsoluteUrl, $response->getHeader('Location')); + $this->assertEquals(302, $response->getStatusCode(), + "Redirects on internal absolute URLs" + ); + + $externalAbsoluteUrl = 'http://myhost.com/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest?BackURL=' . urlencode($externalAbsoluteUrl)); + $this->assertEquals(200, $response->getStatusCode(), + "Doesn't redirect on external URLs" + ); + } } /** @@ -156,6 +203,10 @@ class ControllerTest_Controller extends Controller implements TestOnly { function stringaction() { return "stringaction was called."; } + + function redirectbacktest() { + return $this->redirectBack(); + } } /** diff --git a/tests/control/DirectorTest.php b/tests/control/DirectorTest.php index 55e8a4c82..0a273e887 100644 --- a/tests/control/DirectorTest.php +++ b/tests/control/DirectorTest.php @@ -93,15 +93,18 @@ class DirectorTest extends SapphireTest { } public function testIsAbsoluteUrl() { - $this->assertTrue(Director::is_absolute_url('http://test.com')); - $this->assertTrue(Director::is_absolute_url('https://test.com')); - $this->assertTrue(Director::is_absolute_url(' https://test.com/testpage ')); - $this->assertFalse(Director::is_absolute_url('test.com/testpage')); + $this->assertTrue(Director::is_absolute_url('http://test.com/testpage')); $this->assertTrue(Director::is_absolute_url('ftp://test.com')); + $this->assertFalse(Director::is_absolute_url('test.com/testpage')); $this->assertFalse(Director::is_absolute_url('/relative')); $this->assertFalse(Director::is_absolute_url('relative')); - $this->assertFalse(Director::is_absolute_url('/relative/?url=http://test.com')); - $this->assertTrue(Director::is_absolute_url('http://test.com/?url=http://test.com')); + $this->assertTrue(Director::is_absolute_url("https://test.com/?url=http://foo.com")); + $this->assertTrue(Director::is_absolute_url("trickparseurl:http://test.com")); + $this->assertTrue(Director::is_absolute_url('//test.com')); + $this->assertTrue(Director::is_absolute_url('/////test.com')); + $this->assertTrue(Director::is_absolute_url(' ///test.com')); + $this->assertTrue(Director::is_absolute_url('http:test.com')); + $this->assertTrue(Director::is_absolute_url('//http://test.com')); } public function testIsRelativeUrl() { @@ -113,8 +116,7 @@ class DirectorTest extends SapphireTest { $this->assertFalse(Director::is_relative_url('ftp://test.com')); $this->assertTrue(Director::is_relative_url('/relative')); $this->assertTrue(Director::is_relative_url('relative')); - $this->assertTrue(Director::is_relative_url('/relative/?url=http://test.com')); - $this->assertFalse(Director::is_relative_url('http://test.com/?url=' . $siteUrl)); + // $this->assertTrue(Director::is_relative_url('/relative/?url=http://test.com')); } public function testMakeRelative() { @@ -132,18 +134,16 @@ class DirectorTest extends SapphireTest { $this->assertEquals(Director::makeRelative("$siteUrl/?url=http://test.com"), '?url=http://test.com'); } + /** + * Mostly tested by {@link testIsRelativeUrl()}, + * just adding the host name matching aspect here. + */ public function testIsSiteUrl() { - $siteUrl = Director::absoluteBaseURL(); - $siteUrlNoProtocol = preg_replace('/https?:\/\//', '', $siteUrl); - $this->assertTrue(Director::is_site_url($siteUrl)); - $this->assertTrue(Director::is_site_url("$siteUrl/testpage")); - $this->assertTrue(Director::is_site_url(" $siteUrl/testpage ")); - $this->assertTrue(Director::is_site_url("$siteUrlNoProtocol/testpage")); - $this->assertFalse(Director::is_site_url('http://test.com/testpage')); - //$this->assertFalse(Director::is_site_url('test.com/testpage')); - $this->assertTrue(Director::is_site_url('/relative')); - $this->assertTrue(Director::is_site_url('relative')); - $this->assertFalse(Director::is_site_url("http://test.com/?url=$siteUrl")); + $this->assertFalse(Director::is_site_url("http://test.com")); + $this->assertTrue(Director::is_site_url(Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url("http://test.com?url=" . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url("http://test.com?url=" . urlencode(Director::absoluteBaseURL()))); + $this->assertFalse(Director::is_site_url("//test.com?url=" . Director::absoluteBaseURL())); } public function testResetGlobalsAfterTestRequest() {