From cfa88adf4b3b7fad5a509d1512df8d0ca808a3b6 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Fri, 30 Aug 2013 13:59:38 +0200 Subject: [PATCH] FIX Privilege escalation through APPLY_ROLES assignment (SS-2013-005) See http://www.silverstripe.org/ss-2013-005-privilege-escalation-through-apply-roles-assignment/ --- docs/en/changelogs/3.0.6.md | 5 ++++ security/PermissionRole.php | 16 +++++++++++++ security/PermissionRoleCode.php | 34 +++++++++++++++++++++++++++ tests/security/PermissionRoleTest.php | 31 ++++++++++++++++++++++++ 4 files changed, 86 insertions(+) diff --git a/docs/en/changelogs/3.0.6.md b/docs/en/changelogs/3.0.6.md index 2aaac2704..b6751df70 100644 --- a/docs/en/changelogs/3.0.6.md +++ b/docs/en/changelogs/3.0.6.md @@ -30,6 +30,11 @@ See [announcement](http://www.silverstripe.org/ss-2013-003-privilege-escalation- ### 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 * If you have created your own composite database fields, then you should amend the setValue() to allow the passing of diff --git a/security/PermissionRole.php b/security/PermissionRole.php index 349573312..f0775c691 100644 --- a/security/PermissionRole.php +++ b/security/PermissionRole.php @@ -76,4 +76,20 @@ class PermissionRole extends DataObject { return $labels; } + + public function canView($member = null) { + return Permission::check('APPLY_ROLES', 'any', $member); + } + + 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); + } } diff --git a/security/PermissionRoleCode.php b/security/PermissionRoleCode.php index 0f6ba4d2c..ffd911839 100644 --- a/security/PermissionRoleCode.php +++ b/security/PermissionRoleCode.php @@ -13,4 +13,38 @@ class PermissionRoleCode extends DataObject { private 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); + } } diff --git a/tests/security/PermissionRoleTest.php b/tests/security/PermissionRoleTest.php index 10a7a6062..1cd36651c 100644 --- a/tests/security/PermissionRoleTest.php +++ b/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' + ); + } }