From 6ba00e829a9fb360dfe5cb0bc3d4544016c82357 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 30 Nov 2017 15:50:36 +1300 Subject: [PATCH] [ss-2017-009] Prevent disclosure of sensitive information via LoginAttempt --- security/LoginAttempt.php | 48 ++++++++++++++-------- security/Member.php | 7 ++-- tests/security/MemberAuthenticatorTest.php | 3 +- tests/security/SecurityTest.php | 31 ++++++-------- 4 files changed, 51 insertions(+), 38 deletions(-) diff --git a/security/LoginAttempt.php b/security/LoginAttempt.php index 4da1e2440..ec5c19ca2 100644 --- a/security/LoginAttempt.php +++ b/security/LoginAttempt.php @@ -12,18 +12,20 @@ * @package framework * @subpackage security * - * @property string Email Email address used for login attempt - * @property string Status Status of the login attempt, either 'Success' or 'Failure' - * @property string IP IP address of user attempting to login + * @property string $Email Email address used for login attempt. @deprecated 3.0...5.0 + * @property string $EmailHashed sha1 hashed Email address used for login attempt + * @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), + )); + } } diff --git a/security/Member.php b/security/Member.php index 39573a4b9..a83a9d20e 100644 --- a/security/Member.php +++ b/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}, - ))->sort('Created', 'DESC')->limit($this->config()->lock_out_after_incorrect_logins); + $email = $this->{static::config()->unique_identifier_field}; + $attempts = LoginAttempt::getByEmail($email) + ->sort('Created', 'DESC') + ->limit($this->config()->lock_out_after_incorrect_logins); if ($attempts->count() < $this->config()->lock_out_after_incorrect_logins) { return false; diff --git a/tests/security/MemberAuthenticatorTest.php b/tests/security/MemberAuthenticatorTest.php index f3e4598ca..bf6dd9942 100644 --- a/tests/security/MemberAuthenticatorTest.php +++ b/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); } diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index 7c3283525..e123b8212 100644 --- a/tests/security/SecurityTest.php +++ b/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( - '"LoginAttempt"."Email"' => 'testuser@example.com' - )); - $this->assertTrue(is_object($attempt)); - $member = DataObject::get_one('Member', array( - '"Member"."Email"' => 'testuser@example.com' - )); + $attempt = LoginAttempt::getByEmail('testuser@example.com')->first(); + $this->assertInstanceOf('LoginAttempt', $attempt); + $member = Member::get()->filter('Email', 'testuser@example.com')->first(); $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( - '"LoginAttempt"."Email"' => 'wronguser@silverstripe.com' - )); - $this->assertTrue(is_object($attempt)); + $attempt = LoginAttempt::getByEmail('wronguser@silverstripe.com')->first(); + $this->assertInstanceOf('LoginAttempt', $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( - '"LoginAttempt"."Email"' => 'testuser@example.com' - )); + $attempt = LoginAttempt::getByEmail('testuser@example.com')->first(); $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); }