tonyair
/
silverstripe-framework
mirror of https://github.com/silverstripe/silverstripe-framework
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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

This commit is contained in:
Damian Mooyman 2017-11-30 16:56:18 +13:00
parent d57dea0318
commit f1dd3d6f03
No known key found for this signature in database
GPG Key ID: 78B823A10DE27D1A
4 changed files with 53 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
src/Security/LoginAttempt.php
View File

@ -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),
));
}
}

3
src/Security/Member.php
View File

@ -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);

3
tests/php/Security/MemberAuthenticatorTest.php
View File

@ -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);
}

51
tests/php/Security/SecurityTest.php
View File

@ -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());