Merge pull request #54 from silverstripe-security/pulls/3.5/ss-2017-009

[ss-2017-009] Prevent disclosure of sensitive information via LoginAttempt (3.5 branch)

security/LoginAttempt.php

@@ -12,18 +12,20 @@
  * @package framework
  * @subpackage security
  *
- * @property string Email Email address used for login attempt
+ * @property string $Email Email address used for login attempt. @deprecated 3.0...5.0
- * @property string Status Status of the login attempt, either 'Success' or 'Failure'
+ * @property string $EmailHashed sha1 hashed Email address used for login attempt
- * @property string IP IP address of user attempting to login
+ * @property string $Status Status of the login attempt, either 'Success' or 'Failure'
+ * @property string $IP IP address of user attempting to login
  *
- * @property int MemberID ID of the Member, only if Member with Email exists
+ * @property int $MemberID ID of the Member, only if Member with Email exists
  *
  * @method Member Member() Member object of the user trying to log in, only if Member with Email exists
  */
 class LoginAttempt extends DataObject {
 
 	private static $db = array(
-		'Email' => 'Varchar(255)',
+		'Email' => 'Varchar(255)', // Remove in 5.0
+		'EmailHashed' => 'Varchar(255)',
 		'Status' => "Enum('Success,Failure')",
 		'IP' => 'Varchar(255)',
 	);
@@ -32,24 +34,38 @@ class LoginAttempt extends DataObject {
 		'Member' => 'Member', // only linked if the member actually exists
 	);
 
-	private static $has_many = array();
-
-	private static $many_many = array();
-
-	private static $belongs_many_many = array();
-
-	/**
-	 *
-	 * @param boolean $includerelations a boolean value to indicate if the labels returned include relation fields
-	 *
-	 */
 	public function fieldLabels($includerelations = true) {
 		$labels = parent::fieldLabels($includerelations);
 		$labels['Email'] = _t('LoginAttempt.Email', 'Email Address');
+		$labels['EmailHashed'] = _t('LoginAttempt.EmailHashed', 'Email Address (hashed)');
 		$labels['Status'] = _t('LoginAttempt.Status', 'Status');
 		$labels['IP'] = _t('LoginAttempt.IP', 'IP Address');
 
 		return $labels;
 	}
 
+	/**
+	 * Set email used for this attempt
+	 *
+	 * @param string $email
+	 * @return $this
+	 */
+	public function setEmail($email) {
+		// Store hashed email only
+		$this->EmailHashed = sha1($email);
+		return $this;
+	}
+
+	/**
+	 * Get all login attempts for the given email address
+	 *
+	 * @param string $email
+	 * @return DataList
+	 */
+	public static function getByEmail($email) {
+		return static::get()->filterAny(array(
+			'Email' => $email,
+			'EmailHashed' => sha1($email),
+		));
+	}
 }

security/Member.php

@@ -407,9 +407,10 @@ class Member extends DataObject implements TemplateGlobalProvider {
 			return false;
 		}
 
-		$attempts = LoginAttempt::get()->filter($filter = array(
+		$email = $this->{static::config()->unique_identifier_field};
-				'Email' => $this->{static::config()->unique_identifier_field},
+		$attempts = LoginAttempt::getByEmail($email)
-		))->sort('Created', 'DESC')->limit($this->config()->lock_out_after_incorrect_logins);
+			->sort('Created', 'DESC')
+			->limit($this->config()->lock_out_after_incorrect_logins);
 
 		if ($attempts->count() < $this->config()->lock_out_after_incorrect_logins) {
 			return false;

tests/security/MemberAuthenticatorTest.php

@@ -196,7 +196,8 @@ class MemberAuthenticatorTest extends SapphireTest {
 		$this->assertNull($response);
 		$this->assertCount(1, LoginAttempt::get());
 		$attempt = LoginAttempt::get()->first();
-		$this->assertEquals($email, $attempt->Email);
+		$this->assertEmpty($attempt->Email); // Doesn't store potentially sensitive data
+		$this->assertEquals(sha1($email), $attempt->EmailHashed);
 		$this->assertEquals('Failure', $attempt->Status);
 
 	}

tests/security/SecurityTest.php

@@ -507,25 +507,21 @@ class SecurityTest extends FunctionalTest {
 
 		/* UNSUCCESSFUL ATTEMPTS WITH WRONG PASSWORD FOR EXISTING USER ARE LOGGED */
 		$this->doTestLoginForm('testuser@example.com', 'wrongpassword');
-		$attempt = DataObject::get_one('LoginAttempt', array(
+		$attempt = LoginAttempt::getByEmail('testuser@example.com')->first();
-			'"LoginAttempt"."Email"' => 'testuser@example.com'
+		$this->assertInstanceOf('LoginAttempt', $attempt);
-		));
+		$member = Member::get()->filter('Email', 'testuser@example.com')->first();
-		$this->assertTrue(is_object($attempt));
-		$member = DataObject::get_one('Member', array(
-			'"Member"."Email"' => 'testuser@example.com'
-		));
 		$this->assertEquals($attempt->Status, 'Failure');
-		$this->assertEquals($attempt->Email, 'testuser@example.com');
+		$this->assertEmpty($attempt->Email); // Doesn't store potentially sensitive data
+		$this->assertEquals($attempt->EmailHashed, sha1('testuser@example.com'));
 		$this->assertEquals($attempt->Member(), $member);
 
 		/* UNSUCCESSFUL ATTEMPTS WITH NONEXISTING USER ARE LOGGED */
 		$this->doTestLoginForm('wronguser@silverstripe.com', 'wrongpassword');
-		$attempt = DataObject::get_one('LoginAttempt', array(
+		$attempt = LoginAttempt::getByEmail('wronguser@silverstripe.com')->first();
-			'"LoginAttempt"."Email"' => 'wronguser@silverstripe.com'
+		$this->assertInstanceOf('LoginAttempt', $attempt);
-		));
-		$this->assertTrue(is_object($attempt));
 		$this->assertEquals($attempt->Status, 'Failure');
-		$this->assertEquals($attempt->Email, 'wronguser@silverstripe.com');
+		$this->assertEmpty($attempt->Email); // Doesn't store potentially sensitive data
+		$this->assertEquals($attempt->EmailHashed, sha1('wronguser@silverstripe.com'));
 		$this->assertNotNull(
 			$this->loginErrorMessage(), 'An invalid email returns a message.'
 		);
@@ -536,15 +532,14 @@ class SecurityTest extends FunctionalTest {
 
 		/* SUCCESSFUL ATTEMPTS ARE LOGGED */
 		$this->doTestLoginForm('testuser@example.com', '1nitialPassword');
-		$attempt = DataObject::get_one('LoginAttempt', array(
+		$attempt = LoginAttempt::getByEmail('testuser@example.com')->first();
-			'"LoginAttempt"."Email"' => 'testuser@example.com'
-		));
 		$member = DataObject::get_one('Member', array(
 			'"Member"."Email"' => 'testuser@example.com'
 		));
-		$this->assertTrue(is_object($attempt));
+		$this->assertInstanceOf('LoginAttempt', $attempt);
 		$this->assertEquals($attempt->Status, 'Success');
-		$this->assertEquals($attempt->Email, 'testuser@example.com');
+		$this->assertEmpty($attempt->Email); // Doesn't store potentially sensitive data
+		$this->assertEquals($attempt->EmailHashed, sha1('testuser@example.com'));
 		$this->assertEquals($attempt->Member(), $member);
 	}