Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
security fix several sql injection vulnerabilities reported by Manuel…
… Garcia Cardenas and PKAV TEAM
  • Loading branch information
dleffler committed Sep 13, 2016
1 parent 26cdc39 commit e916702
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 33 deletions.
25 changes: 18 additions & 7 deletions framework/core/subsystems/database/mysqli.php
Expand Up @@ -379,6 +379,8 @@ function columnUpdate($table, $col, $val, $where=1) {
function selectObjects($table, $where = null, $orderby = null) {
if ($where == null)
$where = "1";
else
$where = $this->injectProof($where);
if ($orderby == null)
$orderby = '';
else
Expand Down Expand Up @@ -484,7 +486,7 @@ function selectObjectBySql($sql) {
//$lfh = fopen($logFile, 'a');
//fwrite($lfh, $sql . "\n");
//fclose($lfh);
$res = @mysqli_query($this->connection, $sql);
$res = @mysqli_query($this->connection, $this->injectProof($sql));
if ($res == null)
return null;
return mysqli_fetch_object($res);
Expand All @@ -497,7 +499,7 @@ function selectObjectBySql($sql) {
* @return array
*/
function selectObjectsBySql($sql) {
$res = @mysqli_query($this->connection, $sql);
$res = @mysqli_query($this->connection, $this->injectProof($sql));
if ($res == null)
return array();
$objects = array();
Expand Down Expand Up @@ -638,6 +640,8 @@ function selectValueBySql($sql) {
function selectObjectsIndexedArray($table, $where = null, $orderby = null) {
if ($where == null)
$where = "1";
else
$where = $this->injectProof($where);
if ($orderby == null)
$orderby = '';
else
Expand Down Expand Up @@ -722,6 +726,7 @@ function queryRows($sql) {
* @return object/null|void
*/
function selectObject($table, $where) {
$where = $this->injectProof($where);
$res = mysqli_query($this->connection, "SELECT * FROM `" . $this->prefix . "$table` WHERE $where LIMIT 0,1");
if ($res == null)
return null;
Expand Down Expand Up @@ -773,7 +778,7 @@ function insertObject($object, $table) {
if ($values != ") VALUES (") {
$values .= ",";
}
$values .= "'" . mysqli_real_escape_string($this->connection, $val) . "'";
$values .= "'" . $this->escapeString($val) . "'";
}
}
$sql = substr($sql, 0, -1) . substr($values, 0) . ")";
Expand Down Expand Up @@ -836,13 +841,13 @@ function updateObject($object, $table, $where=null, $identifier='id', $is_revisi
$val = serialize($val);
$sql .= "`$var`='".$val."',";
} else {
$sql .= "`$var`='".mysqli_real_escape_string($this->connection,$val)."',";
$sql .= "`$var`='" . $this->escapeString($val) . "',";
}
}
}
$sql = substr($sql, 0, -1) . " WHERE ";
if ($where != null)
$sql .= $where;
$sql .= $this->injectProof($where);
else
$sql .= "`" . $identifier . "`=" . $object->$identifier;
//if ($table == 'text') eDebug($sql,true);
Expand Down Expand Up @@ -1130,6 +1135,8 @@ function escapeString($string) {
function selectArrays($table, $where = null, $orderby = null) {
if ($where == null)
$where = "1";
else
$where = $this->injectProof($where);
if ($orderby == null)
$orderby = '';
else
Expand All @@ -1156,7 +1163,7 @@ function selectArrays($table, $where = null, $orderby = null) {
* @return array
*/
function selectArraysBySql($sql) {
$res = @mysqli_query($this->connection, $sql);
$res = @mysqli_query($this->connection, $this->injectProof($sql));
if ($res == null)
return array();
$arrays = array();
Expand Down Expand Up @@ -1185,6 +1192,8 @@ function selectArraysBySql($sql) {
function selectArray($table, $where = null, $orderby = null, $is_revisioned=false, $needs_approval=false) {
if ($where == null)
$where = "1";
else
$where = $this->injectProof($where);
$as = '';
if ($is_revisioned) {
// $where.= " AND revision_id=(SELECT MAX(revision_id) FROM `" . $this->prefix . "$table` WHERE $where)";
Expand Down Expand Up @@ -1223,6 +1232,8 @@ function selectArray($table, $where = null, $orderby = null, $is_revisioned=fals
function selectExpObjects($table, $where=null, $classname, $get_assoc=true, $get_attached=true, $except=array(), $cascade_except=false, $order=null, $limitsql=null, $is_revisioned=false, $needs_approval=false) {
if ($where == null)
$where = "1";
else
$where = $this->injectProof($where);
$as = '';
if ($is_revisioned) {
// $where.= " AND revision_id=(SELECT MAX(revision_id) FROM `" . $this->prefix . "$table` WHERE $where)";
Expand Down Expand Up @@ -1259,7 +1270,7 @@ function selectExpObjects($table, $where=null, $classname, $get_assoc=true, $get
* @return array
*/
function selectExpObjectsBySql($sql, $classname, $get_assoc=true, $get_attached=true) {
$res = @mysqli_query($this->connection, $sql);
$res = @mysqli_query($this->connection, $this->injectProof($sql));
if ($res == null)
return array();
$arrays = array();
Expand Down
15 changes: 15 additions & 0 deletions framework/core/subsystems/expDatabase.php
Expand Up @@ -1105,6 +1105,21 @@ abstract function inError();
*/
abstract function escapeString($string);

/**
* Unescape a string based on the database connection
* @param $string
* @return string
*/
function injectProof($string) {
$quotes = substr_count("'", $string);
if ($quotes % 2 != 0)
$string = $this->escapeString($string);
$dquotes = substr_count('"', $string);
if ($dquotes % 2 != 0)
$string = $this->escapeString($string);
return $string;
}

/**
* Create a SQL "limit" phrase
*
Expand Down
29 changes: 22 additions & 7 deletions framework/core/subsystems/expRouter.php
Expand Up @@ -198,11 +198,26 @@ public function routeRequest() {
);
}
}
// conventional method to ensure the 'id' is an id
if (isset($_GET['id'])) {
$_GET['id'] = intval($_GET['id']);
// conventional method to ensure the 'id' is only an id
if (isset($_REQUEST['id'])) {
if (isset($_GET['id']))
$_GET['id'] = intval($_GET['id']);
if (isset($_POST['id']))
$_POST['id'] = intval($_POST['id']);

$_REQUEST['id'] = intval($_REQUEST['id']);
}
// do the same for the other id's
foreach ($_REQUEST as $key=>$var) {
if (is_string($var) && strrpos($key,'_id',-3) !== false) {
if (isset($_GET[$key]))
$_GET[$key] = intval($_GET[$key]);
if (isset($_POST[$key]))
$_POST[$key] = intval($_POST[$key]);

$_REQUEST[$key] = intval($_REQUEST[$key]);
}
}
if (empty($user->id) || (!empty($user->id) && !$user->isAdmin())) { //FIXME why would $user be empty here unless $db is down?
// $_REQUEST['route_sanitized'] = true;//FIXME debug test
expString::sanitize($_REQUEST); // strip other exploits like sql injections
Expand Down Expand Up @@ -307,14 +322,14 @@ public function splitURL() {

if (count($this->url_parts) < 1 || (empty($this->url_parts[0]) && count($this->url_parts) == 1) ) {
$this->url_type = 'base'; // no params
} elseif (count($this->url_parts) == 1 || $db->selectObject('section', "sef_name='" . substr($db->escapeString($this->sefPath),1) . "'") != null) {
} elseif (count($this->url_parts) == 1 || $db->selectObject('section', "sef_name='" . substr($this->sefPath,1) . "'") != null) {
$this->url_type = 'page'; // single param is page name
} elseif ($_SERVER['REQUEST_METHOD'] == 'POST') {
$this->url_type = 'post'; // params via form/post
} else {
// take a peek and see if a page exists with the same name as the first value...if so we probably have a page with
// extra perms...like printerfriendly=1 or ajax_action=1;
if (($db->selectObject('section', "sef_name='" . $db->escapeString($this->url_parts[0]) . "'") != null) && (in_array(array('printerfriendly','exportaspdf','ajax_action'), $this->url_parts))) {
if (($db->selectObject('section', "sef_name='" . $this->url_parts[0] . "'") != null) && (in_array(array('printerfriendly','exportaspdf','ajax_action'), $this->url_parts))) {
$this->url_type = 'page';
} else {
$this->url_type = 'action';
Expand Down Expand Up @@ -547,7 +562,7 @@ public function buildCurrentUrl() {
} else {
$url .= urldecode((empty($_SERVER['REQUEST_URI'])) ? $_ENV['REQUEST_URI'] : $_SERVER['REQUEST_URI']);
}
return expString::sanitize($url);
return expString::escape(expString::sanitize($url));
}

public static function encode($url) {
Expand Down Expand Up @@ -760,7 +775,7 @@ private function buildSEFPath () {
if (substr($this->sefPath,-1) == "/") $this->sefPath = substr($this->sefPath,0,-1);
// sanitize it
$sefPath = explode('">',$this->sefPath); // remove any attempts to close the command
$this->sefPath = expString::sanitize($sefPath[0]);
$this->sefPath = expString::escape(expString::sanitize($sefPath[0]));
}

public function getSection() {
Expand Down
34 changes: 17 additions & 17 deletions framework/core/subsystems/expString.php
Expand Up @@ -522,23 +522,23 @@ public static function sanitize(&$data) {
$data = self::xss_clean($data);

//fixme orig exp method
if(0) {
// remove whitespaces and tags
// $data = strip_tags(trim($data));
// remove whitespaces and script tags
$data = self::strip_tags_content(trim($data), '<script>', true);
// $data = self::strip_tags_content(trim($data), '<iframe>', true);

// apply stripslashes if magic_quotes_gpc is enabled
if (get_magic_quotes_gpc()) {
$data = stripslashes($data);
}

$data = self::escape($data);

// re-escape newlines
$data = str_replace(array('\r', '\n'), array("\r", "\n"), $data);
}
// if(0) {
// // remove whitespaces and tags
//// $data = strip_tags(trim($data));
// // remove whitespaces and script tags
// $data = self::strip_tags_content(trim($data), '<script>', true);
//// $data = self::strip_tags_content(trim($data), '<iframe>', true);
//
// // apply stripslashes if magic_quotes_gpc is enabled
// if (get_magic_quotes_gpc()) {
// $data = stripslashes($data);
// }
//
// $data = self::escape($data);
//
// // re-escape newlines
// $data = str_replace(array('\r', '\n'), array("\r", "\n"), $data);
// }
}
return $data;
}
Expand Down
5 changes: 3 additions & 2 deletions framework/core/subsystems/expTheme.php
Expand Up @@ -762,7 +762,7 @@ public static function runAction()
if (!$user->isAdmin())
expString::sanitize($_REQUEST);
// } elseif (empty($_REQUEST['array_sanitized'])) {
$tmp =1; //FIXME we've already sanitized at this point
// $tmp =1; //FIXME we've already sanitized at this point
// } else {
// $tmp =1; //FIXME we've already sanitized at this point
// }
Expand Down Expand Up @@ -843,7 +843,8 @@ public static function showAction($module, $action, $src = "", $params = array()
//// $_GET[$key] = $value;
// $_GET[$key] = expString::sanitize($value);
// }
if (!$user->isAdmin()) expString::sanitize($_GET);
if (!$user->isAdmin())
expString::sanitize($_GET);
}
//if (isset($['_common'])) $actfile = "/common/actions/" . $_REQUEST['action'] . ".php";

Expand Down

0 comments on commit e916702

Please sign in to comment.