Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

BUGFIX Avoid privilege escalation from EDIT_PERMISSIONS to ADMIN thro…

…ugh TreeMultiselectField (in Member->getCMSFields()) by checking for admin groups in Member->onChangeGroups()
  • Loading branch information...
commit 5bc0d008e9153f1db563149df9c1607e4a6e8a7a 1 parent 6d6c294
@chillu chillu authored
View
18 security/Member.php
@@ -542,6 +542,24 @@ function onAfterWrite() {
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;
+ }
+ }
/**
View
29 tests/security/MemberTest.php
@@ -260,4 +260,33 @@ function testInGroup() {
'Non-existant group returns false'
);
}
+
+ 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'
+ );
+ }
}
View
36 tests/security/MemberTest.yml
@@ -1,4 +1,17 @@
+Permission:
+ admin:
+ Code: ADMIN
+ security-admin:
+ Code: CMS_ACCESS_SecurityAdmin
Group:
+ admingroup:
+ Title: Admin
+ Code: admin
+ Permissions: =>Permission.admin
+ securityadminsgroup:
+ Title: securityadminsgroup
+ Code: securityadminsgroup
+ Permissions: =>Permission.security-admin
staffgroup:
Title: staffgroup
Code: staffgroup
@@ -13,14 +26,26 @@ Group:
ceogroup:
Title: ceogroup
Code: ceogroup
- Parent: =>Group.managementgroup
+ Parent: =>Group.managementgroup
+ memberlessgroup:
+ Title: Memberless Group
+ code: memberless
Member:
+ admin:
+ FirstName: Admin
+ Email: admin@silverstripe.com
+ Groups: =>Group.admingroup
+ other-admin:
+ FirstName: OtherAdmin
+ Email: other-admin@silverstripe.com
+ Groups: =>Group.admingroup
test:
FirstName: Test
Surname: User
Email: sam@silverstripe.com
Password: 1nitialPassword
PasswordExpiry: 2030-01-01
+ Groups: =>Group.securityadminsgroup
expiredpassword:
FirstName: Test
Surname: User
@@ -43,4 +68,11 @@ Member:
Groups: =>Group.accountinggroup
ceomember:
Email: ceomember@test.com
- Groups: =>Group.ceogroup
+ Groups: =>Group.ceogroup
+ grouplessmember:
+ FirstName: Groupless Member
+ noformatmember:
+ Email: noformat@test.com
+ delocalemember:
+ Email: delocalemember@test.com
+ Locale: de_DE
Please sign in to comment.
Something went wrong with that request. Please try again.