From 720c149aee196c0eb3b8369275cc6460b2899198 Mon Sep 17 00:00:00 2001
From: Ingo Schommer <ingo@silverstripe.com>
Date: Fri, 30 Aug 2013 13:58:37 +0200
Subject: [PATCH] FIX Privilege escalation through Group hierarchy setting
 (SS-2013-003)

See http://www.silverstripe.org/ss-2013-003-privilege-escalation-through-group-hierarchy-setting/
---
 docs/en/changelogs/3.0.6.md  |  5 ++++-
 security/Group.php           | 25 +++++++++++++++++++++
 security/Permission.php      | 11 +++++++++
 tests/security/GroupTest.php | 43 ++++++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/docs/en/changelogs/3.0.6.md b/docs/en/changelogs/3.0.6.md
index a35725ddd..540d6e734 100644
--- a/docs/en/changelogs/3.0.6.md
+++ b/docs/en/changelogs/3.0.6.md
@@ -7,7 +7,7 @@
 
 ## Details
 
-### Security: Require ADMIN for ?flush=1
+### Security: Require ADMIN for ?flush=1 (SS-2013-001)
 
 Flushing the various manifests (class, template, config) is performed through a GET
 parameter (`flush=1`). Since this action requires more server resources than normal requests,
@@ -23,6 +23,9 @@ This applies to both `flush=1` and `flush=all` (technically we only check for th
 but only through web requests made through main.php - CLI requests, or any other request that goes through
 a custom start up script will still process all flush requests as normal.
 
+### 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/)
 ## Upgrading
 
  * If you have created your own composite database fields, then you should amend the setValue() to allow the passing of
diff --git a/security/Group.php b/security/Group.php
index a66245361..c00f9503e 100755
--- a/security/Group.php
+++ b/security/Group.php
@@ -344,6 +344,31 @@ class Group extends DataObject {
 	public function setCode($val){
 		$this->setField("Code", Convert::raw2url($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->Parent()->exists() && !Permission::check('ADMIN')) {
+			$inheritedCodes = Permission::get()
+				->filter('GroupID', $this->Parent()->collateAncestorIDs())
+				->column('Code');
+			$privilegedCodes = Config::inst()->get('Permission', 'privileged_permissions');
+			if(array_intersect($inheritedCodes, $privilegedCodes)) {
+				$result->error(sprintf(
+					_t(
+						'Group.HierarchyPermsError',
+						'Can\'t assign parent group "%s" with privileged permissions (requires ADMIN access)'
+					),
+					$this->Parent()->Title
+				));
+			}
+		}
+
+		return $result;
+	}
 	
 	public function onBeforeWrite() {
 		parent::onBeforeWrite();
diff --git a/security/Permission.php b/security/Permission.php
index 771fef203..0fd5f270e 100644
--- a/security/Permission.php
+++ b/security/Permission.php
@@ -90,6 +90,17 @@ class Permission extends DataObject implements TemplateGlobalProvider {
 	 */
 	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 3404fde06..633ccbc5e 100644
--- a/tests/security/GroupTest.php
+++ b/tests/security/GroupTest.php
@@ -133,6 +133,49 @@ class GroupTest extends FunctionalTest {
 			'Grandchild groups are removed');
 	}
 
+	public function testValidatesPrivilegeLevelOfParent() {
+		$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 {