From c14e7f6b764ae4646461f3fc3a46452fdaa9e02a Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 25 May 2015 12:38:34 +1200 Subject: [PATCH] BUG Fix malformed urls redirecting to external sites --- control/Controller.php | 1 + forms/Form.php | 1 + security/ChangePasswordForm.php | 6 ++--- security/MemberLoginForm.php | 23 +++++++++-------- tests/control/ControllerTest.php | 9 ++++--- .../ParameterConfirmationTokenTest.php | 4 +++ tests/security/SecurityTest.php | 25 +++++++++++++++---- thirdparty/simpletest/url.php | 7 ++++-- 8 files changed, 53 insertions(+), 23 deletions(-) diff --git a/control/Controller.php b/control/Controller.php index 47066074f..536981df6 100644 --- a/control/Controller.php +++ b/control/Controller.php @@ -497,6 +497,7 @@ class Controller extends RequestHandler implements TemplateGlobalProvider { // absolute redirection URLs not located on this site may cause phishing if(Director::is_site_url($url)) { + $url = Director::absoluteURL($url, true); return $this->redirect($url); } else { return false; diff --git a/forms/Form.php b/forms/Form.php index 08827275d..f48b51ee4 100644 --- a/forms/Form.php +++ b/forms/Form.php @@ -349,6 +349,7 @@ class Form extends RequestHandler { if(Director::is_site_url($pageURL)) { // Remove existing pragmas $pageURL = preg_replace('/(#.*)/', '', $pageURL); + $pageURL = Director::absoluteURL($pageURL, true); return $this->controller->redirect($pageURL . '#' . $this->FormName()); } } diff --git a/security/ChangePasswordForm.php b/security/ChangePasswordForm.php index 8800d57e0..beda09564 100644 --- a/security/ChangePasswordForm.php +++ b/security/ChangePasswordForm.php @@ -102,12 +102,12 @@ class ChangePasswordForm extends Form { // TODO Add confirmation message to login redirect Session::clear('AutoLoginHash'); - if (isset($_REQUEST['BackURL']) - && $_REQUEST['BackURL'] + if (!empty($_REQUEST['BackURL']) // absolute redirection URLs may cause spoofing && Director::is_site_url($_REQUEST['BackURL']) ) { - $this->controller->redirect($_REQUEST['BackURL']); + $url = Director::absoluteURL($_REQUEST['BackURL']); + $this->controller->redirect($url); } else { // Redirect to default location - the login form saying "You are logged in as..." diff --git a/security/MemberLoginForm.php b/security/MemberLoginForm.php index 40743fb27..a66039a7f 100644 --- a/security/MemberLoginForm.php +++ b/security/MemberLoginForm.php @@ -165,7 +165,7 @@ JS * ) * * @param array $data - * @return void + * @return SS_HTTPResponse */ protected function logInUserAndRedirect($data) { Session::clear('SessionForms.MemberLoginForm.Email'); @@ -181,18 +181,21 @@ JS } // Absolute redirection URLs may cause spoofing - if(isset($_REQUEST['BackURL']) && $_REQUEST['BackURL'] && Director::is_site_url($_REQUEST['BackURL']) ) { - return $this->controller->redirect($_REQUEST['BackURL']); - } - - // Spoofing attack, redirect to homepage instead of spoofing url - if(isset($_REQUEST['BackURL']) && $_REQUEST['BackURL'] && !Director::is_site_url($_REQUEST['BackURL'])) { - return $this->controller->redirect(Director::absoluteBaseURL()); + if(!empty($_REQUEST['BackURL'])) { + $url = $_REQUEST['BackURL']; + if(Director::is_site_url($url) ) { + $url = Director::absoluteURL($url); + } else { + // Spoofing attack, redirect to homepage instead of spoofing url + $url = Director::absoluteBaseURL(); + } + return $this->controller->redirect($url); } // If a default login dest has been set, redirect to that. - if (Security::default_login_dest()) { - return $this->controller->redirect(Director::absoluteBaseURL() . Security::default_login_dest()); + if ($url = Security::default_login_dest()) { + $url = Controller::join_links(Director::absoluteBaseURL(), $url); + return $this->controller->redirect($url); } // Redirect the user to the page where he came from diff --git a/tests/control/ControllerTest.php b/tests/control/ControllerTest.php index be5c61ee0..756c64786 100644 --- a/tests/control/ControllerTest.php +++ b/tests/control/ControllerTest.php @@ -201,14 +201,15 @@ class ControllerTest extends FunctionalTest { public function testRedirectBackByReferer() { $internalRelativeUrl = '/some-url'; + $internalAbsoluteUrl = Controller::join_links(Director::absoluteBaseURL(), '/some-url'); + $response = $this->get('ControllerTest_Controller/redirectbacktest', null, array('Referer' => $internalRelativeUrl)); $this->assertEquals(302, $response->getStatusCode()); - $this->assertEquals($internalRelativeUrl, $response->getHeader('Location'), + $this->assertEquals($internalAbsoluteUrl, $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()); @@ -226,9 +227,11 @@ class ControllerTest extends FunctionalTest { public function testRedirectBackByBackUrl() { $internalRelativeUrl = '/some-url'; + $internalAbsoluteUrl = Controller::join_links(Director::absoluteBaseURL(), '/some-url'); + $response = $this->get('ControllerTest_Controller/redirectbacktest?BackURL=' . urlencode($internalRelativeUrl)); $this->assertEquals(302, $response->getStatusCode()); - $this->assertEquals($internalRelativeUrl, $response->getHeader('Location'), + $this->assertEquals($internalAbsoluteUrl, $response->getHeader('Location'), "Redirects on internal relative URLs" ); diff --git a/tests/core/startup/ParameterConfirmationTokenTest.php b/tests/core/startup/ParameterConfirmationTokenTest.php index 41bc59e59..750be80dd 100644 --- a/tests/core/startup/ParameterConfirmationTokenTest.php +++ b/tests/core/startup/ParameterConfirmationTokenTest.php @@ -31,9 +31,12 @@ class ParameterConfirmationTokenTest extends SapphireTest { return array($answer, $slash); } + + protected $oldHost = null; public function setUp() { parent::setUp(); + $this->oldHost = $_SERVER['HTTP_HOST']; $_GET['parameterconfirmationtokentest_notoken'] = 'value'; $_GET['parameterconfirmationtokentest_empty'] = ''; $_GET['parameterconfirmationtokentest_withtoken'] = '1'; @@ -48,6 +51,7 @@ class ParameterConfirmationTokenTest extends SapphireTest { foreach($_GET as $param) { if(stripos($param, 'parameterconfirmationtokentest_') === 0) unset($_GET[$param]); } + $_SERVER['HTTP_HOST'] = $this->oldHost; parent::tearDown(); } diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index d0e681e9d..946651910 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -177,13 +177,19 @@ class SecurityTest extends FunctionalTest { /* UNEXPIRED PASSWORD GO THROUGH WITHOUT A HITCH */ $goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); $this->assertEquals(302, $goodResponse->getStatusCode()); - $this->assertEquals(Director::baseURL() . 'test/link', $goodResponse->getHeader('Location')); + $this->assertEquals( + Controller::join_links(Director::absoluteBaseURL(), 'test/link'), + $goodResponse->getHeader('Location') + ); $this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs')); /* EXPIRED PASSWORDS ARE SENT TO THE CHANGE PASSWORD FORM */ $expiredResponse = $this->doTestLoginForm('expired@silverstripe.com' , '1nitialPassword'); $this->assertEquals(302, $expiredResponse->getStatusCode()); - $this->assertEquals(Director::baseURL() . 'Security/changepassword', $expiredResponse->getHeader('Location')); + $this->assertEquals( + Controller::join_links(Director::baseURL(), 'Security/changepassword'), + $expiredResponse->getHeader('Location') + ); $this->assertEquals($this->idFromFixture('Member', 'expiredpassword'), $this->session()->inst_get('loggedInAs')); @@ -191,7 +197,10 @@ class SecurityTest extends FunctionalTest { $this->mainSession->followRedirection(); $changedResponse = $this->doTestChangepasswordForm('1nitialPassword', 'changedPassword'); $this->assertEquals(302, $changedResponse->getStatusCode()); - $this->assertEquals(Director::baseURL() . 'test/link', $changedResponse->getHeader('Location')); + $this->assertEquals( + Controller::join_links(Director::absoluteBaseURL(), 'test/link'), + $changedResponse->getHeader('Location') + ); } public function testChangePasswordForLoggedInUsers() { @@ -201,13 +210,19 @@ class SecurityTest extends FunctionalTest { $this->get('Security/changepassword?BackURL=test/back'); $changedResponse = $this->doTestChangepasswordForm('1nitialPassword', 'changedPassword'); $this->assertEquals(302, $changedResponse->getStatusCode()); - $this->assertEquals(Director::baseURL() . 'test/back', $changedResponse->getHeader('Location')); + $this->assertEquals( + Controller::join_links(Director::absoluteBaseURL(), 'test/back'), + $changedResponse->getHeader('Location') + ); $this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs')); // Check if we can login with the new password $goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , 'changedPassword'); $this->assertEquals(302, $goodResponse->getStatusCode()); - $this->assertEquals(Director::baseURL() . 'test/link', $goodResponse->getHeader('Location')); + $this->assertEquals( + Controller::join_links(Director::absoluteBaseURL(), 'test/link'), + $goodResponse->getHeader('Location') + ); $this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs')); } diff --git a/thirdparty/simpletest/url.php b/thirdparty/simpletest/url.php index d3461657a..cdd725079 100644 --- a/thirdparty/simpletest/url.php +++ b/thirdparty/simpletest/url.php @@ -410,7 +410,7 @@ class SimpleUrl { */ function asString() { $path = $this->_path; - $scheme = $identity = $host = $encoded = $fragment = ''; + $scheme = $identity = $host = $port = $encoded = $fragment = ''; if ($this->_username && $this->_password) { $identity = $this->_username . ':' . $this->_password . '@'; } @@ -419,13 +419,16 @@ class SimpleUrl { $scheme .= "://"; $host = $this->getHost(); } + if ($this->getPort() && $this->getPort() != 80 ) { + $port = ':'.$this->getPort(); + } if (substr($this->_path, 0, 1) == '/') { $path = $this->normalisePath($this->_path); } $encoded = $this->getEncodedRequest(); $fragment = $this->getFragment() ? '#'. $this->getFragment() : ''; $coords = $this->getX() === false ? '' : '?' . $this->getX() . ',' . $this->getY(); - return "$scheme$identity$host$path$encoded$fragment$coords"; + return "$scheme$identity$host$port$path$encoded$fragment$coords"; } /**