From f06ba70fc98bd58441a02889bfdb6849e6bd22bd Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Tue, 15 Jan 2013 00:34:05 +0100 Subject: [PATCH] BUG Undefined `$allowed_actions` overrides parent definitions, stricter handling of $allowed_actions on Extension Controller (and subclasses) failed to enforce $allowed_action restrictions on parent classes if a child class didn't have it explicitly defined. Controllers which are extended with $allowed_actions (through an Extension) now deny access to methods defined on the controller, unless this class also has them in its own $allowed_actions definition. --- control/RequestHandler.php | 27 +++-- docs/en/changelogs/3.0.4.md | 33 +++++- tests/control/ControllerTest.php | 165 ++++++++++++++++++++------ tests/control/RequestHandlingTest.php | 13 +- 4 files changed, 188 insertions(+), 50 deletions(-) diff --git a/control/RequestHandler.php b/control/RequestHandler.php index 38221aa75..c46403367 100644 --- a/control/RequestHandler.php +++ b/control/RequestHandler.php @@ -88,7 +88,7 @@ class RequestHandler extends ViewableData { * * * Form getters count as URL actions as well, and should be included in allowed_actions. - * Form actions on the other handed (first argument to {@link FormAction()} shoudl NOT be included, + * Form actions on the other handed (first argument to {@link FormAction()} should NOT be included, * these are handled separately through {@link Form->httpSubmission}. You can control access on form actions * either by conditionally removing {@link FormAction} in the form construction, * or by defining $allowed_actions in your {@link Form} class. @@ -321,21 +321,30 @@ class RequestHandler extends ViewableData { // If we get here an the action is 'index', then it hasn't been specified, which means that // it should be allowed. if($action == 'index' || empty($action)) return true; - - if($allowedActions === null || !$this->config()->get('allowed_actions', - Config::UNINHERITED | Config::EXCLUDE_EXTRA_SOURCES)) { - - // If no allowed_actions are provided, then we should only let through actions that aren't handled by + + // Get actions defined for this specific class + $relevantActions = null; + if($allowedActions !== null && method_exists($this, $action)) { + $r = new ReflectionClass(get_class($this)); + $m = $r->getMethod($actionOrigCasing); + $relevantActions = Object::uninherited_static( + $m->getDeclaringClass()->getName(), + 'allowed_actions' + ); + } + + // If no allowed_actions are provided on in the whole inheritance chain, + // or they aren't provided on the specific class... + if($allowedActions === null || $relevantActions === null) { + // ... only let through actions that aren't handled by // magic methods we test this by calling the unmagic method_exists. if(method_exists($this, $action)) { - // Disallow any methods which aren't defined on RequestHandler or subclasses - // (e.g. ViewableData->getSecurityID()) $r = new ReflectionClass(get_class($this)); if($r->hasMethod($actionOrigCasing)) { $m = $r->getMethod($actionOrigCasing); return ($m && is_subclass_of($m->getDeclaringClass()->getName(), 'RequestHandler')); } else { - throw new Exception("method_exists() true but ReflectionClass can't find method - PHP is b0kred"); + throw new Exception("method_exists() true but ReflectionClass can't find method"); } } else if(!$this->hasMethod($action)){ // Return true so that a template can handle this action diff --git a/docs/en/changelogs/3.0.4.md b/docs/en/changelogs/3.0.4.md index 688bee870..bbeee3bdc 100644 --- a/docs/en/changelogs/3.0.4.md +++ b/docs/en/changelogs/3.0.4.md @@ -2,16 +2,39 @@ ## Overview + * Security: Undefined or empty `$allowed_actions` overrides parent definitions * Security: Information leakage through web access on YAML configuration files * Security: Information leakage through web access on composer files * Security: Require ADMIN permissions for `?showtemplate=1` * Security: Reflected XSS in custom date/time formats in admin/security * Security: Stored XSS in the "New Group" dialog * Security: Reflected XSS in CMS status messages + * API: More restrictive `$allowed_actions` checks for `Controller` when used with `Extension` * Changed `dev/tests/setdb` and `dev/tests/startsession` from session to cookie storage. ## Details +### Security: Undefined or empty `$allowed_actions` overrides parent definitions + +Severity: Important + +Description: `Controller` (and subclasses) failed to enforce `$allowed_action` restrictions +on parent classes if a child class didn't have it explicitly defined, or it is set to an empty array. +Since this is the default configuration on `Page_Controller`, most SilverStripe installations +will be affected. + +Impact: Depends on the used controller code. For any method with public visibility, +the flaw can expose the return value of the method (unless it fails due to wrong arguments). +It can also lead to unauthorized or unintended execution of logic, e.g. modifying the +state of a database record. + +Fix: Apply 3.0.4 update. In addition, we strongly recommend to define `$allowed_actions` +on all controller classes to ensure the intentions are clearly communicated. +Read more about `$allowed_actions` in our "[controller](/topics/controller/#access-control)" +docs. + +Reporter: Zann St Pierre + ### Security: Information exposure through web access on YAML configuration files Severity: Moderate @@ -53,8 +76,6 @@ Severity: Low Description: Avoids information leakage of compiled template data, which might expose some of the internal template logic. -## Upgrading - ### Security: Reflected XSS in custom date/time formats in admin/security Severity: Low @@ -94,7 +115,13 @@ a specifically crafted "Title" field. Credits: Andreas Hunkeler (Compass Security AG, http://www.csnc.ch) -### Misc +### API: More restrictive `$allowed_actions` checks for `Controller` when used with `Extension` + +Controllers which are extended with `$allowed_actions` (through an `Extension`) +now deny access to methods defined on the controller, unless this class also has them in its own +`$allowed_actions` definition. + +## Upgrading * If you are using `dev/tests/setdb` and `dev/tests/startsession`, you'll need to configure a secure token in order to encrypt the cookie value: diff --git a/tests/control/ControllerTest.php b/tests/control/ControllerTest.php index f02fe50c4..a47bbba5a 100644 --- a/tests/control/ControllerTest.php +++ b/tests/control/ControllerTest.php @@ -31,54 +31,82 @@ class ControllerTest extends FunctionalTest { } public function testUndefinedActions() { - $response = Director::test('ControllerTest_UnsecuredController/undefinedaction'); + $response = Director::test('ControllerTest_AccessUnsecuredSubController/undefinedaction'); $this->assertEquals(404, $response->getStatusCode(), 'Undefined actions return a not found response.'); } public function testAllowedActions() { $adminUser = $this->objFromFixture('Member', 'admin'); - $response = $this->get("ControllerTest_SecuredController/methodaction"); - $this->assertEquals(200, $response->getStatusCode()); - - $response = $this->get("ControllerTest_SecuredController/stringaction"); - $this->assertEquals(404, $response->getStatusCode()); + $response = $this->get("ControllerTest_AccessBaseController/unsecuredaction"); + $this->assertEquals(200, $response->getStatusCode(), + 'Access granted on action without $allowed_actions on defining controller' + ); - $response = $this->get("ControllerTest_SecuredController/adminonly"); - $this->assertEquals(403, $response->getStatusCode()); - - $response = $this->get('ControllerTest_UnsecuredController/stringaction'); - $this->assertEquals(200, $response->getStatusCode(), - "test that a controller without a specified allowed_actions allows actions through" + $response = $this->get("ControllerTest_AccessBaseController/onlysecuredinsubclassaction"); + $this->assertEquals(200, $response->getStatusCode(), + 'Access granted on action without $allowed_actions on defining controller, ' . + 'even when action is secured in subclasses' ); - $response = $this->get("ControllerTest_FullSecuredController/index"); + $response = $this->get("ControllerTest_AccessSecuredController/onlysecuredinsubclassaction"); + $this->assertEquals(403, $response->getStatusCode(), + 'Access denied on action with $allowed_actions on defining controller, ' . + 'even if action is unsecured on parent class' + ); + + $response = $this->get("ControllerTest_AccessSecuredController/adminonly"); + $this->assertEquals(403, $response->getStatusCode(), + 'Access denied on action with $allowed_actions on defining controller, ' . + 'when action is not defined on any parent classes' + ); + + $response = $this->get("ControllerTest_AccessSecuredController/aDmiNOnlY"); + $this->assertEquals(403, $response->getStatusCode(), + 'Access denied on action with $allowed_actions on defining controller, ' . + 'regardless of capitalization' + ); + + // TODO Change this API + $response = $this->get('ControllerTest_AccessUnsecuredSubController/unsecuredaction'); + $this->assertEquals(200, $response->getStatusCode(), + "Controller without a specified allowed_actions allows its own actions through" + ); + + $response = $this->get('ControllerTest_AccessUnsecuredSubController/adminonly'); + $this->assertEquals(403, $response->getStatusCode(), + "Controller without a specified allowed_actions still disallows actions defined on parents" + ); + + $response = $this->get("ControllerTest_AccessAsteriskSecuredController/index"); $this->assertEquals(403, $response->getStatusCode(), "Actions can be globally disallowed by using asterisk (*) for index method" ); - $response = $this->get("ControllerTest_FullSecuredController/adminonly"); + $response = $this->get("ControllerTest_AccessAsteriskSecuredController/onlysecuredinsubclassaction"); $this->assertEquals(404, $response->getStatusCode(), - "Actions can be globally disallowed by using asterisk (*) instead of a method name" + "Actions can be globally disallowed by using asterisk (*) instead of a method name, " . + "in which case they'll be marked as 'not found'" ); - $response = $this->get("ControllerTest_FullSecuredController/unsecuredaction"); + $response = $this->get("ControllerTest_AccessAsteriskSecuredController/unsecuredaction"); $this->assertEquals(200, $response->getStatusCode(), "Actions can be overridden to be allowed if globally disallowed by using asterisk (*)" ); $this->session()->inst_set('loggedInAs', $adminUser->ID); - $response = $this->get("ControllerTest_SecuredController/adminonly"); + $response = $this->get("ControllerTest_AccessSecuredController/adminonly"); $this->assertEquals( 200, $response->getStatusCode(), "Permission codes are respected when set in \$allowed_actions" ); - $response = $this->get("ControllerTest_FullSecuredController/adminonly"); + $response = $this->get("ControllerTest_AccessUnsecuredSubController/adminonly"); $this->assertEquals(200, $response->getStatusCode(), "Actions can be globally disallowed by using asterisk (*) instead of a method name" ); + $this->session()->inst_set('loggedInAs', null); } @@ -227,48 +255,111 @@ class ControllerTest_Controller extends Controller implements TestOnly { } /** - * Controller with an $allowed_actions value + * Allowed actions flattened: + * - unsecuredaction: * + * - onlysecuredinsubclassaction: * + * - unsecuredinparentclassaction: * + * - unsecuredinsubclassaction: * */ -class ControllerTest_SecuredController extends Controller implements TestOnly { - static $allowed_actions = array( - "methodaction", - "adminonly" => "ADMIN", - ); - - public $Content = "default content"; - - public function methodaction() { - return array( - "Content" => "methodaction content" - ); - } - - public function stringaction() { - return "stringaction was called."; +class ControllerTest_AccessBaseController extends Controller implements TestOnly { + // Accessible by all + public function unsecuredaction() { + return 'unsecuredaction was called.'; } + // Accessible by all + public function onlysecuredinsubclassaction() { + return 'onlysecuredinsubclass was called.'; + } + + // Accessible by all + public function unsecuredinparentclassaction() { + return 'unsecuredinparentclassaction was called.'; + } + + // Accessible by all + public function unsecuredinsubclassaction() { + return 'unsecuredinsubclass was called.'; + } +} + +/** + * Allowed actions flattened: + * - unsecuredaction: * + * - onlysecuredinsubclassaction: ADMIN (parent: *) + * - unsecuredinparentclassaction: * + * - unsecuredinsubclassaction: (none) (parent: *) + * - adminonly: ADMIN + */ +class ControllerTest_AccessSecuredController extends ControllerTest_AccessBaseController implements TestOnly { + + static $allowed_actions = array( + "onlysecuredinsubclassaction" => 'ADMIN', + "adminonly" => "ADMIN", + ); + + // Accessible by ADMIN only + public function onlysecuredinsubclassaction() { + return 'onlysecuredinsubclass was called.'; + } + + // Not accessible, since overloaded but not mentioned in $allowed_actions + public function unsecuredinsubclassaction() { + return 'unsecuredinsubclass was called.'; + } + + // Accessible by all, since only defined in parent class + //public function unsecuredinparentclassaction() { + + // Accessible by ADMIN only public function adminonly() { return "You must be an admin!"; } } -class ControllerTest_FullSecuredController extends Controller implements TestOnly { +/** + * Allowed actions flattened: + * - unsecuredaction: * + * - onlysecuredinsubclassaction: ADMIN (parent: *) + * - unsecuredinparentclassaction: ADMIN (parent: *) + * - unsecuredinsubclassaction: (none) (parent: *) + * - adminonly: ADMIN (parent: ADMIN) + */ +class ControllerTest_AccessAsteriskSecuredController extends ControllerTest_AccessBaseController implements TestOnly { static $allowed_actions = array( "*" => "ADMIN", 'unsecuredaction' => true, ); + // Accessible by ADMIN only public function adminonly() { return "You must be an admin!"; } + // Accessible by all public function unsecuredaction() { return "Allowed for everybody"; } } -class ControllerTest_UnsecuredController extends ControllerTest_SecuredController implements TestOnly {} +/** + * Allowed actions flattened: + * - unsecuredaction: * + * - onlysecuredinsubclassaction: ADMIN (parent: *) + * - unsecuredinparentclassaction: * + * - unsecuredinsubclassaction: (none) (parent: *) + * - adminonly: ADMIN + */ +class ControllerTest_AccessUnsecuredSubController extends ControllerTest_AccessSecuredController implements TestOnly { + + // No $allowed_actions defined here + + // Accessible by ADMIN only, defined in parent class + public function onlysecuredinsubclassaction() { + return 'onlysecuredinsubclass was called.'; + } +} class ControllerTest_HasAction extends Controller { diff --git a/tests/control/RequestHandlingTest.php b/tests/control/RequestHandlingTest.php index 40fa57d22..698072d58 100644 --- a/tests/control/RequestHandlingTest.php +++ b/tests/control/RequestHandlingTest.php @@ -147,7 +147,7 @@ class RequestHandlingTest extends FunctionalTest { public function testMethodsOnParentClassesOfRequestHandlerDeclined() { $response = Director::test('testGoodBase1/getIterator'); - $this->assertEquals(403, $response->getStatusCode()); + $this->assertEquals(404, $response->getStatusCode()); } public function testFormActionsCanBypassAllowedActions() { @@ -280,6 +280,17 @@ Config::inst()->update('Director', 'rules', array( * Controller for the test */ class RequestHandlingTest_Controller extends Controller implements TestOnly { + + static $allowed_actions = array( + 'method', + 'legacymethod', + 'virtualfile', + 'TestForm', + 'throwexception', + 'throwresponseexception', + 'throwhttperror', + ); + static $url_handlers = array( // The double-slash is need here to ensure that '$Action//$ID/$OtherID' => "handleAction",