From f1dd3d6f03eb1d94c29c495994a1da9176a758d9 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 30 Nov 2017 16:56:18 +1300 Subject: [PATCH] [ss-2017-009] Prevent disclosure of sensitive information via LoginAttempt --- src/Security/LoginAttempt.php | 42 ++++++++++++--- src/Security/Member.php | 3 +- .../php/Security/MemberAuthenticatorTest.php | 3 +- tests/php/Security/SecurityTest.php | 51 +++++-------------- 4 files changed, 53 insertions(+), 46 deletions(-) diff --git a/src/Security/LoginAttempt.php b/src/Security/LoginAttempt.php index 8117359cb..f3e631792 100644 --- a/src/Security/LoginAttempt.php +++ b/src/Security/LoginAttempt.php @@ -2,6 +2,7 @@ namespace SilverStripe\Security; +use SilverStripe\ORM\DataList; use SilverStripe\ORM\DataObject; /** @@ -14,11 +15,11 @@ use SilverStripe\ORM\DataObject; * complies with your privacy standards. We're logging * username and IP. * - * @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 int MemberID ID of the Member, only if Member with Email exists + * @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 * * @method Member Member() Member object of the user trying to log in, only if Member with Email exists */ @@ -35,7 +36,8 @@ class LoginAttempt extends DataObject const FAILURE = 'Failure'; private static $db = array( - 'Email' => 'Varchar(255)', + 'Email' => 'Varchar(255)', // Remove in 5.0 + 'EmailHashed' => 'Varchar(255)', 'Status' => "Enum('Success,Failure')", 'IP' => 'Varchar(255)', ); @@ -55,9 +57,37 @@ class LoginAttempt extends DataObject { $labels = parent::fieldLabels($includerelations); $labels['Email'] = _t(__CLASS__.'.Email', 'Email Address'); + $labels['EmailHashed'] = _t(__CLASS__.'.EmailHashed', 'Email Address (hashed)'); $labels['Status'] = _t(__CLASS__.'.Status', 'Status'); $labels['IP'] = _t(__CLASS__.'.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|LoginAttempt[] + */ + public static function getByEmail($email) + { + return static::get()->filterAny(array( + 'Email' => $email, + 'EmailHashed' => sha1($email), + )); + } } diff --git a/src/Security/Member.php b/src/Security/Member.php index a861b7ba4..34adbb98e 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -384,8 +384,7 @@ class Member extends DataObject } $idField = static::config()->get('unique_identifier_field'); - $attempts = LoginAttempt::get() - ->filter('Email', $this->{$idField}) + $attempts = LoginAttempt::getByEmail($this->{$idField}) ->sort('Created', 'DESC') ->limit($maxAttempts); diff --git a/tests/php/Security/MemberAuthenticatorTest.php b/tests/php/Security/MemberAuthenticatorTest.php index 6a203681f..3aa4e1d0e 100644 --- a/tests/php/Security/MemberAuthenticatorTest.php +++ b/tests/php/Security/MemberAuthenticatorTest.php @@ -265,7 +265,8 @@ class MemberAuthenticatorTest extends SapphireTest $this->assertNull($member); $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(LoginAttempt::FAILURE, $attempt->Status); } diff --git a/tests/php/Security/SecurityTest.php b/tests/php/Security/SecurityTest.php index 49e9a51e5..469371ec0 100644 --- a/tests/php/Security/SecurityTest.php +++ b/tests/php/Security/SecurityTest.php @@ -3,7 +3,6 @@ namespace SilverStripe\Security\Tests; use Page; -use PageController; use SilverStripe\Control\Controller; use SilverStripe\Control\Director; use SilverStripe\Control\HTTPRequest; @@ -615,34 +614,21 @@ class SecurityTest extends FunctionalTest /* UNSUCCESSFUL ATTEMPTS WITH WRONG PASSWORD FOR EXISTING USER ARE LOGGED */ $this->doTestLoginForm('testuser@example.com', 'wrongpassword'); /** @var LoginAttempt $attempt */ - $attempt = DataObject::get_one( - LoginAttempt::class, - array( - '"LoginAttempt"."Email"' => 'testuser@example.com' - ) - ); + $attempt = LoginAttempt::getByEmail('testuser@example.com')->first(); $this->assertInstanceOf(LoginAttempt::class, $attempt); - $member = DataObject::get_one( - Member::class, - array( - '"Member"."Email"' => 'testuser@example.com' - ) - ); + $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()->toMap(), $member->toMap()); /* UNSUCCESSFUL ATTEMPTS WITH NONEXISTING USER ARE LOGGED */ $this->doTestLoginForm('wronguser@silverstripe.com', 'wrongpassword'); - $attempt = DataObject::get_one( - LoginAttempt::class, - array( - '"LoginAttempt"."Email"' => 'wronguser@silverstripe.com' - ) - ); - $this->assertTrue(is_object($attempt)); + $attempt = LoginAttempt::getByEmail('wronguser@silverstripe.com')->first(); + $this->assertInstanceOf(LoginAttempt::class, $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->assertNotEmpty($this->getValidationResult()->getMessages(), 'An invalid email returns a message.'); } @@ -653,22 +639,12 @@ class SecurityTest extends FunctionalTest /* SUCCESSFUL ATTEMPTS ARE LOGGED */ $this->doTestLoginForm('testuser@example.com', '1nitialPassword'); /** @var LoginAttempt $attempt */ - $attempt = DataObject::get_one( - LoginAttempt::class, - array( - '"LoginAttempt"."Email"' => 'testuser@example.com' - ) - ); - /** @var Member $member */ - $member = DataObject::get_one( - Member::class, - array( - '"Member"."Email"' => 'testuser@example.com' - ) - ); - $this->assertTrue(is_object($attempt)); + $attempt = LoginAttempt::getByEmail('testuser@example.com')->first(); + $member = Member::get()->filter('Email', 'testuser@example.com')->first(); + $this->assertInstanceOf(LoginAttempt::class, $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()->toMap(), $member->toMap()); } @@ -725,6 +701,7 @@ class SecurityTest extends FunctionalTest // Ensure page shares the same controller as security $securityClass = Config::inst()->get(Security::class, 'page_class'); + /** @var Page $securityPage */ $securityPage = new $securityClass(); $this->assertInstanceOf($securityPage->getControllerName(), $result); $this->assertEquals($request, $result->getRequest());