Skip to content

Commit 559792b

Browse files
committed
fix sql injection security vulnerability which failed to account for sef urls adequately; reported by many, many users
1 parent b276cf4 commit 559792b

File tree

1 file changed

+96
-39
lines changed

1 file changed

+96
-39
lines changed

Diff for: framework/core/subsystems/expRouter.php

+96-39
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,9 @@ class expRouter {
3838
* either 'sef' or 'query'
3939
* @var string
4040
*/
41-
public $url_style = '';
41+
private $url_style = '';
4242
public $params = array();
43-
public $sefPath = null;
43+
private $sefPath = null;
4444

4545
function __construct() {
4646
self::getRouterMaps();
@@ -189,7 +189,11 @@ public function plainPath() {
189189
public function routeRequest() {
190190
global $user;
191191

192-
// strip out possible xss exploits via url
192+
// start splitting the URL into it's different parts
193+
$this->splitURL();
194+
// edebug($this,1);
195+
196+
// strip out possible xss exploits via old school url
193197
foreach ($_GET as $key=>$var) {
194198
if (is_string($var) && strpos($var,'">')) {
195199
unset(
@@ -198,35 +202,37 @@ public function routeRequest() {
198202
);
199203
}
200204
}
205+
//fixme only old school url and forms have these variables here
201206
// conventional method to ensure the 'id' is only an id
202207
if (isset($_REQUEST['id'])) {
208+
$_REQUEST['id'] = intval($_REQUEST['id']);
203209
if (isset($_GET['id']))
204-
$_GET['id'] = intval($_GET['id']);
210+
$_GET['id'] = $_REQUEST['id'];
205211
if (isset($_POST['id']))
206-
$_POST['id'] = intval($_POST['id']);
207-
208-
$_REQUEST['id'] = intval($_REQUEST['id']);
212+
$_POST['id'] = $_REQUEST['id'];
209213
}
210214
// do the same for the other id's
211215
foreach ($_REQUEST as $key=>$var) {
212216
if (is_string($var) && strlen($key) >= 3 && strrpos($key,'_id',-3) !== false) {
217+
$_REQUEST[$key] = intval($_REQUEST[$key]);
213218
if (isset($_GET[$key]))
214-
$_GET[$key] = intval($_GET[$key]);
219+
$_GET[$key] = $_REQUEST[$key];
215220
if (isset($_POST[$key]))
216-
$_POST[$key] = intval($_POST[$key]);
217-
218-
$_REQUEST[$key] = intval($_REQUEST[$key]);
221+
$_POST[$key] = $_REQUEST[$key];
222+
}
223+
if ($key == 'src') {
224+
$_REQUEST[$key] = preg_replace("/[^A-Za-z0-9@-]/", '', $_REQUEST[$key]);
225+
if (isset($_GET[$key]))
226+
$_GET[$key] = $_REQUEST[$key];
227+
if (isset($_POST[$key]))
228+
$_POST[$key] = $_REQUEST[$key];
219229
}
220230
}
221231
if (empty($user->id) || (!empty($user->id) && !$user->isAdmin())) { //FIXME why would $user be empty here unless $db is down?
222232
// $_REQUEST['route_sanitized'] = true;//FIXME debug test
223233
expString::sanitize($_REQUEST); // strip other exploits like sql injections
224234
}
225235

226-
// start splitting the URL into it's different parts
227-
$this->splitURL();
228-
// edebug($this,1);
229-
230236
if ($this->url_style == 'sef') {
231237
if ($this->url_type == 'page' || $this->url_type == 'base') {
232238
$ret = $this->routePageRequest(); // if we hit this the formatting of the URL looks like the user is trying to go to a page.
@@ -248,8 +254,7 @@ public function routeRequest() {
248254
$module = !empty($_POST['controller']) ? expString::sanitize($_POST['controller']) : expString::sanitize($_POST['module']);
249255
// Figure out if this is module or controller request - WE ONLY NEED THIS CODE UNTIL WE PULL OUT THE OLD MODULES
250256
if (expModules::controllerExists($module)) {
251-
$_POST['controller'] = $module;
252-
$_REQUEST['controller'] = $module;
257+
$_POST['controller'] = $_REQUEST['controller'] = $module;
253258
}
254259
}
255260
}
@@ -305,6 +310,9 @@ public function updateHistory($section=null) {
305310
$db->insertObject($trackingObject,'tracking_rawdata');
306311
}
307312

313+
/**
314+
* Assign url_type & url_style
315+
*/
308316
public function splitURL() {
309317
global $db;
310318

@@ -317,27 +325,31 @@ public function splitURL() {
317325

318326
// remove empty first and last url_parts if they exist
319327
//if (empty($this->url_parts[count($this->url_parts)-1])) array_pop($this->url_parts);
320-
if ($this->url_parts[count($this->url_parts)-1] == '') array_pop($this->url_parts);
321-
if (empty($this->url_parts[0])) array_shift($this->url_parts);
328+
if ($this->url_parts[count($this->url_parts)-1] == '')
329+
array_pop($this->url_parts);
330+
if (empty($this->url_parts[0]))
331+
array_shift($this->url_parts);
332+
else
333+
$this->url_parts[0] = expString::escape($this->url_parts[0]);
322334

323335
if (count($this->url_parts) < 1 || (empty($this->url_parts[0]) && count($this->url_parts) == 1) ) {
324336
$this->url_type = 'base'; // no params
325-
} elseif (count($this->url_parts) == 1 || $db->selectObject('section', "sef_name='" . substr($this->sefPath,1) . "'") != null) {
337+
} elseif (count($this->url_parts) == 1 || $db->selectObject('section', "sef_name='" . substr($this->sefPath, 1) . "'") != null) {
326338
$this->url_type = 'page'; // single param is page name
327339
} elseif ($_SERVER['REQUEST_METHOD'] == 'POST') {
328340
$this->url_type = 'post'; // params via form/post
329341
} else {
330342
// 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
331343
// extra perms...like printerfriendly=1 or ajax_action=1;
332-
if (($db->selectObject('section', "sef_name='" . $this->url_parts[0] . "'") != null) && (in_array(array('printerfriendly','exportaspdf','ajax_action'), $this->url_parts))) {
344+
if (($db->selectObject('section', "sef_name='" . $this->url_parts[0]) . "'" != null) && (in_array(array('printerfriendly','exportaspdf','ajax_action'), $this->url_parts))) {
333345
$this->url_type = 'page';
334346
} else {
335347
$this->url_type = 'action';
336348
}
337349
}
338350
$this->params = $this->convertPartsToParams();
339351
} elseif ($_SERVER['REQUEST_METHOD'] == 'POST') {
340-
$this->url_style = 'sef';
352+
$this->url_style = 'sef'; // even if it's old school, they all come in the same
341353
$this->url_type = 'post';
342354
$this->params = $this->convertPartsToParams();
343355
} elseif (isset($_SERVER['REQUEST_URI'])) {
@@ -350,6 +362,7 @@ public function splitURL() {
350362
$sefPath = explode('%22%3E',$_SERVER['REQUEST_URI']); // remove any attempts to close the command
351363
$_SERVER['REQUEST_URI'] = $sefPath[0];
352364
$this->url_style = 'query';
365+
//note 'query' doesn't need $params
353366
}
354367
} else {
355368
$this->url_type = 'base';
@@ -362,6 +375,11 @@ public function splitURL() {
362375
define('EXPORT_AS_PDF_LANDSCAPE', (isset($_REQUEST['landscapepdf']) || isset($this->params['landscapepdf'])) ? 1 : 0);
363376
}
364377

378+
/**
379+
* Set up for page request, but check store category/product also
380+
*
381+
* @return bool
382+
*/
365383
public function routePageRequest() {
366384
// global $db;
367385

@@ -430,7 +448,7 @@ public function routePageRequest() {
430448
return $this->routeActionRequest();
431449
}
432450
//fixme we may want to log missed pages (no existing store cat/product) requests and set up/use a redirect table (404)??
433-
//fixme and we may also want to log any redirects taken??
451+
//fixme and we may also want to log any redirects taken from that table??
434452
return false;
435453
}
436454
#########################################################
@@ -493,14 +511,20 @@ public function isMappedURL() {
493511
}
494512
}
495513

496-
$this->params = $this->convertPartsToParams();
514+
$this->params = $this->convertPartsToParams(); // update params to new re-mapped url_parts
515+
//fixme do we need to re-sanitize them???
497516
return true;
498517
}
499518
}
500519

501520
return false;
502521
}
503522

523+
/**
524+
* Check and set up for an action request
525+
*
526+
* @return bool
527+
*/
504528
public function routeActionRequest() {
505529
$return_params = array('controller'=>'','action'=>'','url_parts'=>array());
506530

@@ -520,7 +544,7 @@ public function routeActionRequest() {
520544
// now figure out the name<=>value pairs
521545
if (count($this->url_parts) == 3) {
522546
if ( is_numeric($this->url_parts[2])) {
523-
$return_params['url_parts']['id'] = $this->url_parts[2];
547+
$return_params['url_parts']['id'] = intval($this->url_parts[2]);
524548
}
525549
} else {
526550
for ($i = 2, $iMax = count($this->url_parts); $i < $iMax; $i++) {
@@ -530,10 +554,8 @@ public function routeActionRequest() {
530554
}
531555
}
532556

533-
// Set the module or controller - this how the actual routing happens
534-
$_REQUEST[$requestType] = $return_params['controller']; //url_parts[0];
535-
$_GET[$requestType] = $return_params['controller'];
536-
$_POST[$requestType] = $return_params['controller'];
557+
// Set the controller - this how the actual routing happens
558+
$_REQUEST[$requestType] = $_GET[$requestType] = $_POST[$requestType] = $return_params['controller'];
537559

538560
// Set the action for this module or controller
539561
if ($_SERVER['REQUEST_METHOD'] == 'POST') {
@@ -544,15 +566,11 @@ public function routeActionRequest() {
544566
$action = $return_params['action'];
545567
}
546568

547-
$_REQUEST['action'] = $action;
548-
$_GET['action'] = $action;
549-
$_POST['action'] = $action;
569+
$_REQUEST['action'] = $_GET['action'] = $_POST['action'] = $action;
550570

551-
// pass off the name<=>value pairs
571+
// pass off the name<=>value pairs for old school url
552572
foreach($return_params['url_parts'] as $key=>$value) {
553-
$save_value = expString::sanitize($value);
554-
$_REQUEST[$key] = $save_value;
555-
$_GET[$key] = $save_value;
573+
$_REQUEST[$key] = $_GET[$key] = expString::sanitize($value);
556574
}
557575

558576
return true;
@@ -561,7 +579,16 @@ public function routeActionRequest() {
561579
public function buildCurrentUrl() {
562580
$url = URL_BASE;
563581
if ($this->url_style == 'sef') {
564-
$url .= substr(PATH_RELATIVE,0,-1).$this->sefPath;
582+
if (count($this->params) > 2) {
583+
$url .= substr(PATH_RELATIVE,0,-1).'/'.$this->params['controller'].'/'.$this->params['action'];
584+
foreach ($this->params as $key=>$value) {
585+
if ($key != 'controller' && $key != 'action') {
586+
$url .= '/' . $key . '/' . $value;
587+
}
588+
}
589+
} else {
590+
$url .= substr(PATH_RELATIVE,0,-1).$this->sefPath; //fixme do we need to clean this up?
591+
}
565592
} else {
566593
$url .= urldecode((empty($_SERVER['REQUEST_URI'])) ? $_ENV['REQUEST_URI'] : $_SERVER['REQUEST_URI']);
567594
}
@@ -665,6 +692,11 @@ public function convertToOldSchoolUrl() {
665692
return $this->makeLink($params, true);
666693
}
667694

695+
/**
696+
* Convert url_parts() or $_REQUEST to $params
697+
*
698+
* @return array|string
699+
*/
668700
public function convertPartsToParams() {
669701
$params = array();
670702
if ($this->url_type == 'base') {
@@ -681,7 +713,8 @@ public function convertPartsToParams() {
681713
}
682714
}
683715
} elseif ($this->url_type == 'post') {
684-
if (isset($_REQUEST['PHPSESSID'])) unset($_REQUEST['PHPSESSID']);
716+
if (isset($_REQUEST['PHPSESSID']))
717+
unset($_REQUEST['PHPSESSID']);
685718
// foreach($_REQUEST as $name=>$val) {
686719
//// if (get_magic_quotes_gpc()) $val = stripslashes($val); // magic quotes fix??
687720
//// $params[$name] = $val;
@@ -693,9 +726,28 @@ public function convertPartsToParams() {
693726
}
694727
//TODO: fully sanitize all params values here for ---We already do this!
695728
// if (isset($params['src'])) $params['src'] = expString::sanitize(htmlspecialchars($params['src']));
729+
// conventional method to ensure the 'id' is only an id
730+
if (isset($params['id'])) {
731+
$params['id'] = intval($params['id']);
732+
}
733+
// do the same for the other id's
734+
foreach ($params as $key=>$var) {
735+
if (is_string($var) && strlen($key) >= 3 && strrpos($key,'_id',-3) !== false) {
736+
$params[$key] = intval($params[$key]);
737+
}
738+
if ($key == 'src') {
739+
$params[$key] = preg_replace("/[^A-Za-z0-9@-]/", '', $params[$key]);
740+
}
741+
}
696742
return $params;
697743
}
698744

745+
/**
746+
* Attempt to locate a page by name or id
747+
*
748+
* @param $url_name
749+
* @return null|object|void
750+
*/
699751
public function getPageByName($url_name) {
700752
global $db;
701753

@@ -721,6 +773,10 @@ public function getPageByName($url_name) {
721773
return $section;
722774
}
723775

776+
/**
777+
* Get the SEF URL from the server
778+
* if we got an old school url, it will only contain the 'index.php'
779+
*/
724780
private function buildSEFPath () {
725781
// Apache
726782
if (strpos($_SERVER['SERVER_SOFTWARE'],'Apache') !== false || strpos($_SERVER['SERVER_SOFTWARE'],'WebServerX') !== false) {
@@ -775,7 +831,8 @@ private function buildSEFPath () {
775831
}
776832
}
777833
}
778-
if (substr($this->sefPath,-1) == "/") $this->sefPath = substr($this->sefPath,0,-1);
834+
if (substr($this->sefPath,-1) == "/")
835+
$this->sefPath = substr($this->sefPath,0,-1); //fixme isn't this redundant from above?
779836
// sanitize it
780837
$sefPath = explode('">',$this->sefPath); // remove any attempts to close the command
781838
$this->sefPath = expString::escape(expString::sanitize($sefPath[0]));

0 commit comments

Comments
 (0)