diff --git a/docs/en/changelogs/2.4.12.md b/docs/en/changelogs/2.4.12.md new file mode 100644 index 000000000..2a2c56fde --- /dev/null +++ b/docs/en/changelogs/2.4.12.md @@ -0,0 +1,15 @@ +# 2.4.12 + +## Overview + +### Security: Privilege escalation through Group hierarchy setting (SS-2013-003) + +See [announcement](http://www.silverstripe.org/ss-2013-003-privilege-escalation-through-group-hierarchy-setting/) + +### Security: Privilege escalation through Group and Member CSV upload (SS-2013-004) + +See [announcement](http://www.silverstripe.org/ss-2013-004-privilege-escalation-through-group-and-member-csv-upload/) + +### Security: Privilege escalation through APPLY_ROLES assignment (SS-2013-005) + +See [announcement](http://www.silverstripe.org/ss-2013-005-privilege-escalation-through-apply-roles-assignment/) \ No newline at end of file diff --git a/security/Group.php b/security/Group.php index a1c49fe4c..7a1b6329c 100644 --- a/security/Group.php +++ b/security/Group.php @@ -318,6 +318,33 @@ class Group extends DataObject { public function setCode($val){ $this->setField("Code",SiteTree::generateURLSegment($val)); } + + public function validate() { + $result = parent::validate(); + + // Check if the new group hierarchy would add certain "privileged permissions", + // and require an admin to perform this change in case it does. + // This prevents "sub-admin" users with group editing permissions to increase their privileges. + if($this->ParentID && !Permission::check('ADMIN')) { + $ancestorIds = $this->Parent()->collateAncestorIDs(); + $inheritedPermissions = DataObject::get( + 'Permission', + sprintf('"GroupID" IN (%s)', implode(',', array_map('intval', $ancestorIds))) + ); + $inheritedCodes = $inheritedPermissions ? $inheritedPermissions->column('Code') : array(); + if(array_intersect($inheritedCodes, Permission::$privileged_permissions)) { + $result->error(sprintf( + _t( + 'Group.HierarchyPermsError', + 'Can\'t assign parent group "%s" with privileged permissions (requires ADMIN access)' + ), + $this->Parent()->Title + )); + } + } + + return $result; + } function onBeforeWrite() { parent::onBeforeWrite(); diff --git a/security/Permission.php b/security/Permission.php index a85540fd7..839efd04d 100755 --- a/security/Permission.php +++ b/security/Permission.php @@ -90,6 +90,17 @@ class Permission extends DataObject { */ static $hidden_permissions = array(); + /** + * @config These permissions can only be applied by ADMIN users, to prevent + * privilege escalation on group assignments and inheritance. + * @var array + */ + static $privileged_permissions = array( + 'ADMIN', + 'APPLY_ROLES', + 'EDIT_PERMISSIONS' + ); + /** * Check that the current member has the given permission. * diff --git a/tests/security/GroupTest.php b/tests/security/GroupTest.php index 7609e8d6b..fc4b08daa 100644 --- a/tests/security/GroupTest.php +++ b/tests/security/GroupTest.php @@ -5,7 +5,7 @@ */ class GroupTest extends FunctionalTest { static $fixture_file = 'sapphire/tests/security/GroupTest.yml'; - + /** * Test the Group::map() function */ @@ -20,7 +20,7 @@ class GroupTest extends FunctionalTest { $group1 = $this->objFromFixture('Group', 'group1'); $group2 = $this->objFromFixture('Group', 'group2'); - + /* We have added 2 groups to our fixture. They should both appear in $mapOutput. */ $this->assertEquals($mapOutput[$group1->ID], $group1->Title); $this->assertEquals($mapOutput[$group2->ID], $group2->Title); @@ -29,21 +29,21 @@ class GroupTest extends FunctionalTest { function testMemberGroupRelationForm() { Session::set('loggedInAs', $this->idFromFixture('GroupTest_Member', 'admin')); - $adminGroup = $this->objFromFixture('Group', 'admingroup'); - $parentGroup = $this->objFromFixture('Group', 'parentgroup'); - $childGroup = $this->objFromFixture('Group', 'childgroup'); + $adminGroup = $this->objFromFixture('Group', 'admingroup'); + $parentGroup = $this->objFromFixture('Group', 'parentgroup'); + $childGroup = $this->objFromFixture('Group', 'childgroup'); - // Test single group relation through checkboxsetfield - $form = new GroupTest_MemberForm($this, 'Form'); - $member = $this->objFromFixture('GroupTest_Member', 'admin'); - $form->loadDataFrom($member); - $checkboxSetField = $form->Fields()->fieldByName('Groups'); - $checkboxSetField->setValue(array( - $adminGroup->ID => $adminGroup->ID, // keep existing relation - $parentGroup->ID => $parentGroup->ID, // add new relation - )); - $form->saveInto($member); - $updatedGroups = $member->Groups(); + // Test single group relation through checkboxsetfield + $form = new GroupTest_MemberForm($this, 'Form'); + $member = $this->objFromFixture('GroupTest_Member', 'admin'); + $form->loadDataFrom($member); + $checkboxSetField = $form->Fields()->fieldByName('Groups'); + $checkboxSetField->setValue(array( + $adminGroup->ID => $adminGroup->ID, // keep existing relation + $parentGroup->ID => $parentGroup->ID, // add new relation + )); + $form->saveInto($member); + $updatedGroups = $member->Groups(); $controlGroups = new Member_GroupSet( $adminGroup, @@ -52,31 +52,31 @@ class GroupTest extends FunctionalTest { $this->assertEquals( $updatedGroups->Map('ID','ID'), $controlGroups->Map('ID','ID'), - "Adding a toplevel group works" - ); - - // Test unsetting relationship - $form->loadDataFrom($member); - $checkboxSetField = $form->Fields()->fieldByName('Groups'); - $checkboxSetField->setValue(array( - $adminGroup->ID => $adminGroup->ID, // keep existing relation - //$parentGroup->ID => $parentGroup->ID, // remove previously set relation - )); - $form->saveInto($member); - $member->flushCache(); - $updatedGroups = $member->Groups(); + "Adding a toplevel group works" + ); + + // Test unsetting relationship + $form->loadDataFrom($member); + $checkboxSetField = $form->Fields()->fieldByName('Groups'); + $checkboxSetField->setValue(array( + $adminGroup->ID => $adminGroup->ID, // keep existing relation + //$parentGroup->ID => $parentGroup->ID, // remove previously set relation + )); + $form->saveInto($member); + $member->flushCache(); + $updatedGroups = $member->Groups(); $controlGroups = new Member_GroupSet( $adminGroup ); $this->assertEquals( $updatedGroups->Map('ID','ID'), $controlGroups->Map('ID','ID'), - "Removing a previously added toplevel group works" - ); + "Removing a previously added toplevel group works" + ); - // Test adding child group + // Test adding child group - } + } function testDelete() { $adminGroup = $this->objFromFixture('Group', 'admingroup'); @@ -86,41 +86,89 @@ class GroupTest extends FunctionalTest { $this->assertNull(DataObject::get('Group', "\"ID\"={$adminGroup->ID}"), 'Group is removed'); $this->assertNull(DataObject::get('Permission',"\"GroupID\"={$adminGroup->ID}"), 'Permissions removed along with the group'); } + + public function testValidatesPrivilegeLevelOfParent() { + if(!class_exists('ReflectionMethod')) { + $this->markTestSkipped('Test requires PHP 5.3 Reflection API'); + } + + $nonAdminUser = $this->objFromFixture('GroupTest_Member', 'childgroupuser'); + $adminUser = $this->objFromFixture('GroupTest_Member', 'admin'); + $nonAdminGroup = $this->objFromFixture('Group', 'childgroup'); + $adminGroup = $this->objFromFixture('Group', 'admingroup'); + + $nonAdminValidateMethod = new ReflectionMethod($nonAdminGroup, 'validate'); + $nonAdminValidateMethod->setAccessible(true); + + // Making admin group parent of a non-admin group, effectively expanding is privileges + $nonAdminGroup->ParentID = $adminGroup->ID; + + $this->logInWithPermission('APPLY_ROLES'); + $result = $nonAdminValidateMethod->invoke($nonAdminGroup); + $this->assertFalse( + $result->valid(), + 'Members with only APPLY_ROLES can\'t assign parent groups with direct ADMIN permissions' + ); + + $this->logInWithPermission('ADMIN'); + $result = $nonAdminValidateMethod->invoke($nonAdminGroup); + $this->assertTrue( + $result->valid(), + 'Members with ADMIN can assign parent groups with direct ADMIN permissions' + ); + $nonAdminGroup->write(); + $newlyAdminGroup = $nonAdminGroup; + + $this->logInWithPermission('ADMIN'); + $inheritedAdminGroup = $this->objFromFixture('Group', 'group1'); + $inheritedAdminMethod = new ReflectionMethod($inheritedAdminGroup, 'validate'); + $inheritedAdminMethod->setAccessible(true); + $inheritedAdminGroup->ParentID = $adminGroup->ID; + $inheritedAdminGroup->write(); // only works with ADMIN login + + $this->logInWithPermission('APPLY_ROLES'); + $result = $inheritedAdminMethod->invoke($nonAdminGroup); + $this->assertFalse( + $result->valid(), + 'Members with only APPLY_ROLES can\'t assign parent groups with inherited ADMIN permission' + ); + } + } class GroupTest_Member extends Member implements TestOnly { - + function getCMSFields() { - $groups = DataObject::get('Group'); + $groups = DataObject::get('Group'); $groupsMap = ($groups) ? $groups->toDropDownMap() : false; $fields = new FieldSet( - new HiddenField('ID', 'ID'), - new CheckboxSetField( - 'Groups', - 'Groups', - $groupsMap - ) - ); - - return $fields; - } - + new HiddenField('ID', 'ID'), + new CheckboxSetField( + 'Groups', + 'Groups', + $groupsMap + ) + ); + + return $fields; + } + } class GroupTest_MemberForm extends Form { - + function __construct($controller, $name) { - $fields = singleton('GroupTest_Member')->getCMSFields(); + $fields = singleton('GroupTest_Member')->getCMSFields(); $actions = new FieldSet( - new FormAction('doSave','save') - ); - - parent::__construct($controller, $name, $fields, $actions); - } - + new FormAction('doSave','save') + ); + + parent::__construct($controller, $name, $fields, $actions); + } + function doSave($data, $form) { - // done in testing methods - } - + // done in testing methods + } + } ?> \ No newline at end of file