diff --git a/core/control/Director.php b/core/control/Director.php index 9540a05c5..192b73c5a 100644 --- a/core/control/Director.php +++ b/core/control/Director.php @@ -461,6 +461,20 @@ class Director { return preg_match('/^[a-zA-Z]:[\\\\\/]/', $path) == 1; } + /** + * Checks if a given URL is absolute (e.g. starts with 'http://' etc.). + * + * @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('/.*(\?.*)/', '', $url); + $parsed = parse_url($url); + return (isset($parsed['scheme'])); + } + /** * Given a filesystem reference relative to the site root, return the full file-system path. * diff --git a/security/MemberLoginForm.php b/security/MemberLoginForm.php index 6c150b493..0b7d25a13 100644 --- a/security/MemberLoginForm.php +++ b/security/MemberLoginForm.php @@ -111,7 +111,6 @@ class MemberLoginForm extends LoginForm { if($this->performLogin($data)) { Session::clear('SessionForms.MemberLoginForm.Email'); Session::clear('SessionForms.MemberLoginForm.Remember'); - if(Member::currentUser()->isPasswordExpired()) { if(isset($_REQUEST['BackURL']) && $backURL = $_REQUEST['BackURL']) { Session::set('BackURL', $backURL); @@ -121,9 +120,17 @@ class MemberLoginForm extends LoginForm { $cp->sessionMessage('Your password has expired. Please choose a new one.', 'good'); Director::redirect('Security/changepassword'); - } elseif(isset($_REQUEST['BackURL']) && $backURL = $_REQUEST['BackURL']) { - Session::clear("BackURL"); - Director::redirect($backURL); + } elseif( + isset($_REQUEST['BackURL']) + && $_REQUEST['BackURL'] + && ( + // absolute redirection URLs may cause spoofing + !Director::is_absolute_url($_REQUEST['BackURL']) + // absolute URLs on the current domain are allowed + || strpos($_REQUEST['BackURL'], Director::absoluteBaseURL()) !== FALSE + ) + ) { + Director::redirect($_REQUEST['BackURL']); } else { $member = Member::currentUser(); if($member) { diff --git a/tests/control/DirectorTest.php b/tests/control/DirectorTest.php index 89ca7b32a..409224ce3 100644 --- a/tests/control/DirectorTest.php +++ b/tests/control/DirectorTest.php @@ -68,5 +68,16 @@ 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('ftp://test.com')); + $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')); + } + } ?> \ No newline at end of file diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index 623192914..10383c3ab 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -37,6 +37,40 @@ class SecurityTest extends FunctionalTest { parent::tearDown(); } + function testExternalBackUrlRedirectionDisallowed() { + $page = new SiteTree(); + $page->URLSegment = 'testpage'; + $page->Title = 'Testpage'; + $page->write(); + $page->publish('Stage','Live'); + + // Test internal relative redirect + $response = $this->doTestLoginForm('noexpiry@silverstripe.com', '1nitialPassword', 'testpage'); + $this->assertEquals(302, $response->getStatusCode()); + $this->assertRegExp('/testpage/', $response->getHeader('Location'), + "Internal relative BackURLs work when passed through to login form" + ); + // Log the user out + $this->session()->inst_set('loggedInAs', null); + + // Test internal absolute redirect + $response = $this->doTestLoginForm('noexpiry@silverstripe.com', '1nitialPassword', Director::absoluteBaseURL() . 'testpage'); + // for some reason the redirect happens to a relative URL + $this->assertRegExp('/^' . preg_quote(Director::absoluteBaseURL(), '/') . 'testpage/', $response->getHeader('Location'), + "Internal absolute BackURLs work when passed through to login form" + ); + // Log the user out + $this->session()->inst_set('loggedInAs', null); + + // Test external redirect + $response = $this->doTestLoginForm('noexpiry@silverstripe.com', '1nitialPassword', 'http://myspoofedhost.com'); + $this->assertNotRegExp('/^' . preg_quote('http://myspoofedhost.com', '/') . '/', $response->getHeader('Location'), + "Redirection to external links in login form BackURL gets prevented as a measure against spoofing attacks" + ); + // Log the user out + $this->session()->inst_set('loggedInAs', null); + } + /** * Test that the login form redirects to the change password form after logging in with an expired password */ @@ -183,8 +217,8 @@ class SecurityTest extends FunctionalTest { * Execute a log-in form using Director::test(). * Helper method for the tests above */ - function doTestLoginForm($email, $password) { - $this->session()->inst_set('BackURL', 'test/link'); + function doTestLoginForm($email, $password, $backURL = 'test/link') { + $this->session()->inst_set('BackURL', $backURL); $this->get('Security/login'); return $this->submitForm(