tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-06-30 16:39:38 +02:00
Code Issues Projects Releases Wiki Activity

BUG Undefined $allowed_actions overrides parent definitions, stricter handling of $allowed_actions on Extension

Browse Source 
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.
This commit is contained in:
Ingo Schommer 2013-01-15 00:34:05 +01:00
parent 5d3ed12e20
commit 50995fbecb
4 changed files with 220 additions and 70 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

49
core/control/RequestHandler.php
View File

@ -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 @@ 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;
}
}
}

31
docs/en/changelogs/2.4.10.md Normal file
View File

@ -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

181
tests/ControllerTest.php
View File

@ -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() { }
}
}

29
tests/RequestHandlingTest.php
View File

@ -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