Skip to content
This repository
Browse code

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 de1f07045ba35273174907404c92aa0e9d7f1d9a 1 parent 5d87f29
Ingo Schommer authored March 09, 2011
18  security/Member.php
@@ -693,6 +693,24 @@ function onAfterWrite() {
693 693
 			MemberPassword::log($this);
694 694
 		}
695 695
 	}
  696
+	
  697
+	/**
  698
+	 * If any admin groups are requested, deny the whole save operation.
  699
+	 * 
  700
+	 * @param Array $ids Database IDs of Group records
  701
+	 * @return boolean
  702
+	 */
  703
+	function onChangeGroups($ids) {
  704
+		// Filter out admin groups to avoid privilege escalation, 
  705
+		// unless the current user is an admin already
  706
+		if(!Permission::checkMember($this, 'ADMIN')) {
  707
+			$adminGroups = Permission::get_groups_by_permission('ADMIN');
  708
+			$adminGroupIDs = ($adminGroups) ? $adminGroups->column('ID') : array();
  709
+			return count(array_intersect($ids, $adminGroupIDs)) == 0;
  710
+		} else {
  711
+			return true;
  712
+		}
  713
+	}
696 714
 
697 715
 
698 716
 	/**
29  tests/security/MemberTest.php
@@ -527,6 +527,35 @@ function testMembersWithSecurityAdminAccessCantEditAdminsUnlessTheyreAdminsThems
527 527
 		$this->assertFalse($adminMember->canEdit($securityAdminMember), 'Security-Admins can not edit other admins');
528 528
 		$this->assertTrue($ceoMember->canEdit($securityAdminMember), 'Security-Admins can edit other members');
529 529
 	}
  530
+	
  531
+	function testOnChangeGroups() {
  532
+		$staffGroup = $this->objFromFixture('Group', 'staffgroup');
  533
+		$adminGroup = $this->objFromFixture('Group', 'admingroup');
  534
+		$staffMember = $this->objFromFixture('Member', 'staffmember');
  535
+		$adminMember = $this->objFromFixture('Member', 'admin');
  536
+		$newAdminGroup = new Group(array('Title' => 'newadmin'));
  537
+		$newAdminGroup->write();
  538
+		Permission::grant($newAdminGroup->ID, 'ADMIN');
  539
+		$newOtherGroup = new Group(array('Title' => 'othergroup'));
  540
+		$newOtherGroup->write();
  541
+		
  542
+		$this->assertTrue(
  543
+			$staffMember->onChangeGroups(array($staffGroup->ID)),
  544
+			'Adding existing non-admin group relation is allowed for non-admin members'
  545
+		);
  546
+		$this->assertTrue(
  547
+			$staffMember->onChangeGroups(array($newOtherGroup->ID)),
  548
+			'Adding new non-admin group relation is allowed for non-admin members'
  549
+		);
  550
+		$this->assertFalse(
  551
+			$staffMember->onChangeGroups(array($newAdminGroup->ID)),
  552
+			'Adding new admin group relation is not allowed for non-admin members'
  553
+		);
  554
+		$this->assertTrue(
  555
+			$adminMember->onChangeGroups(array($newAdminGroup->ID)),
  556
+			'Adding new admin group relation is allowed for admin members'
  557
+		);
  558
+	}
530 559
 
531 560
 	/**
532 561
 	 * Add the given array of member extensions as class names.

0 notes on commit de1f070

Please sign in to comment.
Something went wrong with that request. Please try again.