Merge branch 'pulls/security-issues-august-3.0' into 3.0

admin/code/SecurityAdmin.php

@@ -86,7 +86,7 @@ class SecurityAdmin extends LeftAndMain implements PermissionProvider {
 		$columns->setDisplayFields(array(
 			'Breadcrumbs' => singleton('Group')->fieldLabel('Title')
 		));
-		
+
 		$fields = new FieldList(
 			$root = new TabSet(
 				'Root',
@@ -100,34 +100,42 @@ class SecurityAdmin extends LeftAndMain implements PermissionProvider {
 									. ' database'
 							)
 						)
-					),
-					new HeaderField(_t('SecurityAdmin.IMPORTUSERS', 'Import users'), 3),
-					new LiteralField(
-						'MemberImportFormIframe',
-						sprintf(
-							'<iframe src="%s" id="MemberImportFormIframe" width="100%%" height="250px" border="0">'
-							. '</iframe>',
-							$this->Link('memberimport')
-						)
 					)
 				),
 				$groupsTab = new Tab('Groups', singleton('Group')->i18n_plural_name(),
-					$groupList,
-					new HeaderField(_t('SecurityAdmin.IMPORTGROUPS', 'Import groups'), 3),
-					new LiteralField(
-						'GroupImportFormIframe',
-						sprintf(
-							'<iframe src="%s" id="GroupImportFormIframe" width="100%%" height="250px" border="0">'
-							. '</iframe>',
-							$this->Link('groupimport')
-						)
-					)
+					$groupList
 				)
 			),
 			// necessary for tree node selection in LeftAndMain.EditForm.js
 			new HiddenField('ID', false, 0)
 		);
 
+		// Add import capabilities. Limit to admin since the import logic can affect assigned permissions
+		if(Permission::check('ADMIN')) {
+			$fields->addFieldsToTab('Root.Users', array(
+				new HeaderField(_t('SecurityAdmin.IMPORTUSERS', 'Import users'), 3),
+				new LiteralField(
+					'MemberImportFormIframe',
+					sprintf(
+						'<iframe src="%s" id="MemberImportFormIframe" width="100%%" height="250px" border="0">'
+						. '</iframe>',
+						$this->Link('memberimport')
+					)
+				)
+			));
+			$fields->addFieldsToTab('Root.Groups', array(
+				new HeaderField(_t('SecurityAdmin.IMPORTGROUPS', 'Import groups'), 3),
+				new LiteralField(
+					'GroupImportFormIframe',
+					sprintf(
+						'<iframe src="%s" id="GroupImportFormIframe" width="100%%" height="250px" border="0">'
+						. '</iframe>',
+						$this->Link('groupimport')
+					)
+				)
+			));
+		}
+
 		// Tab nav in CMS is rendered through separate template		
 		$root->setTemplate('CMSTabSet');
 		
