Skip to content
Permalink
Browse files Browse the repository at this point in the history
iniitial effort to greatly enhance system security (xss, sql inject, …
…file exploit, rce, etc...)
  • Loading branch information
dleffler committed Sep 28, 2016
1 parent fcec965 commit fdafb5e
Show file tree
Hide file tree
Showing 53 changed files with 4,370 additions and 4,242 deletions.
42 changes: 34 additions & 8 deletions framework/core/controllers/expController.php
Expand Up @@ -41,11 +41,19 @@ abstract class expController {
'create' => 'Create',
'edit' => 'Edit',
'delete' => 'Delete',
'approve' => 'Approval',
);
protected $m_permissions = array( // standard set of actions requiring manage permission for all modules
'activate' => 'Activate',
'approve' => 'Approve',
'merge' => 'Merge',
'rerank' => 'ReRank',
'import' => 'Import Items',
'export' => 'Export Items'
);
protected $remove_permissions = array(); // $permissions not applicable for this module from above list
protected $add_permissions = array(); // additional $permissions processed for this module
public $requires_login = array(); // actions/methods which ONLY require user be logged in to access...$permissions take priority
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 $filepath = ''; // location of this controller's files
public $viewpath = ''; // location of this controllers views; defaults to controller file location
Expand Down Expand Up @@ -131,6 +139,8 @@ public function __construct($src = null, $params = array()) {
$this->config = $config->config;

$this->params = $params;
if (ENABLE_WORKFLOW)
$this->permissions = array_merge($this->permissions, array('approve'=>'Approval'));
}

/**
Expand Down Expand Up @@ -294,7 +304,7 @@ public function showall_by_tags() {
$modelname = $this->basemodel_name;

// get the tag being passed
$tag = new expTag($this->params['tag']);
$tag = new expTag(expString::escape($this->params['tag']));

// find all the id's of the portfolios for this module
$item_ids = $db->selectColumn($modelname, 'id', $this->aggregateWhereClause());
Expand Down Expand Up @@ -328,7 +338,7 @@ public function showall_by_tags() {
assign_to_template(array(
'page' => $page,
'items' => $page->records,
'moduletitle' => ucfirst($modelname) . ' ' . gt('items tagged with') . ' "' . expString::sanitize($this->params['tag']) . '"',
'moduletitle' => ucfirst($modelname) . ' ' . gt('items tagged with') . ' "' . expString::escape($this->params['tag']) . '"',
'rank' => ($order === 'rank') ? 1 : 0
));
}
Expand Down Expand Up @@ -454,7 +464,7 @@ public function show() {
if (isset($this->params['id'])) {
$id = $this->params['id'];
} elseif (isset($this->params['title'])) {
$id = $this->params['title'];
$id = expString::escape($this->params['title']);
}

$record = new $modelname($id);
Expand Down Expand Up @@ -485,6 +495,7 @@ public function showByTitle() {
$modelname = $this->basemodel_name;
// first we'll check to see if this matches the sef_url field...if not then we'll look for the
// title field
$this->params['title'] = expString::escape($this->params['title']); // escape title to prevent sql injection
$record = $this->$modelname->find('first', "sef_url='" . $this->params['title'] . "'");
if (!is_object($record)) {
$record = $this->$modelname->find('first', "title='" . $this->params['title'] . "'");
Expand Down Expand Up @@ -1012,7 +1023,7 @@ public function getRSSContent($limit = 0) {
public function rss() {
require_once(BASE . 'external/feedcreator.class.php');

$id = isset($this->params['title']) ? $this->params['title'] : (isset($this->params['id']) ? $this->params['id'] : null);
$id = isset($this->params['title']) ? expString::escape($this->params['title']) : (isset($this->params['id']) ? $this->params['id'] : null);
if (empty($id)) {
$module = !empty($this->params['module']) ? $this->params['module'] : $this->params['controller'];
$id = array('module' => $module, 'src' => $this->params['src']);
Expand Down Expand Up @@ -1143,7 +1154,7 @@ public function downloadfile() {
}

/**
* permission functions to aggregate a module's permissions based on add/remove permissions
* permission functions to aggregate a module's visible permissions based on add/remove permissions
*
* @return array
*/
Expand All @@ -1157,6 +1168,21 @@ public function permissions() {
return $perms;
}

