diff --git a/admin/code/LeftAndMain.php b/admin/code/LeftAndMain.php index 574c6b67e..c7fcae9af 100644 --- a/admin/code/LeftAndMain.php +++ b/admin/code/LeftAndMain.php @@ -193,9 +193,11 @@ class LeftAndMain extends Controller implements PermissionProvider { if(!$member) return false; // alternative extended checks - if($this->hasMethod('alternateAccessCheck')) { - $alternateAllowed = $this->alternateAccessCheck(); - if($alternateAllowed === FALSE) return false; + if ($this->hasMethod('alternateAccessCheck')) { + $alternateAllowed = $this->alternateAccessCheck($member); + if ($alternateAllowed === false) { + return false; + } } // Check for "CMS admin" permission diff --git a/docs/en/04_Changelogs/3.7.4.md b/docs/en/04_Changelogs/3.7.4.md index 8278bf9ec..11d60454b 100644 --- a/docs/en/04_Changelogs/3.7.4.md +++ b/docs/en/04_Changelogs/3.7.4.md @@ -17,3 +17,25 @@ composer require --dev sminnee/phpunit-mock-objects:^3.4.7 This fork is not a supported module and SilverStripe does not commit to maintaining it. + +## Change Log + +### Security + + * 2019-09-16 [a86093fee](https://github.com/silverstripe/silverstripe-framework/commit/a86093fee6398881889d6d330a15f7042be25bff) Session fixation in "change password" form (Serge Latyntcev) - See [cve-2019-12203](https://www.silverstripe.org/download/security-releases/cve-2019-12203) + * 2019-01-10 [c44f06cdf](https://github.com/silverstripe/silverstripe-framework/commit/c44f06cdf10387a987e4efb096ff06b3bb4495ef) Patch SQL Injection vulnerability when arrays are assigned to DataObject Fields (Aaron Carlino) - See [ss-2018-021](https://www.silverstripe.org/download/security-releases/ss-2018-021) + +### Features and Enhancements + + * 2019-03-13 [0bf03a3e7](https://github.com/silverstripe/silverstripe-framework/commit/0bf03a3e77304e242a81cca37ccbc02e35e3dbc6) Add PHP 7.4’s daily snapshot to the travis suite. (Sam Minnee) + +### Bugfixes + + * 2019-08-26 [7d901a6d9](https://github.com/silverstripe/silverstripe-framework/commit/7d901a6d9b126ac057918f827e65aa0360231d1b) Member argument is now passed to LeftAndMain::alternateAccessCheck() (Robbie Averill) + * 2019-07-12 [b250e14ac](https://github.com/silverstripe/silverstripe-framework/commit/b250e14acea12493ee1bb7392cf3f18c5a1f5f8b) Require PHP7.4 compatible fork of phpunit-mock-objects (Maxime Rainville) + * 2019-04-15 [ad3c58f2d](https://github.com/silverstripe/silverstripe-framework/commit/ad3c58f2d875a0ac7a8a17372c7eca84abd1a2b3) Back-port https://github.com/silverstripe/silverstripe-admin/pull/769 to 3.7, fix parsererror issue (Damian Mooyman) + * 2019-02-26 [bd9296941](https://github.com/silverstripe/silverstripe-framework/commit/bd929694188dc7df7277d8430df5534dcb2b914a) Use a function common to MySQL, SQLite and PostgreSQL to test dynamic DBFIeld assigment (Maxime Rainville) + * 2019-02-25 [adbc560bd](https://github.com/silverstripe/silverstripe-framework/commit/adbc560bd70ba2e071f94a41a084768819196ee7) Address PR feedback. (Maxime Rainville) + * 2019-02-21 [4ec1a682c](https://github.com/silverstripe/silverstripe-framework/commit/4ec1a682cf354e2425ef4fd6598c7de8e807bcc7) Renable the ability to do dynamic assignment with DBField (Maxime Rainville) + * 2019-02-19 [ab5f09a9f](https://github.com/silverstripe/silverstripe-framework/commit/ab5f09a9f3ec12333c748dd68bfc504b5e509bfc) Updated unit test were targeting Float/Int which don't exist on PHP7 (#8810) (Maxime Rainville) + \ No newline at end of file diff --git a/model/DataObject.php b/model/DataObject.php index d778a6994..7cc3e4c73 100644 --- a/model/DataObject.php +++ b/model/DataObject.php @@ -1362,12 +1362,16 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity if (!isset($tableManipulation['fields'])) { continue; } - foreach ($tableManipulation['fields'] as $fieldValue) { + foreach ($tableManipulation['fields'] as $fieldName => $fieldValue) { if (is_array($fieldValue)) { - user_error( - 'DataObject::writeManipulation: parameterised field assignments are disallowed', - E_USER_ERROR - ); + $dbObject = $this->dbObject($fieldName); + // If the field allows non-scalar values we'll let it do dynamic assignments + if ($dbObject && $dbObject->scalarValueOnly()) { + user_error( + 'DataObject::writeManipulation: parameterised field assignments are disallowed', + E_USER_ERROR + ); + } } } } diff --git a/model/ManyManyList.php b/model/ManyManyList.php index a64d00c34..bbf024c47 100644 --- a/model/ManyManyList.php +++ b/model/ManyManyList.php @@ -254,8 +254,10 @@ class ManyManyList extends RelationList { ); } + /** @var DBField[] $fieldObjects */ + $fieldObjects = array(); if($extraFields && $this->extraFields) { - // Write extra field to manipluation in the same way + // Write extra field to manipulation in the same way // that DataObject::prepareManipulationTable writes fields foreach($this->extraFields as $fieldName => $fieldSpec) { // Skip fields without an assignment @@ -263,6 +265,7 @@ class ManyManyList extends RelationList { $fieldObject = SS_Object::create_from_string($fieldSpec, $fieldName); $fieldObject->setValue($extraFields[$fieldName]); $fieldObject->writeToManipulation($manipulation[$this->joinTable]); + $fieldObjects[$fieldName] = $fieldObject; } } } @@ -275,12 +278,15 @@ class ManyManyList extends RelationList { if (!isset($tableManipulation['fields'])) { continue; } - foreach ($tableManipulation['fields'] as $fieldValue) { + foreach ($tableManipulation['fields'] as $fieldName => $fieldValue) { if (is_array($fieldValue)) { - user_error( - 'ManyManyList::add: parameterised field assignments are disallowed', - E_USER_ERROR - ); + // If the field allows non-scalar values we'll let it do dynamic assignments + if (isset($fieldObjects[$fieldName]) && $fieldObjects[$fieldName]->scalarValueOnly()) { + user_error( + 'ManyManyList::add: parameterised field assignments are disallowed', + E_USER_ERROR + ); + } } } } diff --git a/security/Security.php b/security/Security.php index 225092569..5ba2064bf 100644 --- a/security/Security.php +++ b/security/Security.php @@ -726,6 +726,12 @@ class Security extends Controller implements TemplateGlobalProvider { $curMember->logOut(); } + if (!headers_sent()) { + // To avoid a potential session fixation attack + // we're refreshing the session id so that it's + // always new and random for every authentication + session_regenerate_id(true); + } // Store the hash for the change password form. Will be unset after reload within the ChangePasswordForm. Session::set('AutoLoginHash', $member->encryptWithUserSettings($_REQUEST['t'])); diff --git a/tests/model/DataObjectTest.php b/tests/model/DataObjectTest.php index 83e9c6bd5..501044ee1 100644 --- a/tests/model/DataObjectTest.php +++ b/tests/model/DataObjectTest.php @@ -32,6 +32,7 @@ class DataObjectTest extends SapphireTest { 'DataObjectTest_Sortable', 'ManyManyListTest_Product', 'ManyManyListTest_Category', + 'MockDynamicAssignmentDataObject' ); /** @@ -1773,6 +1774,35 @@ class DataObjectTest extends SapphireTest { $this->assertNotEmpty($do->CompositeMoneyField); } + + public function testWriteManipulationWithNonScalarValuesAllowed() + { + $do = MockDynamicAssignmentDataObject::create(); + $do->write(); + $do->StaticScalarOnlyField = true; + $do->DynamicScalarOnlyField = false; + $do->DynamicField = true; + $do->write(); + $this->assertEquals(1, $do->StaticScalarOnlyField); + $this->assertEquals(0, $do->DynamicScalarOnlyField); + $this->assertEquals(1, $do->DynamicField); + } + + /** + * @expectedException PHPUnit_Framework_Error + * @expectedExceptionMessageRegExp /parameterised field assignments are disallowed/ + */ + public function testWriteManipulationWithNonScalarValuesDisallowed() + { + + $do = MockDynamicAssignmentDataObject::create(); + $do->write(); + $do->StaticScalarOnlyField = false; + $do->DynamicScalarOnlyField = true; + $do->DynamicField = false; + + $do->write(); + } } class DataObjectTest_Sortable extends DataObject implements TestOnly { diff --git a/tests/model/ManyManyListTest.php b/tests/model/ManyManyListTest.php index 7990a8b51..a75c92cc2 100644 --- a/tests/model/ManyManyListTest.php +++ b/tests/model/ManyManyListTest.php @@ -21,6 +21,7 @@ class ManyManyListTest extends SapphireTest { 'ManyManyListTest_ExtraFields', 'ManyManyListTest_Product', 'ManyManyListTest_Category', + 'MockDynamicAssignmentDataObject', ); @@ -306,7 +307,40 @@ class ManyManyListTest extends SapphireTest { $this->assertEquals(1, $productsRelatedToProductB->count()); } + public function testWriteManipulationWithNonScalarValuesAllowed() + { + $left = MockDynamicAssignmentDataObject::create(); + $left->write(); + $right = MockDynamicAssignmentDataObject::create(); + $right->write(); + $left->MockManyMany()->add($right, array( + 'ManyManyStaticScalarOnlyField' => true, + 'ManyManyDynamicScalarOnlyField' => false, + 'ManyManyDynamicField' => true + )); + $pivot = $left->MockManyMany()->first(); + $this->assertEquals(1, $pivot->ManyManyStaticScalarOnlyField); + $this->assertEquals(0, $pivot->ManyManyDynamicScalarOnlyField); + $this->assertEquals(1, $pivot->ManyManyDynamicField); + } + /** + * @expectedException PHPUnit_Framework_Error + * @expectedExceptionMessageRegExp /parameterised field assignments are disallowed/ + */ + public function testWriteManipulationWithNonScalarValuesDisallowed() + { + $left = MockDynamicAssignmentDataObject::create(); + $left->write(); + $right = MockDynamicAssignmentDataObject::create(); + $right->write(); + + $left->MockManyMany()->add($right, array( + 'ManyManyStaticScalarOnlyField' => false, + 'ManyManyDynamicScalarOnlyField' => true, + 'ManyManyDynamicField' => false + )); + } } /** diff --git a/tests/model/Mock/MockDynamicAssignmentDBField.php b/tests/model/Mock/MockDynamicAssignmentDBField.php new file mode 100644 index 000000000..4a09c4a29 --- /dev/null +++ b/tests/model/Mock/MockDynamicAssignmentDBField.php @@ -0,0 +1,49 @@ +scalarOnly = $scalarOnly; + $this->dynamicAssignment = $dynamicAssignment; + parent::__construct($name); + } + + /** + * If the field value and dynamicAssignment are true, we'll try to do a dynamic assignment + * @param mixed $value + * @return array|int|mixed + */ + public function prepValueForDB($value) + { + if ($value) { + return $this->dynamicAssignment + ? array('ABS(?)' => array(1)) + : 1; + } + + return 0; + } + + public function scalarValueOnly() + { + return $this->scalarOnly; + } +} diff --git a/tests/model/Mock/MockDynamicAssignmentDataObject.php b/tests/model/Mock/MockDynamicAssignmentDataObject.php new file mode 100644 index 000000000..5363de1fa --- /dev/null +++ b/tests/model/Mock/MockDynamicAssignmentDataObject.php @@ -0,0 +1,44 @@ + 'MockDynamicAssignmentDBField(1,0)', + + // This field tries to emit dynamic assignment but will fail because of scalar only + 'DynamicScalarOnlyField' => 'MockDynamicAssignmentDBField(1,1)', + + // This field does dynamic assignment and will pass + 'DynamicField' => 'MockDynamicAssignmentDBField(0,1)', + ); + + private static $many_many = array( + "MockManyMany" => 'MockDynamicAssignmentDataObject' + ); + + private static $belongs_many_many = array( + "MockBelongsManyMany" => 'MockDynamicAssignmentDataObject' + ); + + private static $many_many_extraFields = array( + 'MockManyMany' => array( + // This field only emits scalar value and will save + 'ManyManyStaticScalarOnlyField' => 'MockDynamicAssignmentDBField(1,0)', + + // This field tries to emit dynamic assignment but will fail because of scalar only + 'ManyManyDynamicScalarOnlyField' => 'MockDynamicAssignmentDBField(1,1)', + + // This field does dynamic assignment and will pass + 'ManyManyDynamicField' => 'MockDynamicAssignmentDBField(0,1)', + ) + ); +}