@@ -194,6 +202,8 @@ class SecurityAdmin extends LeftAndMain implements PermissionProvider {
 	 * @return Form
 	 */
 	public function MemberImportForm() {
+		if(!Permission::check('ADMIN')) return false;
+
 		$group = $this->currentPage();
 		$form = new MemberImportForm(
 			$this,
@@ -224,6 +234,8 @@ class SecurityAdmin extends LeftAndMain implements PermissionProvider {
 	 * @return Form
 	 */
 	public function GroupImportForm() {
+		if(!Permission::check('ADMIN')) return false;
+
 		$form = new GroupImportForm(
 			$this,
 			'GroupImportForm'

docs/en/changelogs/3.0.6.md

@@ -7,21 +7,21 @@
 
 ## 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,
-it can facilitate [denial-of-service attacks](https://en.wikipedia.org/wiki/Denial-of-service_attack).
+See [announcement](http://www.silverstripe.org/ss-2013-001-require-admin-for-flush1/)
 
-To prevent this, main.php now checks and only allows the flush parameter in the following cases:
+### Security: Privilege escalation through Group hierarchy setting (SS-2013-003)
 
- * The [environment](/topics/environment-management) is in "dev mode"
- * A user is logged in with ADMIN permissions
- * An error occurs during startup
+See [announcement](http://www.silverstripe.org/ss-2013-003-privilege-escalation-through-group-hierarchy-setting/)
 
-This applies to both `flush=1` and `flush=all` (technically we only check for the existence of any parameter value)
-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 and Member CSV upload (SS-2013-004)
+
+See [announcement](http://www.silverstripe.org/ss-2013-004-privilege-escalation-through-group-and-member-csv-upload/)
+
+### Security: Privilege escalation through APPLY_ROLES assignment (SS-2013-005)
+
+See [announcement](http://www.silverstripe.org/ss-2013-005-privilege-escalation-through-apply-roles-assignment/)
 
 ## Upgrading
 

model/DataObject.php

@@ -167,6 +167,11 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 	
 	/**
 	 * Set whether DataObjects should be validated before they are written.
+	 * 
+	 * Caution: Validation can contain safeguards against invalid/malicious data,
+	 * and check permission levels (e.g. on {@link Group}). Therefore it is recommended
+	 * to only disable validation for very specific use cases.
+	 * 
 	 * @param $enable bool
 	 * @see DataObject::validate()
 	 */

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();

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.
 	 * 

security/PermissionCheckboxSetField.php

@@ -161,6 +161,8 @@ class PermissionCheckboxSetField extends FormField {
 		$odd = 0;
 		$options = '';
 		if($this->source) {
+			$privilegedPermissions = Permission::config()->privileged_permissions;
+
 			// loop through all available categorized permissions and see if they're assigned for the given groups
 			foreach($this->source as $categoryName => $permissions) {
 				$options .= "<li><h5>$categoryName</h5></li>";
@@ -193,6 +195,11 @@ class PermissionCheckboxSetField extends FormField {
 						$inheritMessage = ' (' . join(', ', $uninheritedCodes[$code]).')';
 					}
 
+					// Disallow modification of "privileged" permissions unless currently logged-in user is an admin
+					if(!Permission::check('ADMIN') && in_array($code, $privilegedPermissions)) {
+						$disabled = ' disabled="true"';
+					}
+
 					// If the field is readonly, always mark as "disabled"
 					if($this->readonly) $disabled = ' disabled="true"';
 					
@@ -245,6 +252,16 @@ class PermissionCheckboxSetField extends FormField {
 		$fieldname = $this->name;
 		$managedClass = $this->managedClass;
 
+		// Remove all "privileged" permissions if the currently logged-in user is not an admin
+		$privilegedPermissions = Permission::config()->privileged_permissions;
+		if(!Permission::check('ADMIN')) {
+			foreach($this->value as $id => $bool) {
+				if(in_array($id, $privilegedPermissions)) {
+					unset($this->value[$id]);
+				}
+			}	
+		}
+
 		// remove all permissions and re-add them afterwards
 		$permissions = $record->$fieldname();
 		foreach ( $permissions as $permission ) {

security/PermissionRole.php

@@ -74,4 +74,16 @@ class PermissionRole extends DataObject {
 		
 		return $labels;
 	}
+
+	public function canCreate($member = null) {
+		return Permission::check('APPLY_ROLES', 'any', $member);
+	}
+
+	public function canEdit($member = null) {
+		return Permission::check('APPLY_ROLES', 'any', $member);
+	}
+
+	public function canDelete($member = null) {
+		return Permission::check('APPLY_ROLES', 'any', $member);
+	}
 }

security/PermissionRoleCode.php

@@ -13,4 +13,38 @@ class PermissionRoleCode extends DataObject {
 	static $has_one = array(
 		"Role" => "PermissionRole",
 	);
+
+	protected function validate() {
+		$result = parent::validate();
+
+		// Check that new code doesn't increase privileges, unless an admin is editing.
+		$privilegedCodes = Config::inst()->get('Permission', 'privileged_permissions');
+		if(
+			$this->Code
+			&& in_array($this->Code, $privilegedCodes)
+			&& !Permission::check('ADMIN')
+		) {
+			$result->error(sprintf(
+				_t(
+					'PermissionRoleCode.PermsError',
+					'Can\'t assign code "%s" with privileged permissions (requires ADMIN access)'
+				),
+				$this->Code
+			));
+		}
+
+		return $result;
+	}
+
+	public function canCreate($member = null) {
+		return Permission::check('APPLY_ROLES', 'any', $member);
+	}
+
+	public function canEdit($member = null) {
+		return Permission::check('APPLY_ROLES', 'any', $member);
+	}
+
+	public function canDelete($member = null) {
+		return Permission::check('APPLY_ROLES', 'any', $member);
+	}
 }

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 {

tests/security/PermissionRoleTest.php

@@ -16,4 +16,35 @@ class PermissionRoleTest extends FunctionalTest {
 		$this->assertEquals(0, DataObject::get('PermissionRoleCode',"\"RoleID\"={$role->ID}")->count(),
 			'Permissions removed along with the role');
 	}
+
+	public function testValidatesPrivilegedPermissions() {
+		$nonAdminCode = new PermissionRoleCode(array('Code' => 'CMS_ACCESS_CMSMain'));
+		$nonAdminValidateMethod = new ReflectionMethod($nonAdminCode, 'validate');
+		$nonAdminValidateMethod->setAccessible(true);
+
+		$adminCode = new PermissionRoleCode(array('Code' => 'ADMIN'));
+		$adminValidateMethod = new ReflectionMethod($adminCode, 'validate');
+		$adminValidateMethod->setAccessible(true);
+
+		$this->logInWithPermission('APPLY_ROLES');
+		$result = $nonAdminValidateMethod->invoke($nonAdminCode);
+		$this->assertTrue(
+			$result->valid(),
+			'Members with only APPLY_ROLES can create non-privileged permission role codes'
+		);
+
+		$this->logInWithPermission('APPLY_ROLES');
+		$result = $adminValidateMethod->invoke($adminCode);
+		$this->assertFalse(
+			$result->valid(),
+			'Members with only APPLY_ROLES can\'t create privileged permission role codes'
+		);
+
+		$this->logInWithPermission('ADMIN');
+		$result = $adminValidateMethod->invoke($adminCode);
+		$this->assertTrue(
+			$result->valid(),
+			'Members with ADMIN can create privileged permission role codes'
+		);
+	}
 }