Permalink
Browse files

BUG Undefined `$allowed_actions` overrides parent definitions, strict…

…er 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.
  • Loading branch information...
1 parent 5d3ed12 commit 50995fbecb4b8f229deabe6cef03370b2b783e51 @chillu chillu committed Jan 14, 2013
Showing with 213 additions and 63 deletions.
  1. +30 −19 core/control/RequestHandler.php
  2. +31 −0 docs/en/changelogs/2.4.10.md
  3. +132 −35 tests/ControllerTest.php
  4. +20 −9 tests/RequestHandlingTest.php
@@ -68,7 +68,7 @@ class RequestHandler extends ViewableData {
* </code>
*
* 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 @@ function handleRequest(SS_HTTPRequest $request) {
// 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 @@ function handleRequest(SS_HTTPRequest $request) {
*/
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 @@ public function allowedActions() {
foreach($actions as $key => $value) {
if(is_numeric($key)) $actions[$key] = strtolower($value);
}
-
+
return $actions;
}
}
@@ -224,18 +224,18 @@ public function hasAction($action) {
$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 @@ function checkAccessAction($action) {
// 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 @@ public function httpError($errorCode, $errorMessage = null) {
throw $e;
}
-
+
/**
* Returns the SS_HTTPRequest object that this controller is using.
*
@@ -327,4 +338,4 @@ public function httpError($errorCode, $errorMessage = null) {
function getRequest() {
return $this->request;
}
-}
+ }
@@ -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
@@ -2,7 +2,7 @@
class ControllerTest extends FunctionalTest {
static $fixture_file = 'sapphire/tests/ControllerTest.yml';
-
+
protected $autoFollowRedirection = false;
function testDefaultAction() {
@@ -29,49 +29,83 @@ function testTemplateActions() {
}
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_AccessBaseController/unsecuredaction");
+ $this->assertEquals(200, $response->getStatusCode(),
+ 'Access granted on action without $allowed_actions on defining controller'
+ );
+
+ $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_SecuredController/stringaction");
- $this->assertEquals(404, $response->getStatusCode());
+ $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_SecuredController/adminonly");
- $this->assertEquals(403, $response->getStatusCode());
+ $response = $this->get("ControllerTest_AccessSecuredController/aDmiNOnlY");
+ $this->assertEquals(403, $response->getStatusCode(),
+ 'Access denied on action with $allowed_actions on defining controller, ' .
+ 'regardless of capitalization'
+ );
- $response = $this->get('ControllerTest_UnsecuredController/stringaction');
+ // TODO Change this API
+ $response = $this->get('ControllerTest_AccessUnsecuredSubController/unsecuredaction');
$this->assertEquals(200, $response->getStatusCode(),
- "test that a controller without a specified allowed_actions allows actions through"
+ "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_FullSecuredController/index");
+ $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 @@ function redirectbacktest() {
}
/**
- * Controller with an $allowed_actions value
+ * Allowed actions flattened:
+ * - unsecuredaction: *
+ * - onlysecuredinsubclassaction: *
+ * - unsecuredinparentclassaction: *
+ * - unsecuredinsubclassaction: *
*/
-class ControllerTest_SecuredController extends Controller {
+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(
- "methodaction",
+ "onlysecuredinsubclassaction" => 'ADMIN',
"adminonly" => "ADMIN",
);
-
- public $Content = "default content";
-
- function methodaction() {
- return array(
- "Content" => "methodaction content"
- );
+
+ // Accessible by ADMIN only
+ public function onlysecuredinsubclassaction() {
+ return 'onlysecuredinsubclass was called.';
}
-
- function stringaction() {
- return "stringaction was called.";
+
+ // Not accessible, since overloaded but not mentioned in $allowed_actions
+ public function unsecuredinsubclassaction() {
+ return 'unsecuredinsubclass was called.';
}
- function adminonly() {
+ // 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() { }
-}
+}
Oops, something went wrong.

0 comments on commit 50995fb

Please sign in to comment.