[ss-2017-009] Prevent disclosure of sensitive information via LoginAttempt

Author: Damian Mooyman

src/Security/LoginAttempt.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Security;
 
+use SilverStripe\ORM\DataList;
 use SilverStripe\ORM\DataObject;
 
 /**
@@ -14,11 +15,11 @@
  * 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 @@ public function fieldLabels($includerelations = true)
     {
         $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),
+        ));
+    }
 }

src/Security/Member.php

@@ -384,8 +384,7 @@ public function isLockedOut()
         }
 
         $idField = static::config()->get('unique_identifier_field');
-        $attempts = LoginAttempt::get()
-            ->filter('Email', $this->{$idField})
+        $attempts = LoginAttempt::getByEmail($this->{$idField})
             ->sort('Created', 'DESC')
             ->limit($maxAttempts);
 

tests/php/Security/MemberAuthenticatorTest.php

@@ -265,7 +265,8 @@ public function testNonExistantMemberGetsLoginAttemptRecorded()
         $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);
     }
 

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 @@ public function testUnsuccessfulLoginAttempts()
         /* 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 @@ public function testSuccessfulLoginAttempts()
         /* 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 @@ public function testGetResponseController()
 
         // 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());