CVE-2016-9283: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWE-89)
value: SQL Injection in framework/core/subsystems/expRouter.php in Exponent CMS v2.4.0 allows remote attackers to read database information via address/addContentToSearch/id/ and a trailing string, related to a "sef URL" issue.
CWE description: The product constructs all or part of an SQL command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended SQL command when it is sent to a downstream component. Without sufficient removal or quoting of SQL syntax in user-controllable inputs, the generated SQL query can cause those inputs to be interpreted as SQL instead of ordinary user data.
CVSS v3 base score: 7.5
Published: 2016-11-11T22:59Z
Repository: https://github.com/exponentcms/exponent-cms
Language: PHP
Files changed: framework/core/subsystems/expRouter.php
Commit date: 2016-11-03 21:34:43 -0400
Commit message: fix sql injection security vulnerability which failed to account for sef urls adequately; reported by many, many users

Diff: diff --git a/framework/core/subsystems/expRouter.php b/framework/core/subsystems/expRouter.php
index 88d794bf670..5600779a5d4 100644
--- a/framework/core/subsystems/expRouter.php
+++ b/framework/core/subsystems/expRouter.php
@@ -36,13 +36,13 @@ class expRouter {
     /**
      * Style of url
      * either 'sef' or 'query'
      * @var string
      */
-    public  $url_style = '';
+    private $url_style = '';
     public  $params = array();
-    public  $sefPath = null;
+    private $sefPath = null;
 
     function __construct() {
         self::getRouterMaps();
     }
 
@@ -187,48 +187,54 @@ class expRouter {
     }
 
     public function routeRequest() {
         global $user;
 
-        // strip out possible xss exploits via url
+        // start splitting the URL into it's different parts
+        $this->splitURL();
+        // edebug($this,1);
+
+        // strip out possible xss exploits via old school url
         foreach ($_GET as $key=>$var) {
             if (is_string($var) && strpos($var,'">')) {
                 unset(
                     $_GET[$key],
                     $_REQUEST[$key]
                 );
             }
         }
+        //fixme only old school url and forms have these variables here
         // conventional method to ensure the 'id' is only an id
         if (isset($_REQUEST['id'])) {
+            $_REQUEST['id'] = intval($_REQUEST['id']);
             if (isset($_GET['id']))
-                $_GET['id'] = intval($_GET['id']);
+                $_GET['id'] = $_REQUEST['id'];
             if (isset($_POST['id']))
-                $_POST['id'] = intval($_POST['id']);
-
-            $_REQUEST['id'] = intval($_REQUEST['id']);
+                $_POST['id'] = $_REQUEST['id'];
         }
         // do the same for the other id's
         foreach ($_REQUEST as $key=>$var) {
             if (is_string($var) && strlen($key) >= 3 && strrpos($key,'_id',-3) !== false) {
+                $_REQUEST[$key] = intval($_REQUEST[$key]);
                 if (isset($_GET[$key]))
-                    $_GET[$key] = intval($_GET[$key]);
+                    $_GET[$key] = $_REQUEST[$key];
                 if (isset($_POST[$key]))
-                    $_POST[$key] = intval($_POST[$key]);
-
-                $_REQUEST[$key] = intval($_REQUEST[$key]);
+                    $_POST[$key] = $_REQUEST[$key];
+            }
+            if ($key == 'src') {
+                $_REQUEST[$key] = preg_replace("/[^A-Za-z0-9@-]/", '', $_REQUEST[$key]);
+                if (isset($_GET[$key]))
+                    $_GET[$key] = $_REQUEST[$key];
+                if (isset($_POST[$key]))
+                    $_POST[$key] = $_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
         }
 
-        // start splitting the URL into it's different parts
-        $this->splitURL();
-        // edebug($this,1);
-
         if ($this->url_style == 'sef') {
             if ($this->url_type == 'page' || $this->url_type == 'base') {
                 $ret = $this->routePageRequest();               // if we hit this the formatting of the URL looks like the user is trying to go to a page.
                 if (!$ret) $this->url_type = 'malformed';
             } elseif ($this->url_type == 'action') {
@@ -246,12 +252,11 @@ class expRouter {
                 // a module in the form.
                 if (!empty($_POST['controller']) || !empty($_POST['module'])) {
                     $module = !empty($_POST['controller']) ? expString::sanitize($_POST['controller']) : expString::sanitize($_POST['module']);
                     // Figure out if this is module or controller request - WE ONLY NEED THIS CODE UNTIL WE PULL OUT THE OLD MODULES
                     if (expModules::controllerExists($module)) {
-                        $_POST['controller'] = $module;
-                        $_REQUEST['controller'] = $module;
+                        $_POST['controller'] = $_REQUEST['controller'] = $module;
                     }
                 }
             }
         } elseif ($this->url_style == 'query' && SEF_URLS == 1 && !empty($_REQUEST['section']) && PRINTER_FRIENDLY != 1 && EXPORT_AS_PDF != 1) {
             // if we hit this it's an old school url coming in and we're trying to use SEF's.
@@ -303,10 +308,13 @@ class expRouter {
         $trackingObject->user_agent = $_SERVER['HTTP_USER_AGENT'];
         $trackingObject->session_id = $_COOKIE['PHPSESSID'];
         $db->insertObject($trackingObject,'tracking_rawdata');
     }
 
+    /**
+     * Assign url_type & url_style
+     */
     public function splitURL() {
         global $db;
 
         $this->url_parts = array();
         $this->buildSEFPath();
@@ -315,31 +323,35 @@ class expRouter {
             $this->url_style = 'sef';
             $this->url_parts = explode('/', $this->sefPath);
 
             // remove empty first and last url_parts if they exist
             //if (empty($this->url_parts[count($this->url_parts)-1])) array_pop($this->url_parts);
-            if ($this->url_parts[count($this->url_parts)-1] == '') array_pop($this->url_parts);
-            if (empty($this->url_parts[0])) array_shift($this->url_parts);
+            if ($this->url_parts[count($this->url_parts)-1] == '')
+                array_pop($this->url_parts);
+            if (empty($this->url_parts[0]))
+                array_shift($this->url_parts);
+            else
+                $this->url_parts[0] = expString::escape($this->url_parts[0]);
 
             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($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='" . $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';
                 }
             }
             $this->params = $this->convertPartsToParams();
         } elseif ($_SERVER['REQUEST_METHOD'] == 'POST') {
-            $this->url_style = 'sef';
+            $this->url_style = 'sef';  // even if it's old school, they all come in the same
             $this->url_type = 'post';
             $this->params = $this->convertPartsToParams();
         } elseif (isset($_SERVER['REQUEST_URI'])) {
             // if we hit here, we don't really need to do much.  All the pertinent info will come thru in the POST/GET vars
             // so we don't really need to worry about what the URL looks like.
@@ -348,10 +360,11 @@ class expRouter {
                 $this->params = array();
             } else {
                 $sefPath = explode('%22%3E',$_SERVER['REQUEST_URI']);  // remove any attempts to close the command
                 $_SERVER['REQUEST_URI'] = $sefPath[0];
                 $this->url_style = 'query';
+                //note 'query' doesn't need $params
             }
         } else {
             $this->url_type = 'base';
             $this->params = array();
         }
@@ -360,10 +373,15 @@ class expRouter {
         define('PRINTER_FRIENDLY', (isset($_REQUEST['printerfriendly']) || isset($this->params['printerfriendly'])) ? 1 : 0);
         define('EXPORT_AS_PDF', (isset($_REQUEST['exportaspdf']) || isset($this->params['exportaspdf'])) ? 1 : 0);
         define('EXPORT_AS_PDF_LANDSCAPE', (isset($_REQUEST['landscapepdf']) || isset($this->params['landscapepdf'])) ? 1 : 0);
     }
 
+    /**
+     * Set up for page request, but check store category/product also
+     *
+     * @return bool
+     */
     public function routePageRequest() {
 //        global $db;
 
         if ($this->url_type == 'base') {
             // if we made it in here this is a request for http://www.baseurl.com
@@ -428,11 +446,11 @@ class expRouter {
                     //eDebug($this->url_parts,true);
                     $this->params = $this->convertPartsToParams();
                     return $this->routeActionRequest();
                 }
 //fixme we may want to log missed pages (no existing store cat/product) requests and set up/use a redirect table (404)??
-//fixme and we may also want to log any redirects taken??
+//fixme and we may also want to log any redirects taken from that table??
                 return false;
             }
             #########################################################
             //if (empty($section)) return false;  //couldnt find the page..let the calling action deal with it.
             $_REQUEST['section'] = $section->id;
@@ -491,18 +509,24 @@ class expRouter {
                         $this->url_parts[] = $key;
                         $this->url_parts[] = $value;
                     }
                 }
 
-                $this->params = $this->convertPartsToParams();
+                $this->params = $this->convertPartsToParams(); // update params to new re-mapped url_parts
+                //fixme do we need to re-sanitize them???
                 return true;
             }
         }
 
         return false;
     }
 
+    /**
+     * Check and set up for an action request
+     *
+     * @return bool
+     */
     public function routeActionRequest() {
         $return_params = array('controller'=>'','action'=>'','url_parts'=>array());
 
         // If we have three url parts we assume they are controller->action->id, otherwise split them out into name<=>value pairs
         $return_params['controller'] = $this->url_parts[0]; // set the controller/module
@@ -518,52 +542,55 @@ class expRouter {
         }
 
         // now figure out the name<=>value pairs
         if (count($this->url_parts) == 3) {
             if ( is_numeric($this->url_parts[2])) {
-                $return_params['url_parts']['id'] = $this->url_parts[2];
+                $return_params['url_parts']['id'] = intval($this->url_parts[2]);
             }
         } else {
             for ($i = 2, $iMax = count($this->url_parts); $i < $iMax; $i++) {
                 if ($i % 2 == 0) {
                     $return_params['url_parts'][$this->url_parts[$i]] = isset($this->url_parts[$i+1]) ? $this->url_parts[$i+1] : '';
                 }
             }
         }
 
-        // Set the module or controller - this how the actual routing happens
-        $_REQUEST[$requestType] = $return_params['controller']; //url_parts[0];
-        $_GET[$requestType] = $return_params['controller'];
-        $_POST[$requestType] = $return_params['controller'];
+        // Set the controller - this how the actual routing happens
+        $_REQUEST[$requestType] = $_GET[$requestType] = $_POST[$requestType] = $return_params['controller'];
 
         // Set the action for this module or controller
         if ($_SERVER['REQUEST_METHOD'] == 'POST') {
             // most of the time we can just grab the action outta the POST array since this is passed as a hidden field,
             // but sometimes it is actually set as the action on the form itself...then we get it from the params array instead.
             $action = !empty($_POST['action']) ? $_POST['action'] : $this->params['action'];
         } else {
             $action = $return_params['action'];
         }
 
-        $_REQUEST['action'] = $action;
-        $_GET['action'] = $action;
-        $_POST['action'] = $action;
+        $_REQUEST['action'] = $_GET['action'] = $_POST['action'] = $action;
 
-        // pass off the name<=>value pairs
+        // pass off the name<=>value pairs for old school url
         foreach($return_params['url_parts'] as $key=>$value) {
-            $save_value = expString::sanitize($value);
-            $_REQUEST[$key] = $save_value;
-            $_GET[$key] = $save_value;
+            $_REQUEST[$key] = $_GET[$key] = expString::sanitize($value);
         }
 
         return true;
     }
 
     public function buildCurrentUrl() {
         $url =  URL_BASE;
         if ($this->url_style == 'sef') {
-            $url .= substr(PATH_RELATIVE,0,-1).$this->sefPath;
+            if (count($this->params) > 2) {
+                $url .= substr(PATH_RELATIVE,0,-1).'/'.$this->params['controller'].'/'.$this->params['action'];
+                foreach ($this->params as $key=>$value) {
+                    if ($key != 'controller' && $key != 'action') {
+                        $url .= '/' . $key . '/' . $value;
+                    }
+                }
+            } else {
+                $url .= substr(PATH_RELATIVE,0,-1).$this->sefPath;  //fixme do we need to clean this up?
+            }
         } else {
             $url .= urldecode((empty($_SERVER['REQUEST_URI'])) ? $_ENV['REQUEST_URI'] : $_SERVER['REQUEST_URI']);
         }
         return expString::escape(expString::sanitize($url));
     }
@@ -663,10 +690,15 @@ class expRouter {
     public function convertToOldSchoolUrl() {
         $params = $this->convertPartsToParams();
         return $this->makeLink($params, true);
     }
 
+    /**
+     * Convert url_parts() or $_REQUEST to $params
+     *
+     * @return array|string
+     */
     public function convertPartsToParams() {
         $params = array();
         if ($this->url_type == 'base') {
             $params['section'] = SITE_DEFAULT_SECTION;
         } elseif ($this->url_type == 'page') {
@@ -679,11 +711,12 @@ class expRouter {
                 if ($i % 2 == 0) {
                     $params[$this->url_parts[$i]] = isset($this->url_parts[$i+1]) ? $this->url_parts[$i+1] : '';
                 }
             }
         } elseif ($this->url_type == 'post') {
-            if (isset($_REQUEST['PHPSESSID'])) unset($_REQUEST['PHPSESSID']);
+            if (isset($_REQUEST['PHPSESSID']))
+                unset($_REQUEST['PHPSESSID']);
 //            foreach($_REQUEST as $name=>$val) {
 ////                if (get_magic_quotes_gpc()) $val = stripslashes($val);  // magic quotes fix??
 ////                $params[$name] = $val;
 //                $params[$name] = expString::sanitize($val);  //FIXME need array sanitizer
 //            }
@@ -691,13 +724,32 @@ class expRouter {
                 $params = expString::sanitize($_REQUEST);
 //            if (empty($data['route_sanitized'])) $_REQUEST['pre_sanitized'] = true;//FIXME debug test
         }
         //TODO: fully sanitize all params values here for ---We already do this!
 //        if (isset($params['src'])) $params['src'] = expString::sanitize(htmlspecialchars($params['src']));
+        // conventional method to ensure the 'id' is only an id
+        if (isset($params['id'])) {
+            $params['id'] = intval($params['id']);
+        }
+        // do the same for the other id's
+        foreach ($params as $key=>$var) {
+            if (is_string($var) && strlen($key) >= 3 && strrpos($key,'_id',-3) !== false) {
+                $params[$key] = intval($params[$key]);
+            }
+            if ($key == 'src') {
+                $params[$key] = preg_replace("/[^A-Za-z0-9@-]/", '', $params[$key]);
+            }
+        }
         return $params;
     }
 
+    /**
+     * Attempt to locate a page by name or id
+     *
+     * @param $url_name
+     * @return null|object|void
+     */
     public function getPageByName($url_name) {
         global $db;
 
         $section = null;
         if (is_numeric($url_name)) {
@@ -719,10 +771,14 @@ class expRouter {
 #           return $section;
 #       }
         return $section;
     }
 
+    /**
+     * Get the SEF URL from the server
+     * if we got an old school url, it will only contain the 'index.php'
+     */
     private function buildSEFPath () {
         // Apache
         if (strpos($_SERVER['SERVER_SOFTWARE'],'Apache') !== false || strpos($_SERVER['SERVER_SOFTWARE'],'WebServerX') !== false) {
             switch(php_sapi_name()) {
                 case "cgi":
@@ -773,11 +829,12 @@ class expRouter {
                 {
                     $this->ectid = $s[1];
                 }
             }
         }
-        if (substr($this->sefPath,-1) == "/") $this->sefPath = substr($this->sefPath,0,-1);
+        if (substr($this->sefPath,-1) == "/")
+            $this->sefPath = substr($this->sefPath,0,-1);  //fixme isn't this redundant from above?
         // sanitize it
         $sefPath = explode('">',$this->sefPath);  // remove any attempts to close the command
         $this->sefPath = expString::escape(expString::sanitize($sefPath[0]));
     }
 

Vulnerable code:     public  $url_style = '';
    public  $sefPath = null;
        // strip out possible xss exploits via url
                $_GET['id'] = intval($_GET['id']);
                $_POST['id'] = intval($_POST['id']);

            $_REQUEST['id'] = intval($_REQUEST['id']);
                    $_GET[$key] = intval($_GET[$key]);
                    $_POST[$key] = intval($_POST[$key]);

                $_REQUEST[$key] = intval($_REQUEST[$key]);
        // start splitting the URL into it's different parts
        $this->splitURL();
        // edebug($this,1);

                        $_POST['controller'] = $module;
                        $_REQUEST['controller'] = $module;
            if ($this->url_parts[count($this->url_parts)-1] == '') array_pop($this->url_parts);
            if (empty($this->url_parts[0])) array_shift($this->url_parts);
            } elseif (count($this->url_parts) == 1 || $db->selectObject('section', "sef_name='" . substr($this->sefPath,1) . "'") != null) {
                if (($db->selectObject('section', "sef_name='" . $this->url_parts[0] . "'") != null) && (in_array(array('printerfriendly','exportaspdf','ajax_action'), $this->url_parts))) {
            $this->url_style = 'sef';
//fixme and we may also want to log any redirects taken??
                $this->params = $this->convertPartsToParams();
                $return_params['url_parts']['id'] = $this->url_parts[2];
        // Set the module or controller - this how the actual routing happens
        $_REQUEST[$requestType] = $return_params['controller']; //url_parts[0];
        $_GET[$requestType] = $return_params['controller'];
        $_POST[$requestType] = $return_params['controller'];
        $_REQUEST['action'] = $action;
        $_GET['action'] = $action;
        $_POST['action'] = $action;
        // pass off the name<=>value pairs
            $save_value = expString::sanitize($value);
            $_REQUEST[$key] = $save_value;
            $_GET[$key] = $save_value;
            $url .= substr(PATH_RELATIVE,0,-1).$this->sefPath;
            if (isset($_REQUEST['PHPSESSID'])) unset($_REQUEST['PHPSESSID']);
        if (substr($this->sefPath,-1) == "/") $this->sefPath = substr($this->sefPath,0,-1);
Fixed code:     private $url_style = '';
    private $sefPath = null;
        // start splitting the URL into it's different parts
        $this->splitURL();
        // edebug($this,1);

        // strip out possible xss exploits via old school url
        //fixme only old school url and forms have these variables here
            $_REQUEST['id'] = intval($_REQUEST['id']);
                $_GET['id'] = $_REQUEST['id'];
                $_POST['id'] = $_REQUEST['id'];
                $_REQUEST[$key] = intval($_REQUEST[$key]);
                    $_GET[$key] = $_REQUEST[$key];
                    $_POST[$key] = $_REQUEST[$key];
            }
            if ($key == 'src') {
                $_REQUEST[$key] = preg_replace("/[^A-Za-z0-9@-]/", '', $_REQUEST[$key]);
                if (isset($_GET[$key]))
                    $_GET[$key] = $_REQUEST[$key];
                if (isset($_POST[$key]))
                    $_POST[$key] = $_REQUEST[$key];
                        $_POST['controller'] = $_REQUEST['controller'] = $module;
    /**
     * Assign url_type & url_style
     */
            if ($this->url_parts[count($this->url_parts)-1] == '')
                array_pop($this->url_parts);
            if (empty($this->url_parts[0]))
                array_shift($this->url_parts);
            else
                $this->url_parts[0] = expString::escape($this->url_parts[0]);
            } elseif (count($this->url_parts) == 1 || $db->selectObject('section', "sef_name='" . substr($this->sefPath, 1) . "'") != null) {
                if (($db->selectObject('section', "sef_name='" . $this->url_parts[0]) . "'" != null) && (in_array(array('printerfriendly','exportaspdf','ajax_action'), $this->url_parts))) {
            $this->url_style = 'sef';  // even if it's old school, they all come in the same
                //note 'query' doesn't need $params
    /**
     * Set up for page request, but check store category/product also
     *
     * @return bool
     */
//fixme and we may also want to log any redirects taken from that table??
                $this->params = $this->convertPartsToParams(); // update params to new re-mapped url_parts
                //fixme do we need to re-sanitize them???
    /**
     * Check and set up for an action request
     *
     * @return bool
     */
                $return_params['url_parts']['id'] = intval($this->url_parts[2]);
        // Set the controller - this how the actual routing happens
        $_REQUEST[$requestType] = $_GET[$requestType] = $_POST[$requestType] = $return_params['controller'];
        $_REQUEST['action'] = $_GET['action'] = $_POST['action'] = $action;
        // pass off the name<=>value pairs for old school url
            $_REQUEST[$key] = $_GET[$key] = expString::sanitize($value);
            if (count($this->params) > 2) {
                $url .= substr(PATH_RELATIVE,0,-1).'/'.$this->params['controller'].'/'.$this->params['action'];
                foreach ($this->params as $key=>$value) {
                    if ($key != 'controller' && $key != 'action') {
                        $url .= '/' . $key . '/' . $value;
                    }
                }
            } else {
                $url .= substr(PATH_RELATIVE,0,-1).$this->sefPath;  //fixme do we need to clean this up?
            }
    /**
     * Convert url_parts() or $_REQUEST to $params
     *
     * @return array|string
     */
            if (isset($_REQUEST['PHPSESSID']))
                unset($_REQUEST['PHPSESSID']);
        // conventional method to ensure the 'id' is only an id
        if (isset($params['id'])) {
            $params['id'] = intval($params['id']);
        }
        // do the same for the other id's
        foreach ($params as $key=>$var) {
            if (is_string($var) && strlen($key) >= 3 && strrpos($key,'_id',-3) !== false) {
                $params[$key] = intval($params[$key]);
            }
            if ($key == 'src') {
                $params[$key] = preg_replace("/[^A-Za-z0-9@-]/", '', $params[$key]);
            }
        }
    /**
     * Attempt to locate a page by name or id
     *
     * @param $url_name
     * @return null|object|void
     */
    /**
     * Get the SEF URL from the server
     * if we got an old school url, it will only contain the 'index.php'
     */
        if (substr($this->sefPath,-1) == "/")
            $this->sefPath = substr($this->sefPath,0,-1);  //fixme isn't this redundant from above?