From 6db830605c606d251175943d402820218a0205f9 Mon Sep 17 00:00:00 2001
From: Stig Lindqvist <stig@stojg.net>
Date: Thu, 27 Oct 2011 22:38:11 +0200
Subject: [PATCH 1/2] MINOR Do a isset check before using the value.

This happens if someone accidentially access /Security/LoginForm directly.
---
 security/MemberAuthenticator.php | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/security/MemberAuthenticator.php b/security/MemberAuthenticator.php
index e74cb1599..d2ada0adc 100644
--- a/security/MemberAuthenticator.php
+++ b/security/MemberAuthenticator.php
@@ -30,7 +30,12 @@ class MemberAuthenticator extends Authenticator {
    * @see Security::setDefaultAdmin()
    */
   public static function authenticate($RAW_data, Form $form = null) {
-    $SQL_user = Convert::raw2sql($RAW_data['Email']);
+	if(array_key_exists('Email', $RAW_data) && $RAW_data['Email']){
+		$SQL_user = Convert::raw2sql($RAW_data['Email']);
+	} else {
+		return false;
+	}
+    
 	$isLockedOut = false;
 	$result = null;
 

From 7a4c7a6e23dee40212db41012f761f4fd8fad6cd Mon Sep 17 00:00:00 2001
From: Stig Lindqvist <stig@stojg.net>
Date: Thu, 27 Oct 2011 22:38:29 +0200
Subject: [PATCH 2/2] MINOR Redirect user to homepage if the BackURL have been
 set to another site.

This might indicatate a spoofing attack. I also extracted code into it's own method to make it easier to read.
---
 security/MemberLoginForm.php | 112 ++++++++++++++++++++++-------------
 1 file changed, 71 insertions(+), 41 deletions(-)

diff --git a/security/MemberLoginForm.php b/security/MemberLoginForm.php
index 7f10328a8..fa66e3155 100644
--- a/security/MemberLoginForm.php
+++ b/security/MemberLoginForm.php
@@ -123,48 +123,12 @@ JS
 	 */
 	public function dologin($data) {
 		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);
-				}
-
-				$cp = new ChangePasswordForm($this->controller, 'ChangePasswordForm');
-				$cp->sessionMessage('Your password has expired.  Please choose a new one.', 'good');
-				
-				Director::redirect('Security/changepassword');
-			} elseif(
-				isset($_REQUEST['BackURL']) 
-				&& $_REQUEST['BackURL'] 
-				// absolute redirection URLs may cause spoofing 
-				&& Director::is_site_url($_REQUEST['BackURL'])
-			) {
-				Director::redirect($_REQUEST['BackURL']);
-			} elseif (Security::default_login_dest()) {
-				Director::redirect(Director::absoluteBaseURL() . Security::default_login_dest());
-			} else {
-				$member = Member::currentUser();
-				if($member) {
-					$firstname = Convert::raw2xml($member->FirstName);
-					
-					if(!empty($data['Remember'])) {
-						Session::set('SessionForms.MemberLoginForm.Remember', '1');
-						$member->logIn(true);
-					} else {
-						$member->logIn();
-					}
-					
-					Session::set('Security.Message.message',
-						sprintf(_t('Member.WELCOMEBACK', "Welcome Back, %s"), $firstname) 
-					);
-					Session::set("Security.Message.type", "good");
-				}
-				Director::redirectBack();
-			}
+			$this->logInUserAndRedirect($data);
 		} else {
-			Session::set('SessionForms.MemberLoginForm.Email', $data['Email']);
-			Session::set('SessionForms.MemberLoginForm.Remember', isset($data['Remember']));
+			if(array_key_exists('Email', $data)){
+				Session::set('SessionForms.MemberLoginForm.Email', $data['Email']);
+				Session::set('SessionForms.MemberLoginForm.Remember', isset($data['Remember']));
+			}
 
 			if(isset($_REQUEST['BackURL'])) $backURL = $_REQUEST['BackURL']; 
 			else $backURL = null; 
@@ -182,6 +146,72 @@ JS
 		}
 	}
 
+	/**
+	 * Login in the user and figure out where to redirect the browser.
+	 *
+	 * The $data has this format
+	 * array(
+	 *   'AuthenticationMethod' => 'MemberAuthenticator',
+	 *   'Email' => 'sam@silverstripe.com',
+	 *   'Password' => '1nitialPassword',
+	 *   'BackURL' => 'test/link',
+	 *   [Optional: 'Remember' => 1 ]
+	 * )
+	 *
+	 * @param array $data
+	 * @return void
+	 */
+	protected function logInUserAndRedirect($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);
+			}
+			$cp = new ChangePasswordForm($this->controller, 'ChangePasswordForm');
+			$cp->sessionMessage('Your password has expired. Please choose a new one.', 'good');
+			Director::redirect('Security/changepassword');
+			return;
+		}
+		
+		// Absolute redirection URLs may cause spoofing
+		if(isset($_REQUEST['BackURL']) && $_REQUEST['BackURL'] && Director::is_site_url($_REQUEST['BackURL']) ) {
+			Director::redirect($_REQUEST['BackURL']);
+			return;
+		}
+
+		// Spoofing attack, redirect to homepage instead of spoofing url
+		if(isset($_REQUEST['BackURL']) && $_REQUEST['BackURL'] && !Director::is_site_url($_REQUEST['BackURL'])) {
+			Director::redirect(Director::absoluteBaseURL());
+			return;
+		}
+
+		// If a default login dest has been set, redirect to that.
+		if (Security::default_login_dest()) {
+			Director::redirect(Director::absoluteBaseURL() . Security::default_login_dest());
+			return;
+		}
+
+		// Redirect the user to the page where he came from
+		$member = Member::currentUser();
+		if($member) {
+			$firstname = Convert::raw2xml($member->FirstName);
+			if(!empty($data['Remember'])) {
+				Session::set('SessionForms.MemberLoginForm.Remember', '1');
+				$member->logIn(true);
+			} else {
+				$member->logIn();
+			}
+
+			Session::set('Security.Message.message',
+				sprintf(_t('Member.WELCOMEBACK', "Welcome Back, %s"), $firstname)
+			);
+			Session::set("Security.Message.type", "good");
+		}
+		Controller::curr()->redirectBack();
+	}
+
 
 	/**
 	 * Log out form handler method