Permalink
Browse files

SECURITY More solid relative/site URL checks (related to "BackURL" re…

…direction)

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)

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
  • Loading branch information...
1 parent 2034927 commit 46064f8f88e3dadd36831175a5da9a9496d47a1e @chillu chillu committed May 4, 2012
Showing with 117 additions and 33 deletions.
  1. +40 −12 core/control/Director.php
  2. +57 −1 tests/ControllerTest.php
  3. +20 −20 tests/control/DirectorTest.php
View
52 core/control/Director.php
@@ -531,21 +531,42 @@ public static function is_absolute($path) {
/**
* 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
@@ -555,8 +576,10 @@ public static function is_relative_url($url) {
}
/**
- * 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.
@@ -565,8 +588,13 @@ public static function is_relative_url($url) {
* @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);
+ }
}
/**
@@ -907,4 +935,4 @@ static function isTest() {
return false;
}
-}
+}
View
58 tests/ControllerTest.php
@@ -2,6 +2,8 @@
class ControllerTest extends FunctionalTest {
static $fixture_file = 'sapphire/tests/ControllerTest.yml';
+
+ protected $autoFollowRedirection = false;
function testDefaultAction() {
/* For a controller with a template, the default action will simple run that template. */
@@ -129,7 +131,57 @@ public function testHasAction() {
'Without an allowed_actions, any defined methods are recognised as actions'
);
}
-
+
+ /* Controller::BaseURL no longer exists, but was just a direct call to Director::BaseURL, so not sure what this code was supposed to test
+ public function testBaseURL() {
+ Director::setBaseURL('/baseurl/');
+ $this->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?_REDIRECT_BACK_URL=' . 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?_REDIRECT_BACK_URL=' . 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?_REDIRECT_BACK_URL=' . urlencode($externalAbsoluteUrl));
+ $this->assertEquals(200, $response->getStatusCode(),
+ "Doesn't redirect on external URLs"
+ );
+ }
}
/**
@@ -147,6 +199,10 @@ function methodaction() {
function stringaction() {
return "stringaction was called.";
}
+
+ function redirectbacktest() {
+ return $this->redirectBack();
+ }
}
/**
View
40 tests/control/DirectorTest.php
@@ -93,15 +93,18 @@ public function testIsAbsolute() {
}
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 @@ public function testIsRelativeUrl() {
$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 @@ public function testMakeRelative() {
$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() {
@@ -244,4 +244,4 @@ public function returnRequestValue($request) { return $_REQUEST['somekey']; }
public function returnCookieValue($request) { return $_COOKIE['somekey']; }
-}
+}

0 comments on commit 46064f8

Please sign in to comment.