CVE-2016-9182: Improper Access Control (CWE-284)
value: Exponent CMS 2.4 uses PHP reflection to call a method of a controller class, and then uses the method name to check user permission. But, the method name in PHP reflection is case insensitive, and Exponent CMS permits undefined actions to execute by default, so an attacker can use a capitalized method name to bypass the permission check, e.g., controller=expHTMLEditor&action=preview&editor=ckeditor and controller=expHTMLEditor&action=Preview&editor=ckeditor. An anonymous user will be rejected for the former but can access the latter.
CWE description: The product does not restrict or incorrectly restricts access to a resource from an unauthorized actor.
CVSS v3 base score: 7.5
Published: 2016-11-04T10:59Z
Repository: https://github.com/exponentcms/exponent-cms
Language: PHP
Files changed: framework/core/controllers/expController.php, framework/core/expFramework.php, framework/core/subsystems/expString.php, framework/modules/file/controllers/fileController.php, framework/modules/forms/controllers/formsController.php, framework/modules/navigation/controllers/navigationController.php, framework/modules/pixidou/controllers/pixidouController.php
Commit date: 2016-11-03 09:23:12 -0400
Commit message: fix security vulnerability to bypass permissions using method name in wrong case, reported by fyth

Diff: diff --git a/framework/core/controllers/expController.php b/framework/core/controllers/expController.php
index c90afd5c38..a989f3ff45 100755
--- a/framework/core/controllers/expController.php
+++ b/framework/core/controllers/expController.php
@@ -51,11 +51,11 @@ abstract class expController {
         'export'    => 'Export Items'
     );
     protected $remove_permissions = array();  // $permissions not applicable for this module from above list
     protected $add_permissions = array();  // additional $permissions processed and visible  for this module
     protected $manage_permissions = array();  // additional actions requiring manage permission in addition to $m_permissions
-    public $requires_login = array();  // actions/methods which ONLY require user be logged in to access...$permissions take priority
+    public $requires_login = array();  // actions/methods (lower case ONLY) which ONLY require user be logged in to access...$permissions take priority
 
     public $filepath = ''; // location of this controller's files
     public $viewpath = ''; // location of this controllers views; defaults to controller file location
     public $relative_viewpath = ''; // relative location of controller's views
     public $asset_path = ''; // location of this controller's assets; defaults to controller file location
diff --git a/framework/core/expFramework.php b/framework/core/expFramework.php
index 913905fd74..b7e97ecc41 100755
--- a/framework/core/expFramework.php
+++ b/framework/core/expFramework.php
@@ -275,10 +275,11 @@ function renderAction(array $parms=array()) {
         $action = $parms['action'];
         /* TODO:  Not sure if we need to check for private methods to be here. FJD
 		$meth = $controllerClass->getMethod($action);
         if ($meth->isPrivate()) expQueue::flashAndFlow('error', gt('The requested action could not be performed: Action not found'));*/
     } elseif ($controllerClass->hasMethod('showall')) {
+        //note every invalid command gets converted to 'showall'
         $parms['action'] = 'showall';
         $action = 'showall';
     } else {
         expQueue::flashAndFlow('error', gt('The requested action could not be performed: Action not found'));
     }
@@ -400,43 +401,47 @@ function renderAction(array $parms=array()) {
                 }
             }
         }
     }
 
+    // deal with lower case only to prevent hacking reflection function names
+    $lc_perms = array_change_key_case($perms);
+    $lc_perm_action = strtolower($perm_action);
+    $lc_common_action = strtolower($common_action);
     //FIXME? if the assoc $perm doesn't exist, the 'action' will ALWAYS be allowed, e.g., default is to allow action
