Skip to content

Commit

Permalink
[ss-2015-020]: Prevent possible Privilege escalation
Browse files Browse the repository at this point in the history
  • Loading branch information
Damian Mooyman committed Sep 10, 2015
1 parent 45b22c7 commit 7367cf5
Show file tree
Hide file tree
Showing 6 changed files with 148 additions and 17 deletions.
65 changes: 57 additions & 8 deletions security/Member.php
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,11 @@ public static function default_admin() {

// 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;
Expand Down Expand Up @@ -890,27 +894,30 @@ public function onBeforeWrite() {
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;
}


Expand Down Expand Up @@ -1147,6 +1154,7 @@ public function getTimeFormat() {
* 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');
Expand Down Expand Up @@ -1657,6 +1665,47 @@ public function foreignIDFilter($id = null) {
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);
}
}
}

/**
Expand Down
6 changes: 5 additions & 1 deletion security/Security.php
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,11 @@ public static function findAnAdministrator() {
$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;
Expand Down
12 changes: 8 additions & 4 deletions tests/model/DataListTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,8 @@ public function testSql() {
. ' "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());
}

Expand All @@ -176,7 +177,8 @@ public function testInnerJoin() {
. ' "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());
}
Expand All @@ -197,7 +199,8 @@ public function testLeftJoin() {
. ' "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());

Expand All @@ -220,7 +223,8 @@ public function testLeftJoin() {
. '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');

}
Expand Down
1 change: 1 addition & 0 deletions tests/model/DataObjectTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1521,6 +1521,7 @@ class DataObjectTest_TeamComment extends DataObject {
'Team' => 'DataObjectTest_Team'
);

private static $default_sort = '"Name" ASC';
}

class DataObjectTest_ExtendedTeamComment extends DataObjectTest_TeamComment {
Expand Down
73 changes: 71 additions & 2 deletions tests/security/MemberTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -579,15 +579,14 @@ public function testMembersWithSecurityAdminAccessCantEditAdminsUnlessTheyreAdmi

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'));
$newAdminGroup->write();
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'
Expand All @@ -614,6 +613,76 @@ public function testOnChangeGroups() {
);
}

/**
* 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
*/
Expand Down
8 changes: 6 additions & 2 deletions tests/security/SecurityDefaultAdminTest.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<?php
class SecurityDefaultAdminTest extends SapphireTest {

protected $usesDatabase = true;

protected $defaultUsername = null;
protected $defaultPassword = null;

Expand All @@ -16,10 +18,12 @@ public function setUp() {
$this->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();
}

Expand All @@ -42,9 +46,9 @@ public function testCheckDefaultAdmin() {
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());
Expand Down

0 comments on commit 7367cf5

Please sign in to comment.