From d935140a9528a3a42323b51d84fb2bcd3da065a7 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 9 Apr 2018 11:06:05 +1200 Subject: [PATCH 01/12] [ss-2018-005] Prevent unauthenticated isDev / isTest being allowed --- .../Startup/ParameterConfirmationToken.php | 1 + .../ParameterConfirmationTokenTest.php | 27 +++++++++++-------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/Core/Startup/ParameterConfirmationToken.php b/src/Core/Startup/ParameterConfirmationToken.php index a5069102c..1c80db1d0 100644 --- a/src/Core/Startup/ParameterConfirmationToken.php +++ b/src/Core/Startup/ParameterConfirmationToken.php @@ -214,6 +214,7 @@ class ParameterConfirmationToken */ public function suppress() { + unset($_GET[$this->parameterName]); $this->request->offsetUnset($this->parameterName); } diff --git a/tests/php/Core/Startup/ParameterConfirmationTokenTest.php b/tests/php/Core/Startup/ParameterConfirmationTokenTest.php index b3ae253d0..66616433f 100644 --- a/tests/php/Core/Startup/ParameterConfirmationTokenTest.php +++ b/tests/php/Core/Startup/ParameterConfirmationTokenTest.php @@ -20,17 +20,17 @@ class ParameterConfirmationTokenTest extends SapphireTest protected function setUp() { parent::setUp(); - $get = []; - $get['parameterconfirmationtokentest_notoken'] = 'value'; - $get['parameterconfirmationtokentest_empty'] = ''; - $get['parameterconfirmationtokentest_withtoken'] = '1'; - $get['parameterconfirmationtokentest_withtokentoken'] = 'dummy'; - $get['parameterconfirmationtokentest_nulltoken'] = '1'; - $get['parameterconfirmationtokentest_nulltokentoken'] = null; - $get['parameterconfirmationtokentest_emptytoken'] = '1'; - $get['parameterconfirmationtokentest_emptytokentoken'] = ''; - $get['BackURL'] = 'page?parameterconfirmationtokentest_backtoken=1'; - $this->request = new HTTPRequest('GET', 'anotherpage', $get); + $_GET = []; + $_GET['parameterconfirmationtokentest_notoken'] = 'value'; + $_GET['parameterconfirmationtokentest_empty'] = ''; + $_GET['parameterconfirmationtokentest_withtoken'] = '1'; + $_GET['parameterconfirmationtokentest_withtokentoken'] = 'dummy'; + $_GET['parameterconfirmationtokentest_nulltoken'] = '1'; + $_GET['parameterconfirmationtokentest_nulltokentoken'] = null; + $_GET['parameterconfirmationtokentest_emptytoken'] = '1'; + $_GET['parameterconfirmationtokentest_emptytokentoken'] = ''; + $_GET['BackURL'] = 'page?parameterconfirmationtokentest_backtoken=1'; + $this->request = new HTTPRequest('GET', 'anotherpage', $_GET); $this->request->setSession(new Session([])); } @@ -129,6 +129,11 @@ class ParameterConfirmationTokenTest extends SapphireTest $this->request ); $this->assertEquals('parameterconfirmationtokentest_backtoken', $token->getName()); + + // Test prepare_tokens() unsets $_GET vars + $this->assertArrayNotHasKey('parameterconfirmationtokentest_notoken', $_GET); + $this->assertArrayNotHasKey('parameterconfirmationtokentest_empty', $_GET); + $this->assertArrayNotHasKey('parameterconfirmationtokentest_noparam', $_GET); } public function dataProviderURLs() From 2e13ae746fd16664e16412a7b904378369db0cf7 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 10 Apr 2018 13:46:08 +1200 Subject: [PATCH 02/12] [ss-2018-006] Prevent code execution in template value resolution --- src/Dev/FixtureBlueprint.php | 2 +- src/Forms/GridField/GridFieldDataColumns.php | 2 +- src/ORM/Hierarchy/MarkedSet.php | 2 +- src/View/SSViewer_DataPresenter.php | 2 +- tests/php/View/SSViewerTest.php | 10 ++++++++++ 5 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/Dev/FixtureBlueprint.php b/src/Dev/FixtureBlueprint.php index 9d219a36f..47e6a1805 100644 --- a/src/Dev/FixtureBlueprint.php +++ b/src/Dev/FixtureBlueprint.php @@ -121,7 +121,7 @@ class FixtureBlueprint continue; } - if (is_callable($fieldVal)) { + if (!is_string($fieldVal) && is_callable($fieldVal)) { $obj->$fieldName = $fieldVal($obj, $data, $fixtures); } else { $obj->$fieldName = $fieldVal; diff --git a/src/Forms/GridField/GridFieldDataColumns.php b/src/Forms/GridField/GridFieldDataColumns.php index 623515323..c8dd223da 100644 --- a/src/Forms/GridField/GridFieldDataColumns.php +++ b/src/Forms/GridField/GridFieldDataColumns.php @@ -281,7 +281,7 @@ class GridFieldDataColumns implements GridField_ColumnProvider } $spec = $this->fieldFormatting[$fieldName]; - if (is_callable($spec)) { + if (!is_string($spec) && is_callable($spec)) { return $spec($value, $item); } else { $format = str_replace('$value', "__VAL__", $spec); diff --git a/src/ORM/Hierarchy/MarkedSet.php b/src/ORM/Hierarchy/MarkedSet.php index 96fd23602..52b287a63 100644 --- a/src/ORM/Hierarchy/MarkedSet.php +++ b/src/ORM/Hierarchy/MarkedSet.php @@ -333,7 +333,7 @@ class MarkedSet $parentNode->setField('markingClasses', $this->markingClasses($data['node'])); // Evaluate custom context - if (is_callable($context)) { + if (!is_array($context) && is_callable($context)) { $context = call_user_func($context, $data['node']); } if ($context) { diff --git a/src/View/SSViewer_DataPresenter.php b/src/View/SSViewer_DataPresenter.php index cf609d117..f8d120cd8 100644 --- a/src/View/SSViewer_DataPresenter.php +++ b/src/View/SSViewer_DataPresenter.php @@ -326,7 +326,7 @@ class SSViewer_DataPresenter extends SSViewer_Scope $override = $overrides[$property]; // Late-evaluate this value - if (is_callable($override)) { + if (!is_string($override) && is_callable($override)) { $override = $override(); // Late override may yet return null diff --git a/tests/php/View/SSViewerTest.php b/tests/php/View/SSViewerTest.php index 098a2272b..8f79afe14 100644 --- a/tests/php/View/SSViewerTest.php +++ b/tests/php/View/SSViewerTest.php @@ -109,6 +109,16 @@ class SSViewerTest extends SapphireTest $this->assertEquals('Test partial template: var value', trim(preg_replace("//U", '', $result))); } + /** + * Ensure global methods aren't executed + */ + public function testTemplateExecution() + { + $data = new ArrayData([ 'Var' => 'phpinfo' ]); + $result = $data->renderWith("SSViewerTestPartialTemplate"); + $this->assertEquals('Test partial template: phpinfo', trim(preg_replace("//U", '', $result))); + } + public function testIncludeScopeInheritance() { $data = $this->getScopeInheritanceTestData(); From cd716fb61b58ac9e059310c223491e366a47ee23 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Thu, 10 May 2018 10:32:00 +1200 Subject: [PATCH 03/12] Switch check for is_string --- src/ORM/Hierarchy/MarkedSet.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ORM/Hierarchy/MarkedSet.php b/src/ORM/Hierarchy/MarkedSet.php index 52b287a63..a2adda78d 100644 --- a/src/ORM/Hierarchy/MarkedSet.php +++ b/src/ORM/Hierarchy/MarkedSet.php @@ -333,7 +333,7 @@ class MarkedSet $parentNode->setField('markingClasses', $this->markingClasses($data['node'])); // Evaluate custom context - if (!is_array($context) && is_callable($context)) { + if (!is_string($context) && is_callable($context)) { $context = call_user_func($context, $data['node']); } if ($context) { From 9053014a7e2eba28d000881e0bb3cc1d6e6b2eea Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 10 Apr 2018 16:45:14 +1200 Subject: [PATCH 04/12] [ss-2018-008] Validate against malformed urls --- src/Control/Director.php | 25 +++++++++++++++++++------ tests/php/Control/DirectorTest.php | 4 ++++ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/Control/Director.php b/src/Control/Director.php index 316148578..88908ef6e 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -728,13 +728,26 @@ class Director implements TemplateGlobalProvider */ public static function is_site_url($url) { - $urlHost = parse_url($url, PHP_URL_HOST); - $actualHost = parse_url(self::protocolAndHost(), PHP_URL_HOST); - if ($urlHost && $actualHost && $urlHost == $actualHost) { - return true; - } else { - return self::is_relative_url($url); + $parsedURL = parse_url($url); + + // Validate user (disallow slashes) + if (!empty($parsedURL['user']) && strstr($parsedURL['user'], '\\')) { + return false; } + if (!empty($parsedURL['pass']) && strstr($parsedURL['pass'], '\\')) { + return false; + } + + // Validate host[:port] + $actualHost = parse_url(self::protocolAndHost(), PHP_URL_HOST); + if (!empty($parsedURL['host']) + && $actualHost + && $parsedURL['host'] === $actualHost + ) { + return true; + } + + return self::is_relative_url($url); } /** diff --git a/tests/php/Control/DirectorTest.php b/tests/php/Control/DirectorTest.php index eae00b2b8..9a1de2ec4 100644 --- a/tests/php/Control/DirectorTest.php +++ b/tests/php/Control/DirectorTest.php @@ -380,6 +380,10 @@ class DirectorTest extends SapphireTest $this->assertFalse(Director::is_site_url("http://test.com?url=" . Director::absoluteBaseURL())); $this->assertFalse(Director::is_site_url("http://test.com?url=" . urlencode(Director::absoluteBaseURL()))); $this->assertFalse(Director::is_site_url("//test.com?url=" . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url('http://google.com\@test.com')); + $this->assertFalse(Director::is_site_url('http://google.com/@test.com')); + $this->assertFalse(Director::is_site_url('http://google.com:pass\@test.com')); + $this->assertFalse(Director::is_site_url('http://google.com:pass/@test.com')); } /** From e409d6f673c49846086b23677aecdc3fde5fc4d5 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 11 Apr 2018 13:04:22 +1200 Subject: [PATCH 05/12] [ss-2018-001] Restrict non-admins from being assigned to admin groups --- src/Security/Member.php | 101 ++++++++++++++++++------------ tests/php/Security/MemberTest.php | 38 ++++++++++- 2 files changed, 97 insertions(+), 42 deletions(-) diff --git a/src/Security/Member.php b/src/Security/Member.php index 40bdd9389..f1cf2b568 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -33,6 +33,7 @@ use SilverStripe\ORM\HasManyList; use SilverStripe\ORM\ManyManyList; use SilverStripe\ORM\Map; use SilverStripe\ORM\SS_List; +use SilverStripe\ORM\UnsavedRelationList; use SilverStripe\ORM\ValidationException; use SilverStripe\ORM\ValidationResult; @@ -60,27 +61,26 @@ use SilverStripe\ORM\ValidationResult; */ class Member extends DataObject { - private static $db = array( - 'FirstName' => 'Varchar', - 'Surname' => 'Varchar', - 'Email' => 'Varchar(254)', // See RFC 5321, Section 4.5.3.1.3. (256 minus the < and > character) - 'TempIDHash' => 'Varchar(160)', // Temporary id used for cms re-authentication - 'TempIDExpired' => 'Datetime', // Expiry of temp login - 'Password' => 'Varchar(160)', - 'AutoLoginHash' => 'Varchar(160)', // Used to auto-login the user on password reset - 'AutoLoginExpired' => 'Datetime', + 'FirstName' => 'Varchar', + 'Surname' => 'Varchar', + 'Email' => 'Varchar(254)', // See RFC 5321, Section 4.5.3.1.3. (256 minus the < and > character) + 'TempIDHash' => 'Varchar(160)', // Temporary id used for cms re-authentication + 'TempIDExpired' => 'Datetime', // Expiry of temp login + 'Password' => 'Varchar(160)', + 'AutoLoginHash' => 'Varchar(160)', // Used to auto-login the user on password reset + 'AutoLoginExpired' => 'Datetime', // This is an arbitrary code pointing to a PasswordEncryptor instance, // not an actual encryption algorithm. // Warning: Never change this field after its the first password hashing without // providing a new cleartext password as well. 'PasswordEncryption' => "Varchar(50)", - 'Salt' => 'Varchar(50)', - 'PasswordExpiry' => 'Date', - 'LockedOutUntil' => 'Datetime', - 'Locale' => 'Varchar(6)', + 'Salt' => 'Varchar(50)', + 'PasswordExpiry' => 'Date', + 'LockedOutUntil' => 'Datetime', + 'Locale' => 'Varchar(6)', // handled in registerFailedLogin(), only used if $lock_out_after_incorrect_logins is set - 'FailedLoginCount' => 'Int', + 'FailedLoginCount' => 'Int', ); private static $belongs_many_many = array( @@ -88,7 +88,7 @@ class Member extends DataObject ); private static $has_many = array( - 'LoggedPasswords' => MemberPassword::class, + 'LoggedPasswords' => MemberPassword::class, 'RememberLoginHashes' => RememberLoginHash::class, ); @@ -312,7 +312,7 @@ class Member extends DataObject break; } } - return $result; + return $result; } /** @@ -521,10 +521,9 @@ class Member extends DataObject 'This method is deprecated and now does not add value. Please use Security::getCurrentUser()' ); - if ($member = Security::getCurrentUser()) { - if ($member && $member->exists()) { - return true; - } + $member = Security::getCurrentUser(); + if ($member && $member->exists()) { + return true; } return false; @@ -655,7 +654,7 @@ class Member extends DataObject { /** @var Member $member */ $member = static::get()->filter([ - 'AutoLoginHash' => $hash, + 'AutoLoginHash' => $hash, 'AutoLoginExpired:GreaterThan' => DBDatetime::now()->getValue(), ])->first(); @@ -799,6 +798,7 @@ class Member extends DataObject * @param Member|null|int $member Member or member ID to log in as. * Set to null or 0 to act as a logged out user. * @param callable $callback + * @return mixed Result of $callback */ public static function actAs($member, $callback) { @@ -831,11 +831,11 @@ class Member extends DataObject 'This method is deprecated. Please use Security::getCurrentUser() or an IdentityStore' ); - if ($member = Security::getCurrentUser()) { + $member = Security::getCurrentUser(); + if ($member) { return $member->ID; - } else { - return 0; } + return 0; } /** @@ -892,8 +892,8 @@ class Member extends DataObject 'Can\'t overwrite existing member #{id} with identical identifier ({name} = {value}))', 'Values in brackets show "fieldname = value", usually denoting an existing email address', array( - 'id' => $existingRecord->ID, - 'name' => $identifierField, + 'id' => $existingRecord->ID, + 'name' => $identifierField, 'value' => $this->$identifierField ) )); @@ -912,7 +912,11 @@ class Member extends DataObject ->setHTMLTemplate('SilverStripe\\Control\\Email\\ChangePasswordEmail') ->setData($this) ->setTo($this->Email) - ->setSubject(_t(__CLASS__ . '.SUBJECTPASSWORDCHANGED', "Your password has been changed", 'Email subject')) + ->setSubject(_t( + __CLASS__ . '.SUBJECTPASSWORDCHANGED', + "Your password has been changed", + 'Email subject' + )) ->send(); } @@ -973,17 +977,26 @@ class Member extends DataObject * @return bool True if the change can be accepted */ public function onChangeGroups($ids) + { + // Ensure none of these match disallowed list + $disallowedGroupIDs = $this->disallowedGroups(); + return count(array_intersect($ids, $disallowedGroupIDs)) == 0; + } + + /** + * List of group IDs this user is disallowed from + * + * @return int[] List of group IDs + */ + protected function disallowedGroups() { // unless the current user is an admin already OR the logged in user is an admin if (Permission::check('ADMIN') || Permission::checkMember($this, 'ADMIN')) { - return true; + return []; } - // If there are no admin groups in this set then it's ok - $adminGroups = Permission::get_groups_by_permission('ADMIN'); - $adminGroupIDs = ($adminGroups) ? $adminGroups->column('ID') : array(); - - return count(array_intersect($ids, $adminGroupIDs)) == 0; + // Non-admins may not belong to admin groups + return Permission::get_groups_by_permission('ADMIN')->column('ID'); } @@ -1159,7 +1172,7 @@ class Member extends DataObject if (!$format) { $format = [ 'columns' => ['Surname', 'FirstName'], - 'sep' => ' ', + 'sep' => ' ', ]; } @@ -1287,7 +1300,7 @@ class Member extends DataObject } /** - * @return ManyManyList + * @return ManyManyList|UnsavedRelationList */ public function DirectGroups() { @@ -1468,8 +1481,14 @@ class Member extends DataObject $fields->removeByName('RememberLoginHashes'); if (Permission::check('EDIT_PERMISSIONS')) { + // Filter allowed groups + $groups = Group::get(); + $disallowedGroupIDs = $this->disallowedGroups(); + if ($disallowedGroupIDs) { + $groups = $groups->exclude('ID', $disallowedGroupIDs); + } $groupsMap = array(); - foreach (Group::get() as $group) { + foreach ($groups as $group) { // Listboxfield values are escaped, use ASCII char instead of » $groupsMap[$group->ID] = $group->getBreadcrumbs(' > '); } @@ -1524,7 +1543,11 @@ class Member extends DataObject /** @skipUpgrade */ $labels['Email'] = _t(__CLASS__ . '.EMAIL', 'Email'); $labels['Password'] = _t(__CLASS__ . '.db_Password', 'Password'); - $labels['PasswordExpiry'] = _t(__CLASS__ . '.db_PasswordExpiry', 'Password Expiry Date', 'Password expiry date'); + $labels['PasswordExpiry'] = _t( + __CLASS__ . '.db_PasswordExpiry', + 'Password Expiry Date', + 'Password expiry date' + ); $labels['LockedOutUntil'] = _t(__CLASS__ . '.db_LockedOutUntil', 'Locked out until', 'Security related date'); $labels['Locale'] = _t(__CLASS__ . '.db_Locale', 'Interface Locale'); if ($includerelations) { @@ -1673,8 +1696,8 @@ class Member extends DataObject * * This method will encrypt the password prior to writing. * - * @param string $password Cleartext password - * @param bool $write Whether to write the member afterwards + * @param string $password Cleartext password + * @param bool $write Whether to write the member afterwards * @return ValidationResult */ public function changePassword($password, $write = true) diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index 8476380ca..ccb069ad9 100644 --- a/tests/php/Security/MemberTest.php +++ b/tests/php/Security/MemberTest.php @@ -7,6 +7,7 @@ use SilverStripe\Core\Config\Config; use SilverStripe\Core\Convert; use SilverStripe\Core\Injector\Injector; use SilverStripe\Dev\FunctionalTest; +use SilverStripe\Forms\ListboxField; use SilverStripe\i18n\i18n; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DB; @@ -20,7 +21,6 @@ use SilverStripe\Security\Member_Validator; use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator; use SilverStripe\Security\MemberAuthenticator\SessionAuthenticationHandler; use SilverStripe\Security\MemberPassword; -use SilverStripe\Security\PasswordEncryptor_Blowfish; use SilverStripe\Security\Permission; use SilverStripe\Security\RememberLoginHash; use SilverStripe\Security\Security; @@ -656,6 +656,8 @@ class MemberTest extends FunctionalTest $staffMember = $this->objFromFixture(Member::class, 'staffmember'); /** @var Member $adminMember */ $adminMember = $this->objFromFixture(Member::class, 'admin'); + + // Construct admin and non-admin gruops $newAdminGroup = new Group(array('Title' => 'newadmin')); $newAdminGroup->write(); Permission::grant($newAdminGroup->ID, 'ADMIN'); @@ -688,6 +690,37 @@ class MemberTest extends FunctionalTest ); } + /** + * Ensure DirectGroups listbox disallows admin-promotion + */ + public function testAllowedGroupsListbox() + { + /** @var Group $adminGroup */ + $adminGroup = $this->objFromFixture(Group::class, 'admingroup'); + /** @var Member $staffMember */ + $staffMember = $this->objFromFixture(Member::class, 'staffmember'); + /** @var Member $adminMember */ + $adminMember = $this->objFromFixture(Member::class, 'admin'); + + // Ensure you can see the DirectGroups box + $this->logInWithPermission('EDIT_PERMISSIONS'); + + // Non-admin member field contains non-admin groups + /** @var ListboxField $staffListbox */ + $staffListbox = $staffMember->getCMSFields()->dataFieldByName('DirectGroups'); + $this->assertArrayNotHasKey($adminGroup->ID, $staffListbox->getSource()); + + // admin member field contains admin group + /** @var ListboxField $adminListbox */ + $adminListbox = $adminMember->getCMSFields()->dataFieldByName('DirectGroups'); + $this->assertArrayHasKey($adminGroup->ID, $adminListbox->getSource()); + + // If logged in as admin, staff listbox has admin group + $this->logInWithPermission('ADMIN'); + $staffListbox = $staffMember->getCMSFields()->dataFieldByName('DirectGroups'); + $this->assertArrayHasKey($adminGroup->ID, $staffListbox->getSource()); + } + /** * Test Member_GroupSet::add */ @@ -866,8 +899,6 @@ class MemberTest extends FunctionalTest public function testValidateAutoLoginToken() { - $enc = new PasswordEncryptor_Blowfish(); - $m1 = new Member(); $m1->PasswordEncryption = 'blowfish'; $m1->Salt = $enc->salt('123'); @@ -1463,6 +1494,7 @@ class MemberTest extends FunctionalTest public function testChangePasswordWithExtensionsThatModifyValidationResult() { // Default behaviour + /** @var Member $member */ $member = $this->objFromFixture(Member::class, 'admin'); $result = $member->changePassword('my-secret-new-password'); $this->assertInstanceOf(ValidationResult::class, $result); From beec0c0d47d542c6d5b7aa1d32f49cd6fec326cc Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Mon, 23 Apr 2018 10:29:10 +1200 Subject: [PATCH 06/12] [SS-2018-010] Fix regression of SS-2017-002 --- src/Security/MemberAuthenticator/MemberAuthenticator.php | 9 ++++++++- tests/php/Security/MemberAuthenticatorTest.php | 2 -- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/Security/MemberAuthenticator/MemberAuthenticator.php b/src/Security/MemberAuthenticator/MemberAuthenticator.php index 91dbd50a9..c51cecfd3 100644 --- a/src/Security/MemberAuthenticator/MemberAuthenticator.php +++ b/src/Security/MemberAuthenticator/MemberAuthenticator.php @@ -91,6 +91,11 @@ class MemberAuthenticator implements Authenticator // Validate against member if possible if ($member && !$asDefaultAdmin) { $this->checkPassword($member, $data['Password'], $result); + } elseif (!$asDefaultAdmin) { + // spoof a login attempt + $tempMember = Member::create(); + $tempMember->{Member::config()->get('unique_identifier_field')} = $email; + $tempMember->validateCanLogin($result); } // Emit failure to member and form (if available) @@ -164,7 +169,9 @@ class MemberAuthenticator implements Authenticator */ protected function recordLoginAttempt($data, HTTPRequest $request, $member, $success) { - if (!Security::config()->get('login_recording')) { + if (!Security::config()->get('login_recording') + && !Member::config()->get('lock_out_after_incorrect_logins') + ) { return; } diff --git a/tests/php/Security/MemberAuthenticatorTest.php b/tests/php/Security/MemberAuthenticatorTest.php index 3aa4e1d0e..4314cf558 100644 --- a/tests/php/Security/MemberAuthenticatorTest.php +++ b/tests/php/Security/MemberAuthenticatorTest.php @@ -243,7 +243,6 @@ class MemberAuthenticatorTest extends SapphireTest public function testNonExistantMemberGetsLoginAttemptRecorded() { - Security::config()->set('login_recording', true); Member::config() ->set('lock_out_after_incorrect_logins', 1) ->set('lock_out_delay_mins', 10); @@ -272,7 +271,6 @@ class MemberAuthenticatorTest extends SapphireTest public function testNonExistantMemberGetsLockedOut() { - Security::config()->set('login_recording', true); Member::config() ->set('lock_out_after_incorrect_logins', 1) ->set('lock_out_delay_mins', 10); From f847f186b10d4d58faa1b155f85eb2398b029125 Mon Sep 17 00:00:00 2001 From: Aaron Carlino Date: Tue, 24 Apr 2018 14:27:23 +1200 Subject: [PATCH 07/12] [ss-2018-013] Remove password text from session data on failed submission --- src/Forms/FormRequestHandler.php | 1 - src/Forms/PasswordField.php | 31 ++++++++++++- tests/php/Forms/FormRequestHandlerTest.php | 2 + tests/php/Forms/FormTest.php | 54 ++++++++++++++++++++++ tests/php/Forms/PasswordFieldTest.php | 46 ++++++++++++++++++ 5 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 tests/php/Forms/PasswordFieldTest.php diff --git a/src/Forms/FormRequestHandler.php b/src/Forms/FormRequestHandler.php index 0244a11f5..d743ee5a1 100644 --- a/src/Forms/FormRequestHandler.php +++ b/src/Forms/FormRequestHandler.php @@ -157,7 +157,6 @@ class FormRequestHandler extends RequestHandler "SilverStripe\\Forms\\Form.CSRF_EXPIRED_MESSAGE", "Your session has expired. Please re-submit the form." )); - // Return the user return $this->redirectBack(); } diff --git a/src/Forms/PasswordField.php b/src/Forms/PasswordField.php index cedfe0374..de357d403 100644 --- a/src/Forms/PasswordField.php +++ b/src/Forms/PasswordField.php @@ -19,6 +19,12 @@ class PasswordField extends TextField protected $inputType = 'password'; + /** + * If true, the field can accept a value attribute, e.g. from posted form data + * @var bool + */ + protected $allowValuePostback = false; + /** * Returns an input field. * @@ -39,12 +45,35 @@ class PasswordField extends TextField parent::__construct($name, $title, $value); } + /** + * @param bool $bool + * @return $this + */ + public function setAllowValuePostback($bool) + { + $this->allowValuePostback = (bool) $bool; + + return $this; + } + + /** + * @return bool + */ + public function getAllowValuePostback() + { + return $this->allowValuePostback; + } + /** * {@inheritdoc} */ public function getAttributes() { - $attributes = array(); + $attributes = []; + + if (!$this->getAllowValuePostback()) { + $attributes['value'] = null; + } $autocomplete = $this->config()->get('autocomplete'); diff --git a/tests/php/Forms/FormRequestHandlerTest.php b/tests/php/Forms/FormRequestHandlerTest.php index 09eb8b6a1..5ce4b3120 100644 --- a/tests/php/Forms/FormRequestHandlerTest.php +++ b/tests/php/Forms/FormRequestHandlerTest.php @@ -9,8 +9,10 @@ use SilverStripe\Dev\SapphireTest; use SilverStripe\Forms\FieldList; use SilverStripe\Forms\FormAction; use SilverStripe\Forms\FormRequestHandler; +use SilverStripe\Forms\PasswordField; use SilverStripe\Forms\Tests\FormRequestHandlerTest\TestForm; use SilverStripe\Forms\Tests\FormRequestHandlerTest\TestFormRequestHandler; +use SilverStripe\Forms\TextField; /** * @skipUpgrade diff --git a/tests/php/Forms/FormTest.php b/tests/php/Forms/FormTest.php index de6cb7aac..ab7a91367 100644 --- a/tests/php/Forms/FormTest.php +++ b/tests/php/Forms/FormTest.php @@ -3,6 +3,8 @@ namespace SilverStripe\Forms\Tests; use SilverStripe\Control\Session; +use SilverStripe\Core\Config\Config; +use SilverStripe\Forms\PasswordField; use SilverStripe\Forms\Tests\FormTest\TestController; use SilverStripe\Forms\Tests\FormTest\ControllerWithSecurityToken; use SilverStripe\Forms\Tests\FormTest\ControllerWithStrictPostCheck; @@ -10,6 +12,7 @@ use SilverStripe\Forms\Tests\FormTest\Player; use SilverStripe\Forms\Tests\FormTest\Team; use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\NullSecurityToken; +use SilverStripe\Security\Security; use SilverStripe\Security\SecurityToken; use SilverStripe\Security\RandomGenerator; use SilverStripe\Dev\CSSContentParser; @@ -59,6 +62,17 @@ class FormTest extends FunctionalTest ); } + /** + * @return array + */ + public function boolDataProvider() + { + return [ + [false], + [true], + ]; + } + public function testLoadDataFromRequest() { $form = new Form( @@ -915,6 +929,46 @@ class FormTest extends FunctionalTest $this->assertEmpty($formData['ExtraFieldCheckbox']); } + /** + * @dataProvider boolDataProvider + * @param bool $allow + */ + public function testPasswordPostback($allow) + { + $form = $this->getStubForm(); + $form->enableSecurityToken(); + $form->Fields()->push( + PasswordField::create('Password') + ->setAllowValuePostback($allow) + ); + $form->Actions()->push(FormAction::create('doSubmit')); + $request = new HTTPRequest( + 'POST', + 'FormTest_Controller/Form', + [], + [ + 'key1' => 'foo', + 'Password' => 'hidden', + SecurityToken::inst()->getName() => 'fail', + 'action_doSubmit' => 1, + ] + ); + $form->getRequestHandler()->httpSubmission($request); + $parser = new CSSContentParser($form->forTemplate()); + $passwords = $parser->getBySelector('input#Password'); + $this->assertNotNull($passwords); + $this->assertCount(1, $passwords); + /* @var \SimpleXMLElement $password */ + $password = $passwords[0]; + $attrs = iterator_to_array($password->attributes()); + if ($allow) { + $this->assertArrayHasKey('value', $attrs); + $this->assertEquals('hidden', $attrs['value']); + } else { + $this->assertArrayNotHasKey('value', $attrs); + } + } + protected function getStubForm() { return new Form( diff --git a/tests/php/Forms/PasswordFieldTest.php b/tests/php/Forms/PasswordFieldTest.php new file mode 100644 index 000000000..4dfb4a38c --- /dev/null +++ b/tests/php/Forms/PasswordFieldTest.php @@ -0,0 +1,46 @@ +set(PasswordField::class, 'autocomplete', $bool); + $field = new PasswordField('test'); + $attrs = $field->getAttributes(); + + $this->assertArrayHasKey('autocomplete', $attrs); + $this->assertEquals($bool ? 'on' : 'off', $attrs['autocomplete']); + } + + /** + * @dataProvider boolDataProvider + * @param bool $bool + */ + public function testValuePostback($bool) + { + $field = (new PasswordField('test', 'test', 'password')) + ->setAllowValuePostback($bool); + $attrs = $field->getAttributes(); + + $this->assertArrayHasKey('value', $attrs); + $this->assertEquals($bool ? 'password' : '', $attrs['value']); + } +} From 299131ed22c1afafe52136494e42acb11e270435 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 26 Apr 2018 15:37:50 +1200 Subject: [PATCH 08/12] [ss-2018-012] File security documentation --- .../14_Files/03_File_Security.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/en/02_Developer_Guides/14_Files/03_File_Security.md b/docs/en/02_Developer_Guides/14_Files/03_File_Security.md index 69f9de238..0e5201866 100644 --- a/docs/en/02_Developer_Guides/14_Files/03_File_Security.md +++ b/docs/en/02_Developer_Guides/14_Files/03_File_Security.md @@ -348,6 +348,24 @@ RewriteRule .* ../index.php [QSA] You will need to ensure that your core apache configuration has the necessary `AllowOverride` settings to support the local .htaccess file. +Although assets have a 404 handler which routes to a PHP handler, .php files within assets itself +should not be allowed to be marked as executable. + +When securing your server you should ensure that you protect against both files that can be uploaded as +executable on the server, as well as protect against accidental upload of `.htaccess` which bypasses +this file security. + +For instance your server configuration should look similar to the below: + +``` + + php_admin_flag engine off + +``` + +The `php_admin_flag` will protect against uploaded `.htaccess` files accidentally re-enabling script +execution within the assets directory. + #### Configuring Web Server: Windows IIS 7.5+ Configuring via IIS requires the Rewrite extension to be installed and configured properly. From bb1f0cce583333c7fef3d1c3f280462892f80716 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Thu, 24 May 2018 11:03:49 +1200 Subject: [PATCH 09/12] Added 4.0.4 changelog --- docs/en/04_Changelogs/4.0.4.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 docs/en/04_Changelogs/4.0.4.md diff --git a/docs/en/04_Changelogs/4.0.4.md b/docs/en/04_Changelogs/4.0.4.md new file mode 100644 index 000000000..a415772b9 --- /dev/null +++ b/docs/en/04_Changelogs/4.0.4.md @@ -0,0 +1,30 @@ +# 4.0.4 + +This security release removes the following file extensions from the default whitelist of accepted types for +uploaded files: `dotm`, `potm`, `jar`, `css`, `js` and `xltm`. + +If you require the ability to upload these file types in your projects, you will need to add them back in again. +For more information, see ["Configuring: File types"](https://docs.silverstripe.org/en/4/developer_guides/files/file_security/#configuring-file-types). + + + +## Change Log + +### Security + + * 2018-04-26 [299131ed2](https://github.com/silverstripe/silverstripe-framework/commit/299131ed2) File security documentation (Damian Mooyman) - See [ss-2018-012](http://www.silverstripe.org/download/security-releases/ss-2018-012) + * 2018-04-25 [be96858](https://github.com/silverstripe/silverstripe-installer/commit/be96858e85272ca62f6f0ff3e24a44aa0248ac4d) Remove jar, dotm, potm, xltm from file extension whitelist, hard-code CSS and JS for TinyMCE support (Robbie Averill) - See [ss-2018-014](http://www.silverstripe.org/download/security-releases/ss-2018-014) + * 2018-04-24 [f847f186b](https://github.com/silverstripe/silverstripe-framework/commit/f847f186b) Remove password text from session data on failed submission (Aaron Carlino) - See [ss-2018-013](http://www.silverstripe.org/download/security-releases/ss-2018-013) + * 2018-04-23 [aa365e0](https://github.com/silverstripe/silverstripe-assets/commit/aa365e0) Remove dotm, potm, jar, css, js, xltm from default File.allowed_extensions (Robbie Averill) - See [ss-2018-014](http://www.silverstripe.org/download/security-releases/ss-2018-014) + * 2018-04-23 [f9c03fa](https://github.com/silverstripe/silverstripe-installer/commit/f9c03fa623dc7237005901efd863256b7d356db7) Prevent php code execution in assets folder (Damian Mooyman) - See [ss-2018-012](http://www.silverstripe.org/download/security-releases/ss-2018-012) + * 2018-04-23 [1e27835](https://github.com/silverstripe/silverstripe-assets/commit/1e27835) Prevent php code execution in assets folder (Damian Mooyman) - See [ss-2018-012](http://www.silverstripe.org/download/security-releases/ss-2018-012) + * 2018-04-22 [beec0c0d4](https://github.com/silverstripe/silverstripe-framework/commit/beec0c0d4) regression of SS-2017-002 (Robbie Averill) - See [ss-2018-010](http://www.silverstripe.org/download/security-releases/ss-2018-010) + * 2018-04-11 [e409d6f67](https://github.com/silverstripe/silverstripe-framework/commit/e409d6f67) Restrict non-admins from being assigned to admin groups (Damian Mooyman) - See [ss-2018-001](http://www.silverstripe.org/download/security-releases/ss-2018-001) + * 2018-04-10 [9053014a7](https://github.com/silverstripe/silverstripe-framework/commit/9053014a7) Validate against malformed urls (Damian Mooyman) - See [ss-2018-008](http://www.silverstripe.org/download/security-releases/ss-2018-008) + * 2018-04-10 [2e13ae746](https://github.com/silverstripe/silverstripe-framework/commit/2e13ae746) Prevent code execution in template value resolution (Damian Mooyman) - See [ss-2018-006](http://www.silverstripe.org/download/security-releases/ss-2018-006) + * 2018-04-09 [db04ed9](https://github.com/silverstripe/silverstripe-admin/commit/db04ed9) Remove on* events as allowed properties (Damian Mooyman) - See [ss-2018-004](http://www.silverstripe.org/download/security-releases/ss-2018-004) + * 2018-04-08 [d935140a9](https://github.com/silverstripe/silverstripe-framework/commit/d935140a9) Prevent unauthenticated isDev / isTest being allowed (Damian Mooyman) - See [ss-2018-005](http://www.silverstripe.org/download/security-releases/ss-2018-005) + +### Bugfixes + + * 2018-02-13 [c6095cf](https://github.com/silverstripe/silverstripe-config/commit/c6095cfc0a07a74bb932e2191215d06f102e992a) word boundary issue with pathname matching (Christopher Joe) From e7e32d13a3eef694e3795f3af1a224d526961e4a Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Thu, 24 May 2018 11:24:48 +1200 Subject: [PATCH 10/12] FIX Add namespace and encryptor to tests that expect blowfish to be available --- tests/php/Security/MemberTest.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index ccb069ad9..162d845f9 100644 --- a/tests/php/Security/MemberTest.php +++ b/tests/php/Security/MemberTest.php @@ -21,6 +21,7 @@ use SilverStripe\Security\Member_Validator; use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator; use SilverStripe\Security\MemberAuthenticator\SessionAuthenticationHandler; use SilverStripe\Security\MemberPassword; +use SilverStripe\Security\PasswordEncryptor_Blowfish; use SilverStripe\Security\Permission; use SilverStripe\Security\RememberLoginHash; use SilverStripe\Security\Security; @@ -899,6 +900,8 @@ class MemberTest extends FunctionalTest public function testValidateAutoLoginToken() { + $enc = new PasswordEncryptor_Blowfish(); + $m1 = new Member(); $m1->PasswordEncryption = 'blowfish'; $m1->Salt = $enc->salt('123'); From 5bff64b47bfd5467b7dcdca983fa88b6946e8e2e Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 7 Feb 2018 11:03:32 +1300 Subject: [PATCH 11/12] BUG Fix Director::test() not persisting removed session keys on teardown --- src/Control/Director.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Control/Director.php b/src/Control/Director.php index 88908ef6e..d449f77d8 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -207,12 +207,17 @@ class Director implements TemplateGlobalProvider if ($session instanceof Session) { // Note: If passing $session as object, ensure that changes are written back // This is important for classes such as FunctionalTest which emulate cross-request persistence - $newVars['_SESSION'] = $session->getAll(); - $finally[] = function () use ($session) { + $newVars['_SESSION'] = $sessionArray = $session->getAll(); + $finally[] = function () use ($session, $sessionArray) { if (isset($_SESSION)) { + // Set new / updated keys foreach ($_SESSION as $key => $value) { $session->set($key, $value); } + // Unset removed keys + foreach (array_diff_key($sessionArray, $_SESSION) as $key => $value) { + $session->clear($key); + } } }; } else { From fe4f6f42d3f83ce5700a64b5062f9e693906af7f Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Thu, 24 May 2018 13:11:18 +1200 Subject: [PATCH 12/12] Updated 4.0.4 changelog --- docs/en/04_Changelogs/4.0.4.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/en/04_Changelogs/4.0.4.md b/docs/en/04_Changelogs/4.0.4.md index a415772b9..2176d85df 100644 --- a/docs/en/04_Changelogs/4.0.4.md +++ b/docs/en/04_Changelogs/4.0.4.md @@ -27,4 +27,6 @@ For more information, see ["Configuring: File types"](https://docs.silverstripe. ### Bugfixes + * 2018-05-23 [e7e32d13a](https://github.com/silverstripe/silverstripe-framework/commit/e7e32d13a) Add namespace and encryptor to tests that expect blowfish to be available (Robbie Averill) * 2018-02-13 [c6095cf](https://github.com/silverstripe/silverstripe-config/commit/c6095cfc0a07a74bb932e2191215d06f102e992a) word boundary issue with pathname matching (Christopher Joe) + * 2018-02-06 [5bff64b47](https://github.com/silverstripe/silverstripe-framework/commit/5bff64b47) Fix Director::test() not persisting removed session keys on teardown (Damian Mooyman)