From 50995fbecb4b8f229deabe6cef03370b2b783e51 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. --- core/control/RequestHandler.php | 49 +++++---- docs/en/changelogs/2.4.10.md | 31 ++++++ tests/ControllerTest.php | 181 ++++++++++++++++++++++++-------- tests/RequestHandlingTest.php | 29 +++-- 4 files changed, 220 insertions(+), 70 deletions(-) create mode 100644 docs/en/changelogs/2.4.10.md diff --git a/core/control/RequestHandler.php b/core/control/RequestHandler.php index 5d5e600c8..662bea42e 100755 --- a/core/control/RequestHandler.php +++ b/core/control/RequestHandler.php @@ -68,7 +68,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. @@ -114,7 +114,7 @@ class RequestHandler extends ViewableData { // We stop after RequestHandler; in other words, at ViewableData while($handlerClass && $handlerClass != 'ViewableData') { $urlHandlers = Object::get_static($handlerClass, 'url_handlers'); - + if($urlHandlers) foreach($urlHandlers as $rule => $action) { if(isset($_REQUEST['debug_request'])) Debug::message("Testing '$rule' with '" . $request->remaining() . "' on $this->class"); if($params = $request->match($rule, true)) { @@ -193,13 +193,13 @@ class RequestHandler extends ViewableData { */ public function allowedActions() { $actions = Object::combined_static(get_class($this), 'allowed_actions', 'RequestHandler'); - + foreach($this->extension_instances as $extension) { if($extensionActions = Object::get_static(get_class($extension), 'allowed_actions')) { $actions = array_merge($actions, $extensionActions); } } - + if($actions) { // convert all keys and values to lowercase to // allow for easier comparison, unless it is a permission code @@ -208,7 +208,7 @@ class RequestHandler extends ViewableData { foreach($actions as $key => $value) { if(is_numeric($key)) $actions[$key] = strtolower($value); } - + return $actions; } } @@ -224,18 +224,18 @@ class RequestHandler extends ViewableData { $action = strtolower($action); $actions = $this->allowedActions(); - + // Check if the action is defined in the allowed actions as either a // key or value. Note that if the action is numeric, then keys are not // searched for actions to prevent actual array keys being recognised // as actions. if(is_array($actions)) { $isKey = !is_numeric($action) && array_key_exists($action, $actions); - $isValue = in_array($action, $actions); - - if($isKey || $isValue) return true; + $isValue = in_array($action, $actions, true); + $isWildcard = (in_array('*', $actions) && $this->checkAccessAction($action)); + if($isKey || $isValue || $isWildcard) return true; } - + if(!is_array($actions) || !$this->uninherited('allowed_actions')) { if($action != 'init' && $action != 'run' && method_exists($this, $action)) return true; } @@ -279,19 +279,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->uninherited('allowed_actions')) { - // If no allowed_actions are provided, then we should only let through actions that aren't handled by magic methods - // we test this by calling the unmagic method_exists. + + // 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 @@ -318,7 +329,7 @@ class RequestHandler extends ViewableData { throw $e; } - + /** * Returns the SS_HTTPRequest object that this controller is using. * @@ -327,4 +338,4 @@ class RequestHandler extends ViewableData { function getRequest() { return $this->request; } -} + } diff --git a/docs/en/changelogs/2.4.10.md b/docs/en/changelogs/2.4.10.md new file mode 100644 index 000000000..852160aea --- /dev/null +++ b/docs/en/changelogs/2.4.10.md @@ -0,0 +1,31 @@ +# 2.4.10 + +## Overview + + * Security: Undefined `$allowed_actions` overrides parent definitions + * API: More restrictive `$allowed_actions` checks for `Controller` when used with `Extension` + +## Details + +### Security: Undefined `$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. + +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 the 2.4.10 update. In addition, we strongly recommend to define `$allowed_actions` +on all controller classes to ensure the intentions are clearly communicated. + +### 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 \ No newline at end of file diff --git a/tests/ControllerTest.php b/tests/ControllerTest.php index 1847a95d6..f0d7d8eb2 100755 --- a/tests/ControllerTest.php +++ b/tests/ControllerTest.php @@ -2,7 +2,7 @@ class ControllerTest extends FunctionalTest { static $fixture_file = 'sapphire/tests/ControllerTest.yml'; - + protected $autoFollowRedirection = false; function testDefaultAction() { @@ -29,49 +29,83 @@ 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.'); } 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"); - $this->assertEquals(403, $response->getStatusCode(), - "Actions can be globally disallowed by using asterisk (*) instead of a method name" + $response = $this->get("ControllerTest_AccessAsteriskSecuredController/onlysecuredinsubclassaction"); + $this->assertEquals(404, $response->getStatusCode(), + "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_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); } /** @@ -206,48 +240,111 @@ class ControllerTest_Controller extends Controller { } /** - * Controller with an $allowed_actions value + * Allowed actions flattened: + * - unsecuredaction: * + * - onlysecuredinsubclassaction: * + * - unsecuredinparentclassaction: * + * - unsecuredinsubclassaction: * */ -class ControllerTest_SecuredController extends Controller { - static $allowed_actions = array( - "methodaction", - "adminonly" => "ADMIN", - ); - - public $Content = "default content"; - - function methodaction() { - return array( - "Content" => "methodaction content" - ); - } - - function stringaction() { - return "stringaction was called."; +class ControllerTest_AccessBaseController extends Controller implements TestOnly { + // Accessible by all + public function unsecuredaction() { + return 'unsecuredaction was called.'; } - function adminonly() { + // 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 { +/** + * 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, ); - function adminonly() { + // Accessible by ADMIN only + public function adminonly() { return "You must be an admin!"; } - function unsecuredaction() { + // Accessible by all + public function unsecuredaction() { return "Allowed for everybody"; } } -class ControllerTest_UnsecuredController extends ControllerTest_SecuredController {} +/** + * 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 { @@ -266,4 +363,4 @@ class ControllerTest_HasAction_Unsecured extends ControllerTest_HasAction { public function defined_action() { } -} \ No newline at end of file +} diff --git a/tests/RequestHandlingTest.php b/tests/RequestHandlingTest.php index dc63fa4fb..d39dff422 100755 --- a/tests/RequestHandlingTest.php +++ b/tests/RequestHandlingTest.php @@ -139,9 +139,9 @@ class RequestHandlingTest extends FunctionalTest { $this->assertEquals('This page does not exist.', $response->getBody()); } - function testMethodsOnParentClassesOfRequestHandlerDeclined() { - $response = Director::test('testGoodBase1/getSecurityID'); - $this->assertEquals(403, $response->getStatusCode()); + public function testMethodsOnParentClassesOfRequestHandlerDeclined() { + $response = Director::test('testGoodBase1/getIterator'); + $this->assertEquals(404, $response->getStatusCode()); } function testFormActionsCanBypassAllowedActions() { @@ -229,8 +229,8 @@ class RequestHandlingTest extends FunctionalTest { $this->assertContains('not allowed on form', $response->getBody()); } -} - + } + /** * Director rules for the test */ @@ -260,7 +260,18 @@ Director::addRules(50, array( /** * Controller for the test */ -class RequestHandlingTest_Controller extends Controller { +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", @@ -312,8 +323,8 @@ class RequestHandlingTest_Controller extends Controller { public function throwhttperror() { $this->httpError(404, 'This page does not exist.'); } - -} + + } class RequestHandlingTest_FormActionController extends Controller { @@ -364,7 +375,7 @@ class RequestHandlingTest_FormActionController extends Controller { function formactionInAllowedActions($data, $form = null) { return 'formactionInAllowedActions'; } -} + } /** * Simple extension for the test controller