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

@@ -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 @@ 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',
+		// Get actions defined for this specific class
-				Config::UNINHERITED | Config::EXCLUDE_EXTRA_SOURCES)) {
+		$relevantActions = null;
-			
+		if($allowedActions !== null && method_exists($this, $action)) {
-			// If no allowed_actions are provided, then we should only let through actions that aren't handled by
+			$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

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:

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");
+		$response = $this->get("ControllerTest_AccessBaseController/unsecuredaction");
-		$this->assertEquals(200, $response->getStatusCode());
+		$this->assertEquals(200, $response->getStatusCode(),
-		
+			'Access granted on action without $allowed_actions on defining controller'
-		$response = $this->get("ControllerTest_SecuredController/stringaction");
+		);
-		$this->assertEquals(404, $response->getStatusCode());
 
-		$response = $this->get("ControllerTest_SecuredController/adminonly");
+		$response = $this->get("ControllerTest_AccessBaseController/onlysecuredinsubclassaction");
-		$this->assertEquals(403, $response->getStatusCode());
+		$this->assertEquals(200, $response->getStatusCode(),
-		
+			'Access granted on action without $allowed_actions on defining controller, ' .
-		$response = $this->get('ControllerTest_UnsecuredController/stringaction');
+			'even when action is secured in subclasses'
-		$this->assertEquals(200, $response->getStatusCode(), 
-			"test that a controller without a specified allowed_actions allows actions through"
 		);
 		
-		$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 {
+class ControllerTest_AccessBaseController extends Controller implements TestOnly {
-	static $allowed_actions = array(
+	// Accessible by all
-		"methodaction",
+	public function unsecuredaction() {
-		"adminonly" => "ADMIN",
+		return 'unsecuredaction was called.';
-	);
-	
-	public $Content = "default content";
-	
-	public function methodaction() {
-		return array(
-			"Content" => "methodaction content"
-		);
-	}
-	
-	public function stringaction() {
-		return "stringaction 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 {
 	

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",