From 498e5758bf43308e6c97f3dfeb234d888cc5b6d5 Mon Sep 17 00:00:00 2001
From: Ingo Schommer <ingo@silverstripe.com>
Date: Wed, 9 Mar 2011 15:49:41 +1300
Subject: [PATCH] BUGFIX Avoid privilege escalation from EDIT_PERMISSIONS to
 ADMIN through TreeMultiselectField (in Member->getCMSFields()) by checking
 for admin groups in Member->onChangeGroups()

---
 security/Member.php           | 18 ++++++++++++++++++
 tests/security/MemberTest.php | 29 +++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/security/Member.php b/security/Member.php
index 7bc7ce8d6..175e485e9 100755
--- a/security/Member.php
+++ b/security/Member.php
@@ -695,6 +695,24 @@ class Member extends DataObject {
 			MemberPassword::log($this);
 		}
 	}
+	
+	/**
+	 * If any admin groups are requested, deny the whole save operation.
+	 * 
+	 * @param Array $ids Database IDs of Group records
+	 * @return boolean
+	 */
+	function onChangeGroups($ids) {
+		// Filter out admin groups to avoid privilege escalation, 
+		// unless the current user is an admin already
+		if(!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 {
+			return true;
+		}
+	}
 
 
 	/**
diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php
index c4de0d64f..1b2632b93 100644
--- a/tests/security/MemberTest.php
+++ b/tests/security/MemberTest.php
@@ -527,6 +527,35 @@ class MemberTest extends FunctionalTest {
 		$this->assertFalse($adminMember->canEdit($securityAdminMember), 'Security-Admins can not edit other admins');
 		$this->assertTrue($ceoMember->canEdit($securityAdminMember), 'Security-Admins can edit other members');
 	}
+	
+	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'
+		);
+		$this->assertTrue(
+			$staffMember->onChangeGroups(array($newOtherGroup->ID)),
+			'Adding new non-admin group relation is allowed for non-admin members'
+		);
+		$this->assertFalse(
+			$staffMember->onChangeGroups(array($newAdminGroup->ID)),
+			'Adding new admin group relation is not allowed for non-admin members'
+		);
+		$this->assertTrue(
+			$adminMember->onChangeGroups(array($newAdminGroup->ID)),
+			'Adding new admin group relation is allowed for admin members'
+		);
+	}
 
 	/**
 	 * Add the given array of member extensions as class names.