Skip to content
Permalink
Browse files Browse the repository at this point in the history
fix security vulnerability to bypass permissions using method name in…
… wrong case, reported by fyth
  • Loading branch information
dleffler committed Nov 3, 2016
1 parent 00ff175 commit 684d794
Show file tree
Hide file tree
Showing 7 changed files with 25 additions and 19 deletions.
2 changes: 1 addition & 1 deletion framework/core/controllers/expController.php
Expand Up @@ -53,7 +53,7 @@ abstract class expController {
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
Expand Down
23 changes: 14 additions & 9 deletions framework/core/expFramework.php
Expand Up @@ -277,6 +277,7 @@ function renderAction(array $parms=array()) {
$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 {
Expand Down Expand Up @@ -402,39 +403,43 @@ 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();
Expand Down
4 changes: 2 additions & 2 deletions framework/core/subsystems/expString.php
Expand Up @@ -509,8 +509,8 @@ public static function html2text($val) {
* 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;
Expand Down
7 changes: 4 additions & 3 deletions framework/modules/file/controllers/fileController.php
Expand Up @@ -35,12 +35,13 @@ class fileController extends expController {
'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',
);
Expand Down
2 changes: 1 addition & 1 deletion framework/modules/forms/controllers/formsController.php
Expand Up @@ -1500,7 +1500,7 @@ public function export_csv() {
*
* @param $items
*
* @param null $rptcols
* @param array|null $rptcols
*
* @return string
*/
Expand Down
Expand Up @@ -25,12 +25,12 @@ class navigationController extends expController {
'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"
Expand Down
Expand Up @@ -25,7 +25,7 @@ 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"); }
Expand Down

0 comments on commit 684d794

Please sign in to comment.