Permalink
Browse files

FIX Privilege escalation through Group hierarchy setting (SS-2013-003)

  • Loading branch information...
1 parent 1c31c09 commit 68ca47b0ddbeb01e32f18ff69a403068e62f5caf @chillu chillu committed Aug 30, 2013
Showing with 83 additions and 1 deletion.
  1. +4 −1 docs/en/changelogs/3.0.6.md
  2. +25 −0 security/Group.php
  3. +11 −0 security/Permission.php
  4. +43 −0 tests/security/GroupTest.php
@@ -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
View
@@ -334,6 +334,31 @@ public function getTreeTitle() {
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();
View
@@ -87,6 +87,17 @@ class Permission extends DataObject implements TemplateGlobalProvider {
private 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
+ */
+ private static $privileged_permissions = array(
+ 'ADMIN',
+ 'APPLY_ROLES',
+ 'EDIT_PERMISSIONS'
+ );
+
+ /**
* Check that the current member has the given permission.
*
* @param string|array $code Code of the permission to check (case-sensitive)
@@ -109,6 +109,49 @@ public function testDelete() {
'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 {

0 comments on commit 68ca47b

Please sign in to comment.