From 7367cf54c4069a8e296fafb511fb28e27a8efd7e Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 7 Sep 2015 13:44:16 +1200 Subject: [PATCH] [ss-2015-020]: Prevent possible Privilege escalation --- security/Member.php | 65 +++++++++++++++--- security/Security.php | 6 +- tests/model/DataListTest.php | 12 ++-- tests/model/DataObjectTest.php | 1 + tests/security/MemberTest.php | 73 ++++++++++++++++++++- tests/security/SecurityDefaultAdminTest.php | 8 ++- 6 files changed, 148 insertions(+), 17 deletions(-) diff --git a/security/Member.php b/security/Member.php index 1fd308cb7..fa19ff2de 100644 --- a/security/Member.php +++ b/security/Member.php @@ -254,7 +254,11 @@ class Member extends DataObject implements TemplateGlobalProvider { // Ensure this user is in the admin group if(!$admin->inGroup($adminGroup)) { - $admin->Groups()->add($adminGroup); + // Add member to group instead of adding group to member + // This bypasses the privilege escallation code in Member_GroupSet + $adminGroup + ->DirectMembers() + ->add($admin); } return $admin; @@ -890,27 +894,30 @@ class Member extends DataObject implements TemplateGlobalProvider { public function onAfterWrite() { parent::onAfterWrite(); + Permission::flush_permission_cache(); + if($this->isChanged('Password')) { MemberPassword::log($this); } } /** + * Filter out admin groups to avoid privilege escalation, * If any admin groups are requested, deny the whole save operation. * * @param Array $ids Database IDs of Group records - * @return boolean + * @return boolean True if the change can be accepted */ public function onChangeGroups($ids) { - // Filter out admin groups to avoid privilege escalation, // unless the current user is an admin already OR the logged in user is an admin - if(!(Permission::check('ADMIN') || Permission::checkMember($this, 'ADMIN'))) { - $adminGroups = Permission::get_groups_by_permission('ADMIN'); - $adminGroupIDs = ($adminGroups) ? $adminGroups->column('ID') : array(); - return count(array_intersect($ids, $adminGroupIDs)) == 0; - } else { + if(Permission::check('ADMIN') || Permission::checkMember($this, 'ADMIN')) { return true; } + + // 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; } @@ -1147,6 +1154,7 @@ class Member extends DataObject implements TemplateGlobalProvider { * Use {@link DirectGroups()} to only retrieve the group relations without inheritance. * * @todo Push all this logic into Member_GroupSet's getIterator()? + * @return Member_Groupset */ public function Groups() { $groups = Member_GroupSet::create('Group', 'Group_Members', 'GroupID', 'MemberID'); @@ -1657,6 +1665,47 @@ class Member_GroupSet extends ManyManyList { public function foreignIDWriteFilter($id = null) { return parent::foreignIDFilter($id); } + + public function add($item, $extraFields = null) { + // Get Group.ID + $itemID = null; + if(is_numeric($item)) { + $itemID = $item; + } else if($item instanceof Group) { + $itemID = $item->ID; + } + + // Check if this group is allowed to be added + if($this->canAddGroups(array($itemID))) { + parent::add($item, $extraFields); + } + } + + /** + * Determine if the following groups IDs can be added + * + * @param array $itemIDs + * @return boolean + */ + protected function canAddGroups($itemIDs) { + if(empty($itemIDs)) { + return true; + } + $member = $this->getMember(); + return empty($member) || $member->onChangeGroups($itemIDs); + } + + /** + * Get foreign member record for this relation + * + * @return Member + */ + protected function getMember() { + $id = $this->getForeignID(); + if($id) { + return DataObject::get_by_id('Member', $id); + } + } } /** diff --git a/security/Security.php b/security/Security.php index 1033fbe99..f18f8e125 100644 --- a/security/Security.php +++ b/security/Security.php @@ -767,7 +767,11 @@ class Security extends Controller { $member = Member::create(); $member->FirstName = _t('Member.DefaultAdminFirstname', 'Default Admin'); $member->write(); - $member->Groups()->add($adminGroup); + // Add member to group instead of adding group to member + // This bypasses the privilege escallation code in Member_GroupSet + $adminGroup + ->DirectMembers() + ->add($member); } return $member; diff --git a/tests/model/DataListTest.php b/tests/model/DataListTest.php index faf9b353b..a30bc2209 100755 --- a/tests/model/DataListTest.php +++ b/tests/model/DataListTest.php @@ -156,7 +156,8 @@ class DataListTest extends SapphireTest { . ' "DataObjectTest_TeamComment"."Comment", "DataObjectTest_TeamComment"."TeamID",' . ' "DataObjectTest_TeamComment"."ID", CASE WHEN "DataObjectTest_TeamComment"."ClassName" IS NOT NULL' . ' THEN "DataObjectTest_TeamComment"."ClassName" ELSE '.$db->prepStringForDB('DataObjectTest_TeamComment') - . ' END AS "RecordClassName" FROM "DataObjectTest_TeamComment"'; + . ' END AS "RecordClassName" FROM "DataObjectTest_TeamComment"' + . ' ORDER BY "DataObjectTest_TeamComment"."Name" ASC'; $this->assertEquals($expected, $list->sql()); } @@ -176,7 +177,8 @@ class DataListTest extends SapphireTest { . ' "DataObjectTest_TeamComment"."ID", CASE WHEN "DataObjectTest_TeamComment"."ClassName" IS NOT NULL' . ' THEN "DataObjectTest_TeamComment"."ClassName" ELSE '.$db->prepStringForDB('DataObjectTest_TeamComment') . ' END AS "RecordClassName" FROM "DataObjectTest_TeamComment" INNER JOIN "DataObjectTest_Team" AS "Team"' - . ' ON "DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"'; + . ' ON "DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"' + . ' ORDER BY "DataObjectTest_TeamComment"."Name" ASC'; $this->assertEquals($expected, $list->sql()); } @@ -197,7 +199,8 @@ class DataListTest extends SapphireTest { . ' "DataObjectTest_TeamComment"."ID", CASE WHEN "DataObjectTest_TeamComment"."ClassName" IS NOT NULL' . ' THEN "DataObjectTest_TeamComment"."ClassName" ELSE '.$db->prepStringForDB('DataObjectTest_TeamComment') . ' END AS "RecordClassName" FROM "DataObjectTest_TeamComment" LEFT JOIN "DataObjectTest_Team" AS "Team"' - . ' ON "DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"'; + . ' ON "DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"' + . ' ORDER BY "DataObjectTest_TeamComment"."Name" ASC'; $this->assertEquals($expected, $list->sql()); @@ -220,7 +223,8 @@ class DataListTest extends SapphireTest { . 'ELSE ' . $db->prepStringForDB('DataObjectTest_TeamComment') . ' END AS "RecordClassName" ' . 'FROM "DataObjectTest_TeamComment" ' . 'LEFT JOIN "DataObjectTest\NamespacedClass" ON ' - . '"DataObjectTest\NamespacedClass"."ID" = "DataObjectTest_TeamComment"."ID"'; + . '"DataObjectTest\NamespacedClass"."ID" = "DataObjectTest_TeamComment"."ID"' + . ' ORDER BY "DataObjectTest_TeamComment"."Name" ASC'; $this->assertEquals($expected, $list->sql(), 'Retains backslashes in namespaced classes'); } diff --git a/tests/model/DataObjectTest.php b/tests/model/DataObjectTest.php index 73fbe33ec..e967e5fad 100644 --- a/tests/model/DataObjectTest.php +++ b/tests/model/DataObjectTest.php @@ -1521,6 +1521,7 @@ class DataObjectTest_TeamComment extends DataObject { 'Team' => 'DataObjectTest_Team' ); + private static $default_sort = '"Name" ASC'; } class DataObjectTest_ExtendedTeamComment extends DataObjectTest_TeamComment { diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php index f058a755f..36554dba9 100644 --- a/tests/security/MemberTest.php +++ b/tests/security/MemberTest.php @@ -579,7 +579,6 @@ class MemberTest extends FunctionalTest { public function testOnChangeGroups() { $staffGroup = $this->objFromFixture('Group', 'staffgroup'); - $adminGroup = $this->objFromFixture('Group', 'admingroup'); $staffMember = $this->objFromFixture('Member', 'staffmember'); $adminMember = $this->objFromFixture('Member', 'admin'); $newAdminGroup = new Group(array('Title' => 'newadmin')); @@ -587,7 +586,7 @@ class MemberTest extends FunctionalTest { Permission::grant($newAdminGroup->ID, 'ADMIN'); $newOtherGroup = new Group(array('Title' => 'othergroup')); $newOtherGroup->write(); - + $this->assertTrue( $staffMember->onChangeGroups(array($staffGroup->ID)), 'Adding existing non-admin group relation is allowed for non-admin members' @@ -614,6 +613,76 @@ class MemberTest extends FunctionalTest { ); } + /** + * Test Member_GroupSet::add + */ + public function testOnChangeGroupsByAdd() { + $staffMember = $this->objFromFixture('Member', 'staffmember'); + $adminMember = $this->objFromFixture('Member', 'admin'); + + // Setup new admin group + $newAdminGroup = new Group(array('Title' => 'newadmin')); + $newAdminGroup->write(); + Permission::grant($newAdminGroup->ID, 'ADMIN'); + + // Setup non-admin group + $newOtherGroup = new Group(array('Title' => 'othergroup')); + $newOtherGroup->write(); + + // Test staff can be added to other group + $this->assertFalse($staffMember->inGroup($newOtherGroup)); + $staffMember->Groups()->add($newOtherGroup); + $this->assertTrue( + $staffMember->inGroup($newOtherGroup), + 'Adding new non-admin group relation is allowed for non-admin members' + ); + + // Test staff member can't be added to admin groups + $this->assertFalse($staffMember->inGroup($newAdminGroup)); + $staffMember->Groups()->add($newAdminGroup); + $this->assertFalse( + $staffMember->inGroup($newAdminGroup), + 'Adding new admin group relation is not allowed for non-admin members' + ); + + // Test staff member can be added to admin group by admins + $this->logInAs($adminMember); + $staffMember->Groups()->add($newAdminGroup); + $this->assertTrue( + $staffMember->inGroup($newAdminGroup), + 'Adding new admin group relation is allowed for normal users, when granter is logged in as admin' + ); + + // Test staff member can be added if they are already admin + $this->session()->inst_set('loggedInAs', null); + $this->assertFalse($adminMember->inGroup($newAdminGroup)); + $adminMember->Groups()->add($newAdminGroup); + $this->assertTrue( + $adminMember->inGroup($newAdminGroup), + 'Adding new admin group relation is allowed for admin members' + ); + } + + /** + * Test Member_GroupSet::add + */ + public function testOnChangeGroupsBySetIDList() { + $staffMember = $this->objFromFixture('Member', 'staffmember'); + + // Setup new admin group + $newAdminGroup = new Group(array('Title' => 'newadmin')); + $newAdminGroup->write(); + Permission::grant($newAdminGroup->ID, 'ADMIN'); + + // Test staff member can't be added to admin groups + $this->assertFalse($staffMember->inGroup($newAdminGroup)); + $staffMember->Groups()->setByIDList(array($newAdminGroup->ID)); + $this->assertFalse( + $staffMember->inGroup($newAdminGroup), + 'Adding new admin group relation is not allowed for non-admin members' + ); + } + /** * Test that extensions using updateCMSFields() are applied correctly */ diff --git a/tests/security/SecurityDefaultAdminTest.php b/tests/security/SecurityDefaultAdminTest.php index 738722233..4f1c1db06 100644 --- a/tests/security/SecurityDefaultAdminTest.php +++ b/tests/security/SecurityDefaultAdminTest.php @@ -1,6 +1,8 @@ defaultPassword = Security::default_admin_password(); Security::clear_default_admin(); Security::setDefaultAdmin('admin', 'password'); + Permission::flush_permission_cache(); } public function tearDown() { Security::setDefaultAdmin($this->defaultUsername, $this->defaultPassword); + Permission::flush_permission_cache(); parent::tearDown(); } @@ -42,9 +46,9 @@ class SecurityDefaultAdminTest extends SapphireTest { public function testFindAnAdministratorCreatesNewUser() { $adminMembers = Permission::get_members_by_permission('ADMIN'); $this->assertEquals(0, $adminMembers->count()); - + $admin = Security::findAnAdministrator(); - + $this->assertInstanceOf('Member', $admin); $this->assertTrue(Permission::checkMember($admin, 'ADMIN')); $this->assertEquals($admin->Email, Security::default_admin_username());