From: bradymiller Date: Sat, 2 Mar 2013 07:45:37 +0000 (-0800) Subject: Sql-injection functions and techniques for escaping(take 3): X-Git-Tag: whats-been-changed~381 X-Git-Url: https://repo.or.cz/w/openemr.git/commitdiff_plain/f33c777e8dcef6eb44dbfae6f44ab8d62c372dae Sql-injection functions and techniques for escaping(take 3): 1. When variables within limits 2. When variable for the sort order 3. When variable for an identifier (all of these are things that are exceptions to standard binding/escaping) --- diff --git a/interface/globals.php b/interface/globals.php index b1f07e362..078df4722 100644 --- a/interface/globals.php +++ b/interface/globals.php @@ -152,9 +152,12 @@ $GLOBALS['edi_271_file_path'] = $GLOBALS['OE_SITE_DIR'] . "/edi/"; // open the openemr mysql connection. include_once (dirname(__FILE__) . "/../library/translation.inc.php"); -// Include convenience functions with shorter names than "htmlspecialchars" +// Include convenience functions with shorter names than "htmlspecialchars" (for security) require_once (dirname(__FILE__) . "/../library/htmlspecialchars.inc.php"); +// Include sanitization/checking functions (for security) +require_once (dirname(__FILE__) . "/../library/formdata.inc.php"); + // Include sanitization/checking function (for security) require_once (dirname(__FILE__) . "/../library/sanitize.inc.php"); diff --git a/library/formdata.inc.php b/library/formdata.inc.php index fa4706c08..21c348d6a 100644 --- a/library/formdata.inc.php +++ b/library/formdata.inc.php @@ -1,18 +1,108 @@ * - * This program is free software; you can redistribute it and/or + * LICENSE: This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License * as published by the Free Software Foundation; either version 2 * of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * You should have received a copy of the GNU General Public License + * along with this program. If not, see ;. + * + * @package OpenEMR + * @author Rod Roark + * @author Brady Miller + * @link http://www.open-emr.org + */ + +/** + * Escape a parameter to prepare for a sql query. + * + * @param string $s Parameter to be escaped. + * @return string Escaped parameter. + */ +function add_escape_custom($s) { + //prepare for safe mysql insertion + $s = mysql_real_escape_string($s); + return $s; +} + +/** + * Escape a sql limit variable to prepare for a sql query. + * + * This will escape integers within the LIMIT ?, ? part of a sql query. + * Note that there is a maximum value to these numbers, which is why + * should only use for the LIMIT ? , ? part of the sql query and why + * this is centralized to a function (in case need to upgrade this + * function to support larger numbers in the future). + * + * @param string $s Limit variable to be escaped. + * @return string Escaped limit variable. + */ +function escape_limit($s) { + //prepare for safe mysql insertion + $s = (int)$s; + return $s; +} + +/** + * Escape/sanitize a sql sort order keyword variable to prepare for a sql query. + * + * This will escape/sanitize the sort order keyword. It is done by whitelisting + * only certain keywords(asc,desc). If the keyword is illegal, then will default + * to asc. * - * These functions should be used to globally validate and prepare - * data for sql database insertion. + * @param string $s Sort order keyword variable to be escaped/sanitized. + * @return string Escaped sort order keyword variable. + */ +function escape_sort_order($s) { + $ok = array("asc","desc"); + $key = array_search(strtolower($s),$ok); + return $ok[$key]; +} + +/** + * Escape/sanitize a sql identifier variable to prepare for a sql query. + * + * This will escape/sanitize a sql identifier. There are two options provided by this funtion. + * The first option is done by whitelisting ($whitelist_flag=true) and in this case + * only certain identifiers (listed in the $whitelist_items array) can be used; if + * there is no match, then it will default to the first item in the $whitelist_items array. + * The second option is done by sanitizing ($whitelist_flag=false) and in this case + * only US alphanumeric,'_' and '.' items are kept in the returned string. Note + * the second option is still experimental as we figure out the ideal items to + * filter out of the identifier. The first option is ideal if all the possible identifiers + * are known, however we realize this may not always be the case. * + * @param string $s Sql identifier variable to be escaped/sanitized. + * @param boolean $whitelist_flag True to use whitelisting method (See function description for details of whitelisting method). + * @param array $whitelist_items Items used in whitelisting method (See function description for details of whitelisting method). + * @return string Escaped/sanitized sql identifier variable. */ +function escape_identifier($s,$whitelist_flag=FALSE,$whitelist_items) { + if ($whitelist_flag) { + // Only return an item within the whitelist_items + // (if no match, then it will return the first item in whitelist_items) + $ok = $whitelist_items; + $key = array_search($s,$ok); + return $ok[$key]; + } + else { + // Return an item that has been "cleaned" up + // (this is currently experimental) + return preg_replace('/[^a-zA-Z0-9_.]/','',$s); + } +} -/** Main function that will manage POST, GET, and REQUEST variables +/** + * (Note this function is deprecated for new scripts and is only utilized to support legacy scripts) + * Function to manage POST, GET, and REQUEST variables. * * @param string $name name of the variable requested. * @param string $type 'P', 'G' for post or get data, otherwise uses request. @@ -30,9 +120,16 @@ function formData($name, $type='P', $isTrim=false) { return formDataCore($s,$isTrim); } -// Core function that will be called by formData. -// Note it can also be called directly if preparing -// normal variables (not GET,POST, or REQUEST) +/** + * (Note this function is deprecated for new scripts and is only utilized to support legacy scripts) + * Core function that will be called by formData. + * Note it can also be called directly if preparing + * normal variables (not GET,POST, or REQUEST) + * + * @param string $s + * @param bool $istrim whether to use trim() on the data. + * @return string + */ function formDataCore($s, $isTrim=false) { //trim if selected if ($isTrim) {$s = trim($s);} @@ -43,31 +140,32 @@ function formDataCore($s, $isTrim=false) { return $s; } -// Will remove escapes if needed (ie magic quotes turned on) from string -// Called by above formDataCore() function to prepare for database insertion. -// Can also be called directly if simply need to remove escaped characters -// from a string before processing. +/** + * (Note this function is deprecated for new scripts and is only utilized to support legacy scripts) + * Will remove escapes if needed (ie magic quotes turned on) from string + * Called by above formDataCore() function to prepare for database insertion. + * Can also be called directly if simply need to remove escaped characters + * from a string before processing. + * + * @param string $s + * @return string + */ function strip_escape_custom($s) { //strip slashes if magic quotes turned on if (get_magic_quotes_gpc()) {$s = stripslashes($s);} return $s; } -// Will add escapes as needed onto a string -// Called by above formDataCore() function to prepare for database insertion. -// Can also be called directly if need to escape an already process string (ie. -// escapes were already removed, then processed, and now want to insert into -// database) -function add_escape_custom($s) { - //prepare for safe mysql insertion - $s = mysql_real_escape_string($s); - return $s; -} - -// This function is only being kept to support -// previous functionality. If you want to trim -// variables, this should be done using above -// functions. +/** + * (Note this function is deprecated for new scripts and is only utilized to support legacy scripts) + * This function is only being kept to support + * previous functionality. If you want to trim + * variables, this should be done using above + * functions. + * + * @param string $s + * @return string + */ function formTrim($s) { return formDataCore($s,true); } diff --git a/library/pnotes.inc b/library/pnotes.inc index 5b6e265c6..d8e6550c7 100644 --- a/library/pnotes.inc +++ b/library/pnotes.inc @@ -38,7 +38,7 @@ function getPnoteById($id, $cols = "*") * messages, or all users' messages. * @param string $user The user whom's notes you want to retrieve. * @param bool $count Whether to return a count, or just return 0. - * @param string $sortby A field to sort results by. + * @param string $sortby A field to sort results by. (options are users.lname,patient_data.lname,pnotes.title,pnotes.date,pnotes.message_status) (will default to users.lname) * @param string $sortorder whether to sort ascending or descending. * @param string $begin what row to start retrieving results from. * @param string $listnumber number of rows to return. @@ -47,6 +47,9 @@ function getPnoteById($id, $cols = "*") function getPnotesByUser($activity="1",$show_all="no",$user='',$count=false,$sortby='',$sortorder='',$begin='',$listnumber='') { + // Set the options for $sortby (for sql-injection hardening) + $sortbyoptions = array("users.lname","patient_data.lname","pnotes.title","pnotes.date","pnotes.message_status"); + // Set the activity part of query if ($activity=='1') { $activity_query = " pnotes.message_status != 'Done' AND pnotes.activity = 1 AND "; @@ -75,8 +78,9 @@ function getPnotesByUser($activity="1",$show_all="no",$user='',$count=false,$sor LEFT JOIN patient_data ON pnotes.pid = patient_data.pid) WHERE $activity_query pnotes.deleted != '1' AND pnotes.assigned_to LIKE ?"; if (!empty($sortby) || !empty($sortorder) || !empty($begin) || !empty($listnumber)) { - $sql .= " order by ".add_escape_custom($sortby)." ".add_escape_custom($sortorder). - " limit ".add_escape_custom($begin).", ".add_escape_custom($listnumber); + $sql .= " order by ".escape_identifier($sortby,TRUE,$sortbyoptions). + " ".escape_sort_order($sortorder). + " limit ".escape_limit($begin).", ".escape_limit($listnumber); } $result = sqlStatement($sql, array($usrvar)); @@ -129,7 +133,7 @@ $sqlParameterArray = array(); $sql .= " AND message_status IN ('".str_replace(",", "','", add_escape_custom($status) )."')"; $sql .= " ORDER BY date DESC"; if($limit != "all") - $sql .= " LIMIT ".add_escape_custom($start).", ".add_escape_custom($limit); + $sql .= " LIMIT ".escape_limit($start).", ".escape_limit($limit); $res = sqlStatement($sql, $sqlParameterArray); @@ -174,7 +178,7 @@ $sqlParameterArray = array(); $sql .= " AND message_status IN ('".str_replace(",", "','", add_escape_custom($status) )."')"; $sql .= " ORDER BY date DESC"; if($limit != "all") - $sql .= " LIMIT ".add_escape_custom($start).", ".$limit; + $sql .= " LIMIT ".escape_limit($start).", ".escape_limit($limit); $res = sqlStatement($sql, $sqlParameterArray); @@ -187,7 +191,7 @@ $sqlParameterArray = array(); function getPatientNotes($pid = '', $limit = '', $offset = 0, $search = '') { if($limit){ - $limit = "LIMIT ".add_escape_custom($offset).", ".add_escape_custom($limit); + $limit = "LIMIT ".escape_limit($offset).", ".escape_limit($limit); } $sql = " SELECT @@ -223,7 +227,7 @@ function getPatientNotes($pid = '', $limit = '', $offset = 0, $search = '') function getPatientNotifications($pid = '', $limit = '', $offset = 0, $search = '') { if($limit){ - $limit = "LIMIT ".add_escape_custom($offset).", ".add_escape_custom($limit); + $limit = "LIMIT ".escape_limit($offset).", ".escape_limit($limit); } $sql = " SELECT @@ -259,7 +263,7 @@ function getPatientNotifications($pid = '', $limit = '', $offset = 0, $search = function getPatientSentNotes($pid = '', $limit = '', $offset = 0, $search = '') { if($limit){ - $limit = "LIMIT ".add_escape_custom($offset).", ".add_escape_custom($limit); + $limit = "LIMIT ".escape_limit($offset).", ".escape_limit($limit); } $sql = " SELECT @@ -303,7 +307,7 @@ function getPnotesByPid ($pid, $activity = "1", $cols = "*", $limit=10, $start=0 "AND activity = '1' ". " AND message_status != 'Done' ". " AND deleted != 1 ". - " ORDER BY date DESC LIMIT ".add_escape_custom($start).",".add_escape_custom($limit), array($pid) ); + " ORDER BY date DESC LIMIT ".escape_limit($start).",".escape_limit($limit), array($pid) ); } else if ($activity == '0') { // return only inactive @@ -311,13 +315,13 @@ function getPnotesByPid ($pid, $activity = "1", $cols = "*", $limit=10, $start=0 "AND (activity = '0' ". " OR message_status = 'Done') ". " AND deleted != 1 ". - " ORDER BY date DESC LIMIT ".add_escape_custom($start).",".add_escape_custom($limit), array($pid) ); + " ORDER BY date DESC LIMIT ".escape_limit($start).",".escape_limit($limit), array($pid) ); } else { // $activity == "all" // return both active and inactive $res = sqlStatement("SELECT $cols FROM pnotes WHERE pid LIKE ? " . " AND deleted != 1 ". - " ORDER BY date DESC LIMIT ".add_escape_custom($start).",".add_escape_custom($limit), array($pid) ); + " ORDER BY date DESC LIMIT ".escape_limit($start).",".escape_limit($limit), array($pid) ); } for ($iter = 0; $row = sqlFetchArray($res); $iter++) $all[$iter] = $row;