From 3bb435c241fbc3eb973d4262322106faf53f1d3a Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Thu, 11 Feb 2021 13:05:10 +1300 Subject: [PATCH 1/4] [CVE-2020-25817] Prevent loading of xml entities --- _config/dev.yml | 3 +++ src/Dev/CSSContentParser.php | 9 +++++++++ tests/php/Dev/CSSContentParserTest.php | 10 ++++++++++ 3 files changed, 22 insertions(+) diff --git a/_config/dev.yml b/_config/dev.yml index ca74c082f..4c1636bc4 100644 --- a/_config/dev.yml +++ b/_config/dev.yml @@ -17,3 +17,6 @@ SilverStripe\Dev\DevelopmentAdmin: controller: Silverstripe\Dev\DevConfigController links: config: 'View the current config, useful for debugging' + +SilverStripe\Dev\CSSContentParser: + disable_xml_external_entities: true diff --git a/src/Dev/CSSContentParser.php b/src/Dev/CSSContentParser.php index 3ebd9576b..c07f73785 100644 --- a/src/Dev/CSSContentParser.php +++ b/src/Dev/CSSContentParser.php @@ -2,6 +2,7 @@ namespace SilverStripe\Dev; +use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Injector\Injectable; use SimpleXMLElement; use tidy; @@ -25,6 +26,7 @@ use Exception; class CSSContentParser { use Injectable; + use Configurable; protected $simpleXML = null; @@ -56,6 +58,13 @@ class CSSContentParser $tidy = $content; } + // Prevent loading of external entities to prevent XXE attacks + // Note: as of libxml 2.9.0 entity substitution is disabled by default so this won't be required + if ($this->config()->get('disable_xml_external_entities')) { + libxml_set_external_entity_loader(function () { + return null; + }); + } $this->simpleXML = @simplexml_load_string($tidy, 'SimpleXMLElement', LIBXML_NOWARNING); if (!$this->simpleXML) { throw new Exception('CSSContentParser::__construct(): Could not parse content.' diff --git a/tests/php/Dev/CSSContentParserTest.php b/tests/php/Dev/CSSContentParserTest.php index 5377e7865..a9551abac 100644 --- a/tests/php/Dev/CSSContentParserTest.php +++ b/tests/php/Dev/CSSContentParserTest.php @@ -50,4 +50,14 @@ HTML $result = $parser->getBySelector('#A .other'); $this->assertEquals("result", $result[0] . ''); } + + public function testXmlEntitiesDisabled() + { + // CSSContentParser uses simplexml to parse html + // Ensure XML entities are not substituted in to prevent XXE attacks + $xml = ']>
Hello &myentity;
'; + $parser = new CSSContentParser($xml); + $div = $parser->getBySelector('div')[0]->asXML(); + $this->assertEquals('
Hello &myentity;
', $div); + } } From 06dbd5237b97891239b2b1334e3c03f08ba70eb0 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Fri, 19 Feb 2021 10:28:03 +1300 Subject: [PATCH 2/4] [CVE-2020-26138] Validate custom multi-file uploads --- src/Forms/FileField.php | 40 ++++++++++++++++++-- tests/php/Forms/FileFieldTest.php | 62 +++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 4 deletions(-) diff --git a/src/Forms/FileField.php b/src/Forms/FileField.php index d56585ee1..6a6b54b49 100644 --- a/src/Forms/FileField.php +++ b/src/Forms/FileField.php @@ -177,13 +177,46 @@ class FileField extends FormField implements FileHandleField public function validate($validator) { - if (!isset($_FILES[$this->name])) { + // FileField with the name multi_file_syntax[] or multi_file_syntax[key] will have the brackets trimmed in + // $_FILES super-global so it will be stored as $_FILES['mutli_file_syntax'] + // multi-file uploads, which are not officially supported by Silverstripe, though may be + // implemented in custom code, so we should still ensure they are at least validated + $isMultiFileUpload = strpos($this->name, '[') !== false; + $fieldName = preg_replace('#\[(.*?)\]$#', '', $this->name); + + if (!isset($_FILES[$fieldName])) { return true; } - $tmpFile = $_FILES[$this->name]; + if ($isMultiFileUpload) { + $isValid = true; + foreach (array_keys($_FILES[$fieldName]['name']) as $key) { + $fileData = [ + 'name' => $_FILES[$fieldName]['name'][$key], + 'type' => $_FILES[$fieldName]['type'][$key], + 'tmp_name' => $_FILES[$fieldName]['tmp_name'][$key], + 'error' => $_FILES[$fieldName]['error'][$key], + 'size' => $_FILES[$fieldName]['size'][$key], + ]; + if (!$this->validateFileData($validator, $fileData)) { + $isValid = false; + } + } + return $isValid; + } - $valid = $this->upload->validate($tmpFile); + // regular single-file upload + return $this->validateFileData($validator, $_FILES[$this->name]); + } + + /** + * @param Validator $validator + * @param array $fileData + * @return bool + */ + private function validateFileData($validator, array $fileData): bool + { + $valid = $this->upload->validate($fileData); if (!$valid) { $errors = $this->upload->getErrors(); if ($errors) { @@ -193,7 +226,6 @@ class FileField extends FormField implements FileHandleField } return false; } - return true; } diff --git a/tests/php/Forms/FileFieldTest.php b/tests/php/Forms/FileFieldTest.php index 577d279e8..19779d986 100644 --- a/tests/php/Forms/FileFieldTest.php +++ b/tests/php/Forms/FileFieldTest.php @@ -3,6 +3,7 @@ namespace SilverStripe\Forms\Tests; use ReflectionMethod; +use SilverStripe\Assets\Upload_Validator; use SilverStripe\Dev\FunctionalTest; use SilverStripe\Control\Controller; use SilverStripe\Forms\FileField; @@ -39,6 +40,67 @@ class FileFieldTest extends FunctionalTest $this->assertTrue($form->validationResult()->isValid()); } + /** + * Test that FileField::validate() is run on FileFields with both single and multi-file syntax + * By default FileField::validate() will return true early if the $_FILES super-global does not contain the + * corresponding FileField::name. This early return means the files was not fully run through FileField::validate() + * So for this test we create an invalid file upload on purpose and test that false was returned which means that + * the file was run through FileField::validate() function + */ + public function testMultiFileSyntaxUploadIsValidated() + { + $names = [ + 'single_file_syntax', + 'multi_file_syntax_a[]', + 'multi_file_syntax_b[0]', + 'multi_file_syntax_c[key]' + ]; + foreach ($names as $name) { + $form = new Form( + Controller::curr(), + 'Form', + new FieldList($fileField = new FileField($name, 'My desc')), + new FieldList() + ); + $fileData = $this->createInvalidUploadedFileData($name, "FileFieldTest.txt"); + // FileFields with multi_file_syntax[] files will appear in the $_FILES super-global + // with the [] brackets trimmed e.g. $_FILES[multi_file_syntax] + $_FILES = [preg_replace('#\[(.*?)\]#', '', $name) => $fileData]; + $fileField->setValue($fileData); + $validator = $form->getValidator(); + $isValid = $fileField->validate($validator); + $this->assertFalse($isValid, "$name was run through the validate() function"); + } + } + + protected function createInvalidUploadedFileData($name, $tmpFileName): array + { + $tmpFilePath = TEMP_PATH . DIRECTORY_SEPARATOR . $tmpFileName; + + // multi_file_syntax + if (strpos($name, '[') !== false) { + $key = 0; + if (preg_match('#\[(.+?)\]#', $name, $m)) { + $key = $m[1]; + } + return [ + 'name' => [$key => $tmpFileName], + 'type' => [$key => 'text/plaintext'], + 'size' => [$key => 0], + 'tmp_name' => [$key => $tmpFilePath], + 'error' => [$key => UPLOAD_ERR_NO_FILE], + ]; + } + // single_file_syntax + return [ + 'name' => $tmpFileName, + 'type' => 'text/plaintext', + 'size' => 0, + 'tmp_name' => $tmpFilePath, + 'error' => UPLOAD_ERR_NO_FILE, + ]; + } + /** * @skipUpgrade */ From 7ed7ad0254195980f8226d1dbb542debe5288d85 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Thu, 17 Jun 2021 12:05:20 +1200 Subject: [PATCH 3/4] FIX Ensure changing a password to blank is validated --- src/Security/Member.php | 6 +++--- src/Security/MemberPassword.php | 2 +- tests/php/Security/MemberTest.php | 12 +++++++++++- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/Security/Member.php b/src/Security/Member.php index aac772bb3..c88ea357d 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -875,7 +875,7 @@ class Member extends DataObject if ($this->Email) { $this->Email = trim($this->Email); } - + // If a member with the same "unique identifier" already exists with a different ID, don't allow merging. // Note: This does not a full replacement for safeguards in the controller layer (e.g. in a registration form), // but rather a last line of defense against data inconsistencies. @@ -1705,8 +1705,8 @@ class Member extends DataObject $valid = parent::validate(); $validator = static::password_validator(); - if (!$this->ID || $this->isChanged('Password')) { - if ($this->Password && $validator) { + if ($validator) { + if ((!$this->ID && $this->Password) || $this->isChanged('Password')) { $userValid = $validator->validate($this->Password, $this); $valid->combineAnd($userValid); } diff --git a/src/Security/MemberPassword.php b/src/Security/MemberPassword.php index de71c6e23..845110446 100644 --- a/src/Security/MemberPassword.php +++ b/src/Security/MemberPassword.php @@ -53,6 +53,6 @@ class MemberPassword extends DataObject public function checkPassword($password) { $encryptor = PasswordEncryptor::create_for_algorithm($this->PasswordEncryption); - return $encryptor->check($this->Password, $password, $this->Salt, $this->Member()); + return $encryptor->check($this->Password ?? '', $password, $this->Salt, $this->Member()); } } diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index 96a30041b..52f9a3c10 100644 --- a/tests/php/Security/MemberTest.php +++ b/tests/php/Security/MemberTest.php @@ -1593,7 +1593,7 @@ class MemberTest extends FunctionalTest $this->assertSame('Johnson', $member->getLastName(), 'getLastName should proxy to Surname'); } - + public function testEmailIsTrimmed() { $member = new Member(); @@ -1601,4 +1601,14 @@ class MemberTest extends FunctionalTest $member->write(); $this->assertNotNull(Member::get()->find('Email', 'trimmed@test.com')); } + + public function testChangePasswordToBlankIsValidated() + { + // override setup() function which setMinLength(0) + PasswordValidator::singleton()->setMinLength(8); + // 'test' member has a password defined in yml + $member = $this->objFromFixture(Member::class, 'test'); + $result = $member->changePassword(''); + $this->assertFalse($result->isValid()); + } } From b625ba99b3a5877e847004c41ae3e774b795f5be Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Fri, 18 Jun 2021 09:50:13 +1200 Subject: [PATCH 4/4] ENH Remove wording for authenticated devices being manageable --- lang/en.yml | 6 ++---- src/Security/MemberAuthenticator/CMSMemberLoginForm.php | 6 +++--- src/Security/MemberAuthenticator/MemberLoginForm.php | 6 +++--- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/lang/en.yml b/lang/en.yml index b5c617fdb..0d6d2e403 100644 --- a/lang/en.yml +++ b/lang/en.yml @@ -7,7 +7,7 @@ en: EDITINFO: 'Edit this file' REMOVE: Remove SilverStripe\Control\ChangePasswordEmail_ss: - CHANGEPASSWORDFOREMAIL: "The password for account with email address {email} has been changed. If you didn't change your password please change your password using the link below" + CHANGEPASSWORDFOREMAIL: 'The password for account with email address {email} has been changed. If you didn\''t change your password please change your password using the link below' CHANGEPASSWORDTEXT1: 'You changed your password for' CHANGEPASSWORDTEXT3: 'Change password' HELLO: Hi @@ -266,8 +266,7 @@ en: ERRORWRONGCRED: 'The provided details don''t seem to be correct. Please try again.' FIRSTNAME: 'First Name' INTERFACELANG: 'Interface Language' - KEEPMESIGNEDIN: 'Keep me signed in' - REMEMBER_ME: 'Keep me signed in for {count} days' + KEEP_ME_SIGNED_IN: 'Keep me signed in for {count} days' KEEP_ME_SIGNED_IN_TOOLTIP: 'You will remain authenticated on this device for {count} days. Only use this feature if you trust the device you are using.' LOGGEDINAS: 'You''re logged in as {name}.' NEWPASSWORD: 'New Password' @@ -277,7 +276,6 @@ en: PLURALS: one: 'A Member' other: '{count} Members' - REMEMBERME: 'Remember me next time? (for {count} days on this device)' SINGULARNAME: Member SUBJECTPASSWORDCHANGED: 'Your password has been changed' SUBJECTPASSWORDRESET: 'Your password reset link' diff --git a/src/Security/MemberAuthenticator/CMSMemberLoginForm.php b/src/Security/MemberAuthenticator/CMSMemberLoginForm.php index 24bdda81c..43da03d53 100644 --- a/src/Security/MemberAuthenticator/CMSMemberLoginForm.php +++ b/src/Security/MemberAuthenticator/CMSMemberLoginForm.php @@ -59,8 +59,8 @@ class CMSMemberLoginForm extends MemberLoginForm CheckboxField::create( "Remember", _t( - 'SilverStripe\\Security\\Member.REMEMBER_ME', - "Remember me for {count} days", + 'SilverStripe\\Security\\Member.KEEP_ME_SIGNED_IN', + 'Keep me signed in for {count} days', [ 'count' => RememberLoginHash::config()->uninherited('token_expiry_days') ] ) ) @@ -68,7 +68,7 @@ class CMSMemberLoginForm extends MemberLoginForm 'title', _t( 'SilverStripe\\Security\\Member.KEEP_ME_SIGNED_IN_TOOLTIP', - 'You will remain authenticated on this device for {count} days. Only use this feature if you trust the device you are using. Authenticated devices can be managed in your profile.', + 'You will remain authenticated on this device for {count} days. Only use this feature if you trust the device you are using.', ['count' => RememberLoginHash::config()->uninherited('token_expiry_days')] ) ) diff --git a/src/Security/MemberAuthenticator/MemberLoginForm.php b/src/Security/MemberAuthenticator/MemberLoginForm.php index 3b299dc64..027d62995 100644 --- a/src/Security/MemberAuthenticator/MemberLoginForm.php +++ b/src/Security/MemberAuthenticator/MemberLoginForm.php @@ -154,8 +154,8 @@ class MemberLoginForm extends BaseLoginForm CheckboxField::create( "Remember", _t( - 'SilverStripe\\Security\\Member.REMEMBER_ME', - "Remember me for {count} days", + 'SilverStripe\\Security\\Member.KEEP_ME_SIGNED_IN', + 'Keep me signed in for {count} days', [ 'count' => RememberLoginHash::config()->uninherited('token_expiry_days') ] ) ) @@ -163,7 +163,7 @@ class MemberLoginForm extends BaseLoginForm 'title', _t( 'SilverStripe\\Security\\Member.KEEP_ME_SIGNED_IN_TOOLTIP', - 'You will remain authenticated on this device for {count} days. Only use this feature if you trust the device you are using. Authenticated devices can be managed in your profile.', + 'You will remain authenticated on this device for {count} days. Only use this feature if you trust the device you are using.', ['count' => RememberLoginHash::config()->uninherited('token_expiry_days')] ) )