-    if (array_key_exists($perm_action, $perms)) {
+    if (array_key_exists($lc_perm_action, $lc_perms)) {
         if (!expPermissions::check($perm_action, $controller->loc)) {
             if (expTheme::inAction()) {
-                flash('error', gt("You don't have permission to")." ".$perms[$perm_action]);
+                flash('error', gt("You don't have permission to")." ".$lc_perms[$lc_perm_action]);
                 notfoundController::handle_not_authorized();
                 expHistory::returnTo('viewable');
             } else {
                 return false;
             }
         }
-    } elseif (array_key_exists($common_action, $perms)) {
+    } elseif (array_key_exists($lc_common_action, $lc_perms)) {
         if (!expPermissions::check($common_action, $controller->loc)) {
             if (expTheme::inAction()) {
-                flash('error', gt("You don't have permission to")." ".$perms[$common_action]);
+                flash('error', gt("You don't have permission to")." ".$lc_perms[$lc_common_action]);
                 notfoundController::handle_not_authorized();
                 expHistory::returnTo('viewable');
             } else {
                 return false;
             }
         }
-    } elseif (array_key_exists($perm_action, $controller->requires_login)) {
+    } elseif (array_key_exists($lc_perm_action, $controller->requires_login)) {
         // check if the action requires the user to at least be logged in
         if (!$user->isLoggedIn()) {
-            $msg = empty($controller->requires_login[$perm_action]) ? gt("You must be logged in to perform this action") : gt($controller->requires_login[$perm_action]);
+            $msg = empty($controller->requires_login[$lc_perm_action]) ? gt("You must be logged in to perform this action") : gt($controller->requires_login[$lc_perm_action]);
             flash('error', $msg);
             notfoundController::handle_not_authorized();
             expHistory::redirecto_login();
         }
-    } elseif (array_key_exists($common_action, $controller->requires_login)) {
-        // check if the action requires the user to at least be logged in
+    } elseif (array_key_exists($lc_common_action, $controller->requires_login)) {
+        // check if the common action requires the user to at least be logged in
         if (!$user->isLoggedIn()) {
-            $msg = empty($controller->requires_login[$common_action]) ? gt("You must be logged in to perform this action") : gt($controller->requires_login[$common_action]);
+            $msg = empty($controller->requires_login[$lc_common_action]) ? gt("You must be logged in to perform this action") : gt($controller->requires_login[$lc_common_action]);
             flash('error', $msg);
             notfoundController::handle_not_authorized();
             expHistory::redirecto_login();
         }
     }
diff --git a/framework/core/subsystems/expString.php b/framework/core/subsystems/expString.php
index 7a9f7f7925..2fa3fcbd2e 100644
--- a/framework/core/subsystems/expString.php
+++ b/framework/core/subsystems/expString.php
@@ -507,12 +507,12 @@ class expString {
 
     /**
      * Scrub input string for possible security issues.
      *
      * @static
-     * @param $data string
-     * @return string
+     * @param $data string|array
+     * @return string|array
      */
     public static function sanitize(&$data) {
 //        return $data;
 
         if (is_array($data)) {
diff --git a/framework/modules/file/controllers/fileController.php b/framework/modules/file/controllers/fileController.php
index f89738c945..53dc53eb14 100755
--- a/framework/modules/file/controllers/fileController.php
+++ b/framework/modules/file/controllers/fileController.php
@@ -33,16 +33,17 @@ class fileController extends expController {
 //    );
     public $requires_login = array(
         'picker'=>'You must be logged in to perform this action',
         'adder'=>'You must be logged in to perform this action',
         'addit'=>'You must be logged in to perform this action',
-        'batchDelete'=>'You must be logged in to perform this action',
-        'createFolder'=>'You must be logged in to perform this action',
+        'batchdelete'=>'You must be logged in to perform this action',
+        'createfolder'=>'You must be logged in to perform this action',
+        'delete'=>'You must be logged in to perform this action',
         'deleter'=>'You must be logged in to perform this action',
         'deleteit'=>'You must be logged in to perform this action',
         'edit'=>'You must be logged in to perform this action',
-        'quickUpload'=>'You must be logged in to perform this action',
+        'quickupload'=>'You must be logged in to perform this action',
         'upload'=>'You must be logged in to perform this action',
         'uploader'=>'You must be logged in to perform this action',
     );
 
     static function displayname() { return gt("File Manager"); }
diff --git a/framework/modules/forms/controllers/formsController.php b/framework/modules/forms/controllers/formsController.php
index 61fa37cc78..d60f374ca1 100644
--- a/framework/modules/forms/controllers/formsController.php
+++ b/framework/modules/forms/controllers/formsController.php
@@ -1498,11 +1498,11 @@ class formsController extends expController {
      * So if we want to make a header row, we just run through once at the beginning and
      * use the array_keys function to strip out a functional header
      *
      * @param      $items
      *
-     * @param null $rptcols
+     * @param array|null $rptcols
      *
      * @return string
      */
     public static function sql2csv($items, $rptcols = null) {
         $str = "";
diff --git a/framework/modules/navigation/controllers/navigationController.php b/framework/modules/navigation/controllers/navigationController.php
index 4db87e64de..90180d6276 100644
--- a/framework/modules/navigation/controllers/navigationController.php
+++ b/framework/modules/navigation/controllers/navigationController.php
@@ -23,16 +23,16 @@ class navigationController extends expController {
     public $basemodel_name = 'section';
     public $useractions = array(
         'showall' => 'Show Navigation',
         'breadcrumb' => 'Breadcrumb',
     );
-    protected $remove_permissions = array(
+//    protected $remove_permissions = array(
 //        'configure',
 //        'create',
 //        'delete',
 //        'edit'
-    );
+//    );
     protected $add_permissions = array(
         'manage'    => 'Manage',
         'view'      => "View Page"
     );
     protected $manage_permissions = array(
diff --git a/framework/modules/pixidou/controllers/pixidouController.php b/framework/modules/pixidou/controllers/pixidouController.php
index 6294e5134b..124c5ba954 100755
--- a/framework/modules/pixidou/controllers/pixidouController.php
+++ b/framework/modules/pixidou/controllers/pixidouController.php
@@ -23,11 +23,11 @@
 
 class pixidouController extends expController {
 	public $cacheDir = "tmp/pixidou/";
     public $requires_login = array(
         'editor'=>'You must be logged in to perform this action',
-        'exitEditor'=>'You must be logged in to perform this action',
+        'exiteditor'=>'You must be logged in to perform this action',
     );
 
     static function displayname() { return gt("Pixidou Image Editor"); }
     static function description() { return gt("Add and manage Exponent Files"); }
     static function author() { return "Phillip Ball - OIC Group, Inc"; }

Vulnerable code:     public $requires_login = array();  // actions/methods which ONLY require user be logged in to access...$permissions take priority
    if (array_key_exists($perm_action, $perms)) {
                flash('error', gt("You don't have permission to")." ".$perms[$perm_action]);
    } elseif (array_key_exists($common_action, $perms)) {
                flash('error', gt("You don't have permission to")." ".$perms[$common_action]);
    } elseif (array_key_exists($perm_action, $controller->requires_login)) {
            $msg = empty($controller->requires_login[$perm_action]) ? gt("You must be logged in to perform this action") : gt($controller->requires_login[$perm_action]);
    } elseif (array_key_exists($common_action, $controller->requires_login)) {
        // check if the action requires the user to at least be logged in
            $msg = empty($controller->requires_login[$common_action]) ? gt("You must be logged in to perform this action") : gt($controller->requires_login[$common_action]);
     * @param $data string
     * @return string
        'batchDelete'=>'You must be logged in to perform this action',
        'createFolder'=>'You must be logged in to perform this action',
        'quickUpload'=>'You must be logged in to perform this action',
     * @param null $rptcols
    protected $remove_permissions = array(
    );
        'exitEditor'=>'You must be logged in to perform this action',
Fixed code:     public $requires_login = array();  // actions/methods (lower case ONLY) which ONLY require user be logged in to access...$permissions take priority
        //note every invalid command gets converted to 'showall'
    // deal with lower case only to prevent hacking reflection function names
    $lc_perms = array_change_key_case($perms);
    $lc_perm_action = strtolower($perm_action);
    $lc_common_action = strtolower($common_action);
    if (array_key_exists($lc_perm_action, $lc_perms)) {
                flash('error', gt("You don't have permission to")." ".$lc_perms[$lc_perm_action]);
    } elseif (array_key_exists($lc_common_action, $lc_perms)) {
                flash('error', gt("You don't have permission to")." ".$lc_perms[$lc_common_action]);
    } elseif (array_key_exists($lc_perm_action, $controller->requires_login)) {
            $msg = empty($controller->requires_login[$lc_perm_action]) ? gt("You must be logged in to perform this action") : gt($controller->requires_login[$lc_perm_action]);
    } elseif (array_key_exists($lc_common_action, $controller->requires_login)) {
        // check if the common action requires the user to at least be logged in
            $msg = empty($controller->requires_login[$lc_common_action]) ? gt("You must be logged in to perform this action") : gt($controller->requires_login[$lc_common_action]);
     * @param $data string|array
     * @return string|array
        'batchdelete'=>'You must be logged in to perform this action',
        'createfolder'=>'You must be logged in to perform this action',
        'delete'=>'You must be logged in to perform this action',
        'quickupload'=>'You must be logged in to perform this action',
     * @param array|null $rptcols
//    protected $remove_permissions = array(
//    );
        'exiteditor'=>'You must be logged in to perform this action',