From b58e2dbe3a1b94c6f69c6b708761f1dce71c0429 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 11 Jul 2013 01:17:02 +0200 Subject: [PATCH] Member.lock_out_delay_mins configurable, password security docs --- docs/en/topics/security.md | 34 ++++++++++++--- lang/en.yml | 1 + security/Member.php | 28 +++++++++--- tests/security/SecurityTest.php | 77 ++++++++++++++++++++------------- 4 files changed, 97 insertions(+), 43 deletions(-) diff --git a/docs/en/topics/security.md b/docs/en/topics/security.md index 59e8deaf6..3e64d5b6c 100644 --- a/docs/en/topics/security.md +++ b/docs/en/topics/security.md @@ -407,13 +407,37 @@ configuration and test fixtures). You should therefore block access to all yaml files (extension .yml) by default, and white list only yaml files you need to serve directly. -See [Apache](/installation/webserver) and [Nginx](/installation/nginx) installation documentation for details -specific to your web server +See [Apache](/installation/webserver) and [Nginx](/installation/nginx) installation documentation for details specific to your web server + +## Passwords + +SilverStripe stores passwords with a strong hashing algorithm (blowfish) by default +(see [api:PasswordEncryptor]). It adds randomness to these hashes via +salt values generated with the strongest entropy generators available on the platform +(see [api:RandomGenerator]). This prevents brute force attacks with +[Rainbow tables](http://en.wikipedia.org/wiki/Rainbow_table). + +Strong passwords are a crucial part of any system security. +So in addition to storing the password in a secure fashion, +you can also enforce specific password policies by configuring +a [api:PasswordValidator]: + + :::php + $validator = new PasswordValidator(); + $validator->minLength(7); + $validator->checkHistoricalPasswords(6); + $validator->characterStrength('lowercase','uppercase','digits','punctuation'); + Member::set_password_validator($validator); + +In addition, you can tighten password security with the following configuration settings: + + * `Member.password_expiry_days`: Set the number of days that a password should be valid for. + * `Member.lock_out_after_incorrect_logins`: Number of incorrect logins after which + the user is blocked from further attempts for the timespan defined in `$lock_out_delay_mins` + * `Member.lock_out_delay_mins`: Minutes of enforced lockout after incorrect password attempts. + Only applies if `lock_out_after_incorrect_logins` is greater than 0. ## Related * [http://silverstripe.org/security-releases/](http://silverstripe.org/security-releases/) - -## Links - * [Best-practices for securing MySQL (securityfocus.com)](http://www.securityfocus.com/infocus/1726) diff --git a/lang/en.yml b/lang/en.yml index 64df41040..154dc2ad4 100644 --- a/lang/en.yml +++ b/lang/en.yml @@ -367,6 +367,7 @@ en: EMPTYNEWPASSWORD: 'The new password can''t be empty, please try again' ENTEREMAIL: 'Please enter an email address to get a password reset link.' ERRORLOCKEDOUT: 'Your account has been temporarily disabled because of too many failed attempts at logging in. Please try again in 20 minutes.' + ERRORLOCKEDOUT2: 'Your account has been temporarily disabled because of too many failed attempts at logging in. Please try again in {count} minutes.' ERRORNEWPASSWORD: 'You have entered your new password differently, try again' ERRORPASSWORDNOTMATCH: 'Your current password does not match, please try again' ERRORWRONGCRED: 'That doesn''t seem to be the right e-mail address or password. Please try again.' diff --git a/security/Member.php b/security/Member.php index 3afbd5361..90afb5101 100644 --- a/security/Member.php +++ b/security/Member.php @@ -113,9 +113,18 @@ class Member extends DataObject implements TemplateGlobalProvider { /** * @config - * @var Int + * @var Int Number of incorrect logins after which + * the user is blocked from further attempts for the timespan + * defined in {@link $lock_out_delay_mins}. */ private static $lock_out_after_incorrect_logins = null; + + /** + * @config + * @var integer Minutes of enforced lockout after incorrect password attempts. + * Only applies if {@link $lock_out_after_incorrect_logins} greater than 0. + */ + private static $lock_out_delay_mins = 15; /** * @config @@ -238,11 +247,15 @@ class Member extends DataObject implements TemplateGlobalProvider { $result = new ValidationResult(); if($this->isLockedOut()) { - $result->error(_t ( - 'Member.ERRORLOCKEDOUT', - 'Your account has been temporarily disabled because of too many failed attempts at ' . - 'logging in. Please try again in 20 minutes.' - )); + $result->error( + _t( + 'Member.ERRORLOCKEDOUT2', + 'Your account has been temporarily disabled because of too many failed attempts at ' . + 'logging in. Please try again in {count} minutes.', + null, + array('count' => $this->config()->lock_out_delay_mins) + ) + ); } $this->extend('canLogIn', $result); @@ -1407,7 +1420,8 @@ class Member extends DataObject implements TemplateGlobalProvider { $this->write(); if($this->FailedLoginCount >= self::config()->lock_out_after_incorrect_logins) { - $this->LockedOutUntil = date('Y-m-d H:i:s', time() + 15*60); + $lockoutMins = self::config()->lock_out_delay_mins; + $this->LockedOutUntil = date('Y-m-d H:i:s', time() + $lockoutMins*60); $this->write(); } } diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index 294b082b4..0f76c7ac6 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -250,61 +250,76 @@ class SecurityTest extends FunctionalTest { i18n::set_locale('en_US'); Member::config()->lock_out_after_incorrect_logins = 5; + Member::config()->lock_out_delay_mins = 15; - /* LOG IN WITH A BAD PASSWORD 7 TIMES */ - - for($i=1;$i<=7;$i++) { + // Login with a wrong password for more than the defined threshold + for($i = 1; $i <= Member::config()->lock_out_after_incorrect_logins+1; $i++) { $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); $member = DataObject::get_by_id("Member", $this->idFromFixture('Member', 'test')); - /* THE FIRST 4 TIMES, THE MEMBER SHOULDN'T BE LOCKED OUT */ - if($i < 5) { - $this->assertNull($member->LockedOutUntil); + if($i < Member::config()->lock_out_after_incorrect_logins) { + $this->assertNull( + $member->LockedOutUntil, + 'User does not have a lockout time set if under threshold for failed attempts' + ); $this->assertContains($this->loginErrorMessage(), _t('Member.ERRORWRONGCRED')); + } else { + // Fuzzy matching for time to avoid side effects from slow running tests + $this->assertGreaterThan( + time() + 14*60, + strtotime($member->LockedOutUntil), + 'User has a lockout time set after too many failed attempts' + ); } - - /* AFTER THAT THE USER IS LOCKED OUT FOR 15 MINUTES */ - //(we check for at least 14 minutes because we don't want a slow running test to report a failure.) - else { - $this->assertGreaterThan(time() + 14*60, strtotime($member->LockedOutUntil)); - } - - if($i > 5) { - $this->assertContains(_t('Member.ERRORLOCKEDOUT'), $this->loginErrorMessage()); - // $this->assertTrue(false !== stripos($this->loginErrorMessage(), _t('Member.ERRORLOCKEDOUT'))); + $msg = _t( + 'Member.ERRORLOCKEDOUT2', + 'Your account has been temporarily disabled because of too many failed attempts at ' . + 'logging in. Please try again in {count} minutes.', + null, + array('count' => Member::config()->lock_out_delay_mins) + ); + if($i > Member::config()->lock_out_after_incorrect_logins) { + $this->assertContains($msg, $this->loginErrorMessage()); } } - /* THE USER CAN'T LOG IN NOW, EVEN IF THEY GET THE RIGHT PASSWORD */ - $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); - $this->assertNull($this->session()->inst_get('loggedInAs')); + $this->assertNull( + $this->session()->inst_get('loggedInAs'), + 'The user can\'t log in after being locked out, even with the right password' + ); - /* BUT, IF TIME PASSES, THEY CAN LOG IN */ - // (We fake this by re-setting LockedOutUntil) $member = DataObject::get_by_id("Member", $this->idFromFixture('Member', 'test')); $member->LockedOutUntil = date('Y-m-d H:i:s', time() - 30); $member->write(); - $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); - $this->assertEquals($this->session()->inst_get('loggedInAs'), $member->ID); + $this->assertEquals( + $this->session()->inst_get('loggedInAs'), + $member->ID, + 'After lockout expires, the user can login again' + ); // Log the user out $this->session()->inst_set('loggedInAs', null); - /* NOW THAT THE LOCK-OUT HAS EXPIRED, CHECK THAT WE ARE ALLOWED 4 FAILED ATTEMPTS BEFORE LOGGING IN */ - - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); + // Login again with wrong password, but less attempts than threshold + for($i = 1; $i < Member::config()->lock_out_after_incorrect_logins; $i++) { + $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); + } $this->assertNull($this->session()->inst_get('loggedInAs')); - $this->assertTrue(false !== stripos($this->loginErrorMessage(), _t('Member.ERRORWRONGCRED'))); + $this->assertTrue( + false !== stripos($this->loginErrorMessage(), _t('Member.ERRORWRONGCRED')), + 'The user can retry with a wrong password after the lockout expires' + ); $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); - $this->assertEquals($this->session()->inst_get('loggedInAs'), $member->ID); + $this->assertEquals( + $this->session()->inst_get('loggedInAs'), + $member->ID, + 'The user can login successfully after lockout expires, if staying below the threshold' + ); i18n::set_locale($local); }