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 3033529 commit f06ba70fc98bd58441a02889bfdb6849e6bd22bd @chillu chillu committed Jan 14, 2013
Showing with 181 additions and 43 deletions.
  1. +18 −9 control/RequestHandler.php
  2. +30 −3 docs/en/changelogs/3.0.4.md
  3. +121 −30 tests/control/ControllerTest.php
  4. +12 −1 tests/control/RequestHandlingTest.php
@@ -88,7 +88,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.
@@ -321,21 +321,30 @@ public 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->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
@@ -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:
@@ -31,54 +31,82 @@ public 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.');
}
public 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_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, ' .
+ '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'
+ );
- $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");
+ $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 @@ public function redirectbacktest() {
}
/**
- * Controller with an $allowed_actions value
+ * Allowed actions flattened:
+ * - unsecuredaction: *
+ * - onlysecuredinsubclassaction: *
+ * - unsecuredinparentclassaction: *
+ * - unsecuredinsubclassaction: *
+ */
+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_SecuredController extends Controller implements TestOnly {
+class ControllerTest_AccessSecuredController extends ControllerTest_AccessBaseController implements TestOnly {
+
static $allowed_actions = array(
- "methodaction",
+ "onlysecuredinsubclassaction" => 'ADMIN',
"adminonly" => "ADMIN",
);
-
- public $Content = "default content";
-
- public function methodaction() {
- return array(
- "Content" => "methodaction content"
- );
+
+ // Accessible by ADMIN only
+ public function onlysecuredinsubclassaction() {
+ return 'onlysecuredinsubclass was called.';
}
-
- public function stringaction() {
- return "stringaction 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 {
@@ -147,7 +147,7 @@ public function testHTTPError() {
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 @@ public function testActionHandlingOnField() {
* 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",

0 comments on commit f06ba70

Please sign in to comment.