Merge pull request #3 from silverstripe-security/fixes/ss-2015-020

[ss-2015-020]: Prevent possible Privilege escalation

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);
+		}
+	}
 }
 
 /**

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;

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');
 
 	}

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 {

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
 	 */

tests/security/SecurityDefaultAdminTest.php

@@ -1,6 +1,8 @@
 <?php
 class SecurityDefaultAdminTest extends SapphireTest {
 
+	protected $usesDatabase = true;
+
 	protected $defaultUsername = null;
 	protected $defaultPassword = null;
 	
@@ -16,10 +18,12 @@ class SecurityDefaultAdminTest extends SapphireTest {
 		$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();
 	}
 	
@@ -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());