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
This commit is contained in:
Ingo Schommer 2012-05-04 11:55:40 +02:00
parent 1f7f8b8aee
commit d5b3dbc6fb
3 changed files with 109 additions and 30 deletions

View File

@ -509,21 +509,42 @@ class Director implements TemplateGlobalProvider {
/** /**
* Checks if a given URL is absolute (e.g. starts with 'http://' etc.). * 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 * @param string $url
* @return boolean * @return boolean
*/ */
public static function is_absolute_url($url) { public static function is_absolute_url($url) {
$url = trim($url); $colonPosition = strpos($url, ':');
// remove all query strings to avoid parse_url choking on URLs like 'test.com?url=http://test.com' return (
$url = preg_replace('/(.*)\?.*/', '$1', $url); // Base check for existence of a host on a compliant URL
$parsed = parse_url($url); parse_url($url, PHP_URL_HOST)
return (isset($parsed['scheme'])); // 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 * Checks if a given URL is relative by checking {@link is_absolute_url()}.
* {@link is_absolute_url()}.
* *
* @param string $url * @param string $url
* @return boolean * @return boolean
@ -533,8 +554,10 @@ class Director implements TemplateGlobalProvider {
} }
/** /**
* Checks if the given URL is belonging to this "site", * Checks if the given URL is belonging to this "site" (not an external link).
* as defined by {@link makeRelative()} and {@link absoluteBaseUrl()}. * 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 * Useful to check before redirecting based on a URL from user submissions
* through $_GET or $_POST, and avoid phishing attacks by redirecting * through $_GET or $_POST, and avoid phishing attacks by redirecting
* to an attackers server. * to an attackers server.
@ -543,8 +566,13 @@ class Director implements TemplateGlobalProvider {
* @return boolean * @return boolean
*/ */
public static function is_site_url($url) { public static function is_site_url($url) {
$relativeUrl = Director::makeRelative($url); $urlHost = parse_url($url, PHP_URL_HOST);
return (bool)self::is_relative_url($relativeUrl); $actualHost = parse_url(self::protocolAndHost(), PHP_URL_HOST);
if($urlHost && $actualHost && $urlHost == $actualHost) {
return true;
} else {
return self::is_relative_url($url);
}
} }
/** /**

View File

@ -1,8 +1,11 @@
<?php <?php
class ControllerTest extends FunctionalTest { class ControllerTest extends FunctionalTest {
static $fixture_file = 'ControllerTest.yml'; static $fixture_file = 'ControllerTest.yml';
protected $autoFollowRedirection = false;
function testDefaultAction() { function testDefaultAction() {
/* For a controller with a template, the default action will simple run that template. */ /* For a controller with a template, the default action will simple run that template. */
$response = $this->get("ControllerTest_Controller/"); $response = $this->get("ControllerTest_Controller/");
@ -139,6 +142,50 @@ class ControllerTest extends FunctionalTest {
$this->assertEquals(Controller::BaseURL(), Director::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?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() { function stringaction() {
return "stringaction was called."; return "stringaction was called.";
} }
function redirectbacktest() {
return $this->redirectBack();
}
} }
/** /**

View File

@ -93,15 +93,18 @@ class DirectorTest extends SapphireTest {
} }
public function testIsAbsoluteUrl() { public function testIsAbsoluteUrl() {
$this->assertTrue(Director::is_absolute_url('http://test.com')); $this->assertTrue(Director::is_absolute_url('http://test.com/testpage'));
$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('ftp://test.com')); $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')); $this->assertFalse(Director::is_absolute_url('relative'));
$this->assertFalse(Director::is_absolute_url('/relative/?url=http://test.com')); $this->assertTrue(Director::is_absolute_url("https://test.com/?url=http://foo.com"));
$this->assertTrue(Director::is_absolute_url('http://test.com/?url=http://test.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() { public function testIsRelativeUrl() {
@ -113,8 +116,7 @@ class DirectorTest extends SapphireTest {
$this->assertFalse(Director::is_relative_url('ftp://test.com')); $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')); $this->assertTrue(Director::is_relative_url('relative'));
$this->assertTrue(Director::is_relative_url('/relative/?url=http://test.com')); // $this->assertTrue(Director::is_relative_url('/relative/?url=http://test.com'));
$this->assertFalse(Director::is_relative_url('http://test.com/?url=' . $siteUrl));
} }
public function testMakeRelative() { public function testMakeRelative() {
@ -132,18 +134,16 @@ class DirectorTest extends SapphireTest {
$this->assertEquals(Director::makeRelative("$siteUrl/?url=http://test.com"), '?url=http://test.com'); $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() { public function testIsSiteUrl() {
$siteUrl = Director::absoluteBaseURL(); $this->assertFalse(Director::is_site_url("http://test.com"));
$siteUrlNoProtocol = preg_replace('/https?:\/\//', '', $siteUrl); $this->assertTrue(Director::is_site_url(Director::absoluteBaseURL()));
$this->assertTrue(Director::is_site_url($siteUrl)); $this->assertFalse(Director::is_site_url("http://test.com?url=" . Director::absoluteBaseURL()));
$this->assertTrue(Director::is_site_url("$siteUrl/testpage")); $this->assertFalse(Director::is_site_url("http://test.com?url=" . urlencode(Director::absoluteBaseURL())));
$this->assertTrue(Director::is_site_url(" $siteUrl/testpage ")); $this->assertFalse(Director::is_site_url("//test.com?url=" . Director::absoluteBaseURL()));
$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"));
} }
public function testResetGlobalsAfterTestRequest() { public function testResetGlobalsAfterTestRequest() {