/**
* permission functions to aggregate a module's permissions based on add/remove and manage permissions
*
* @return array
*/
public function permissions_all() {
//set the permissions array
$perms = array();
foreach ($this->permissions as $perm => $name) {
if (!in_array($perm, $this->remove_permissions)) $perms[$perm] = $name;
}
$perms = array_merge($perms, $this->m_permissions, $this->add_permissions, $this->manage_permissions);
return $perms;
}

// create a psuedo global permission specific to the module; return true grants permission, false continues with other permission checks
public static function checkPermissions($permission, $location) {
return false;
Expand Down
70 changes: 36 additions & 34 deletions framework/core/expFramework.php
Expand Up @@ -285,13 +285,13 @@ function renderAction(array $parms=array()) {

// initialize the controller.
$src = isset($parms['src']) ? $parms['src'] : null;
$controller = new $fullControllerName($src, $parms);
$controller = new $fullControllerName($src, $parms);

//Set up the correct template to use for this action
global $template;
$view = !empty($parms['view']) ? $parms['view'] : $action;
$template = expTemplate::get_template_for_action($controller, $view, $controller->loc);

//setup default model(s) for this controller's actions to use
foreach ($controller->getModels() as $model) {
$controller->$model = new $model(null,false,false); //added null,false,false to reduce unnecessary queries. FJD
Expand Down Expand Up @@ -324,28 +324,30 @@ function renderAction(array $parms=array()) {
}

// check the perms for this action
$perms = $controller->permissions();

$perms = $controller->permissions_all();

$common_action = null;
// action convention for controllers that manage more than one model (datatype).
// if you preface the name action name with a common crud action name we can check perms on
// it with the developer needing to specify any...better safe than sorry.
// i.e if the action is edit_mymodel it will be checked against the edit permission
if (stristr($parms['action'], '_'))
$parts = explode("_", $parms['action']);
else
$parts = preg_split('/(?=[A-Z])/', $parms['action']); // account for actions with camelCase action/perm such as editItem
$common_action = isset($parts[0]) ? $parts[0] : null;
// we have to treat the update permission a little different..it's tied to the create/edit
// permissions. Really the only way this will fail will be if someone bypasses the perm check
// on the edit form somehow..like a hacker trying to bypass the form and just submit straight to
// the action. To safeguard, we'll catch if the action is update and change it either to create or
// edit depending on whether an id param is passed to. that should be sufficient.
$common_action = null;
//FIXME do we also need to account for actions with camelcase action/perm such as editItem ???
if ($parms['action'] == 'update') {
if ($parms['action'] == 'update' || $common_action == 'update') {
$perm_action = (!isset($parms['id']) || $parms['id'] == 0) ? 'create' : 'edit';
} elseif ($parms['action'] == 'edit' && (!isset($parms['id']) || $parms['id'] == 0)) {
} elseif (($parms['action'] == 'edit' || $common_action == 'edit') && (!isset($parms['id']) || $parms['id'] == 0)) {
$perm_action = 'create';
} elseif ($parms['action'] == 'saveconfig') {
$perm_action = 'configure';
} else {
// action convention for controllers that manage more than one model (datatype).
// if you preface the name action name with a common crud action name we can check perms on
// it with the developer needing to specify any...better safe than sorry.
// i.e if the action is edit_mymodel it will be checked against the edit permission
if (stristr($parms['action'], '_')) $parts = explode("_", $parms['action']);
$common_action = isset($parts[0]) ? $parts[0] : null;
$perm_action = $parms['action'];
}

Expand Down Expand Up @@ -424,24 +426,24 @@ function renderAction(array $parms=array()) {
} elseif (array_key_exists($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") : $controller->requires_login[$perm_action];
$msg = empty($controller->requires_login[$perm_action]) ? gt("You must be logged in to perform this action") : gt($controller->requires_login[$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
if (!$user->isLoggedIn()) {
$msg = empty($controller->requires_login[$common_action]) ? gt("You must be logged in to perform this action") : $controller->requires_login[$common_action];
$msg = empty($controller->requires_login[$common_action]) ? gt("You must be logged in to perform this action") : gt($controller->requires_login[$common_action]);
flash('error', $msg);
notfoundController::handle_not_authorized();
expHistory::redirecto_login();
}
}
}

// register this controllers permissions to the view for in view perm checks
$template->register_permissions(array_keys($perms), $controller->loc);

// globalizing $user inside all templates
$template->assign('user', $user);

Expand Down Expand Up @@ -486,7 +488,7 @@ function redirect_to($params=array(), $secure=false) {
$link = (!is_array($params)) ? $params : $router->makeLink($params, false, $secure);
header("Location: " . $link);
exit();
}
}

function flash($name, $msg) {
expQueue::flash($name, $msg);
Expand Down Expand Up @@ -514,7 +516,7 @@ function show_msg_queue($name=null) {
*/
function assign_to_template(array $vars=array()) {
global $template;

if (empty($template) || count($vars) == 0) return false;
foreach ($vars as $key=>$val) {
$template->assign($key, $val);
Expand All @@ -540,7 +542,7 @@ function get_common_template($view, $loc, $controllername='') {
$controller = new stdClass();
$controller->baseclassname = empty($controllername) ? 'common' : $controllername;
$controller->loc = $loc;

$themepath = BASE . 'themes/' . DISPLAY_THEME . '/modules/common/views/' . $controllername . '/' . $view . '.tpl';
$basenewuipath = BASE . 'framework/modules/common/views/' . $controllername . '/' . $view . '.newui.tpl';
$basepath = BASE . 'framework/modules/common/views/' . $controllername . '/' . $view . '.tpl';
Expand Down Expand Up @@ -581,19 +583,19 @@ function get_config_templates($controller, $loc) {
return expTemplate::get_config_templates($controller, $loc);

// global $db;

// set paths we will search in for the view
$commonpaths = array(
BASE.'framework/modules/common/views/configure',
BASE.'themes/'.DISPLAY_THEME.'/modules/common/views/configure',
);

$modpaths = array(
$controller->viewpath.'/configure',
BASE.'themes/'.DISPLAY_THEME.'/modules/'.$controller->relative_viewpath.'/configure'
);
// get the common configuration files

// get the common configuration files
$common_views = expTemplate::find_config_views($commonpaths, $controller->remove_configs);
foreach ($common_views as $key=>$value) {
$common_views[$key]['name'] = gt($value['name']);
Expand All @@ -608,7 +610,7 @@ function get_config_templates($controller, $loc) {
$module_views[$key]['name'] = gt($value['name']);
}

// look for a config form for this module's current view
// look for a config form for this module's current view
// $controller->loc->mod = expModules::getControllerClassName($controller->loc->mod);
//check to see if hcview was passed along, indicating a hard-coded module
// if (!empty($controller->params['hcview'])) {
Expand All @@ -625,7 +627,7 @@ function get_config_templates($controller, $loc) {
// $module_views[$viewname]['file'] =$path.'/'.$viewconfig;
// }
// }

// sort the views highest to lowest by filename
// we are reverse sorting now so our array merge
// will overwrite property..we will run array_reverse
Expand Down Expand Up @@ -671,7 +673,7 @@ function find_config_views($paths=array(), $excludes=array()) {
}
}
}

return $views;
}

Expand Down Expand Up @@ -759,12 +761,12 @@ function get_action_views($ctl, $action, $human_readable) {
// $controller = new $controllerName();
$controller = expModules::getController($ctl);

// set path information
// set path information
$paths = array(
$controller->viewpath,
BASE.'themes/'.DISPLAY_THEME.'/modules/'.$controller->relative_viewpath,
);

$views = array();
foreach ($paths as $path) {
if (is_readable($path)) {
Expand Down Expand Up @@ -804,7 +806,7 @@ function get_filedisplay_views() {
BASE.'framework/modules/common/views/file/',
BASE.'themes/'.DISPLAY_THEME.'modules/common/views/file/',
);

$views = array();
foreach ($paths as $path) {
if (is_readable($path)) {
Expand All @@ -817,7 +819,7 @@ function get_filedisplay_views() {
}
}
}

return $views;
}

Expand Down
21 changes: 13 additions & 8 deletions framework/core/models/expRecord.php
Expand Up @@ -104,7 +104,7 @@ function __construct($params = null, $get_assoc = true, $get_attached = true) {
$needs_approval = $this->needs_approval && ENABLE_WORKFLOW;

// if the user passed in arguments to this constructor then we need to
// retrieve objects
// retrieve objects

// If a number was sent in, we assume this is a DB record ID, so pull it
if (!is_object($params) && !is_array($params)) {
Expand All @@ -122,7 +122,7 @@ function __construct($params = null, $get_assoc = true, $get_attached = true) {
$params = array('title'=> $params);
}
} else {
// Otherwise we assume that in inbound is an array or Object to be processed as is.
// Otherwise we assume that in inbound is an array or Object to be processed as is.
$this->build($params);
}

Expand All @@ -135,7 +135,7 @@ function __construct($params = null, $get_assoc = true, $get_attached = true) {
$this->publish_date = $this->created_at;
}

// setup the exception array if it's not there. This array tells the getAssociatedObjectsForThisModel() function which
// setup the exception array if it's not there. This array tells the getAssociatedObjectsForThisModel() function which
// modules NOT to setup. This stops us from getting infinite loops with many to many relationships.
if (is_array($params)){
$params['except'] = isset($params['except']) ? $params['except'] : array();
Expand Down Expand Up @@ -180,6 +180,11 @@ public function find($range = 'all', $where = null, $order = null, $limit = null
//eDebug("Supports Revisions:" . $this->supports_revisions);
// if ($this->supports_revisions && $range != 'revisions') $sql .= " AND revision_id=(SELECT MAX(revision_id) FROM `" . $db->prefix . $this->tablename . "` WHERE $where)";
// $sql .= empty($order) ? '' : ' ORDER BY ' . $order;
$order = expString::escape($order);
if ($limit !== null)
$limit = intval($limit);
if ($limitstart !== null)
$limitstart = intval($limitstart);
$supports_revisions = $this->supports_revisions && ENABLE_WORKFLOW;
if (ENABLE_WORKFLOW && $this->needs_approval) {
$needs_approval = $user->id;
Expand Down Expand Up @@ -361,10 +366,10 @@ public function build($params = array()) {
foreach ($table as $col=> $colDef) {
// check if the DB column has a corresponding value in the params array
// if not, we check to see if the column is boolean...if so we set it to false
// if not, then we check to see if we had a previous value in this particular
// if not, then we check to see if we had a previous value in this particular
// record. if so we reset it to itself so we don't lose the existing value.
// this is good for when the developer is trying to update just a field or two
// in an existing record.
// this is good for when the developer is trying to update just a field or two
// in an existing record.
if (array_key_exists($col, $params)) {
$value = is_array($params) ? $params[$col] : $params->$col;
if ($colDef[0] == DB_DEF_INTEGER || $colDef[0] == DB_DEF_ID) {
Expand Down Expand Up @@ -464,7 +469,7 @@ public function save($validate = false, $force_no_revisions = false) {

// Save this object's associated objects to the database.
// FIXME: we're not going to do this automagically until we get the refreshing figured out.
//$this->saveAssociatedObjects();
//$this->saveAssociatedObjects();

//Only grab fields that are valid and save this object
$saveObj = new stdClass();
Expand Down Expand Up @@ -534,7 +539,7 @@ public function validate() {
}

// safeguard again loc data not being pass via forms...sometimes this happens when you're in a router
// mapped view and src hasn't been passed in via link to the form
// mapped view and src hasn't been passed in via link to the form
if (isset($this->id) && empty($this->location_data)) {
$loc = $db->selectValue($this->tablename, 'location_data', 'id=' . $this->id);
if (!empty($loc)) $this->location_data = $loc;
Expand Down

0 comments on commit fdafb5e

Please sign in to comment.