From d56d58fcfbe04214f9df419901e766341a1b95d1 Mon Sep 17 00:00:00 2001 From: bradymiller Date: Sun, 10 Mar 2013 01:35:20 -0800 Subject: [PATCH] Additional Sql-injection functions and techniques for escaping; 1. Improved/clarified the functions in library/formdata.inc.php 2. Added mechanism for whitelisting openemr sql table names. 3. Added mechanism for whitelisting openemr sql column names. 4. Incorporated it into the messages module 5. Incorporated into dictation form 6. Incorporated into work/school form/note --- interface/forms/dictation/new.php | 14 +++-- interface/forms/dictation/report.php | 4 +- interface/forms/dictation/save.php | 10 ++-- interface/forms/dictation/table.sql | 12 ---- interface/forms/dictation/view.php | 16 +++-- interface/forms/note/new.php | 34 ++++++----- interface/forms/note/print.php | 24 ++++---- interface/forms/note/report.php | 4 +- interface/forms/note/save.php | 4 ++ interface/forms/note/view.php | 42 +++++++------ library/api.inc | 28 ++++++--- library/formdata.inc.php | 110 ++++++++++++++++++++++++++++++----- library/forms.inc | 17 ++++-- library/pnotes.inc | 5 +- 14 files changed, 215 insertions(+), 109 deletions(-) delete mode 100644 interface/forms/dictation/table.sql diff --git a/interface/forms/dictation/new.php b/interface/forms/dictation/new.php index 6ef6abd53..b372978bb 100644 --- a/interface/forms/dictation/new.php +++ b/interface/forms/dictation/new.php @@ -1,5 +1,9 @@ " . xl($key) . ": " . - nl2br($value) . ""; + print "" . xlt($key) . ": " . + nl2br(text($value)) . ""; $count++; if ($count == $cols) { $count = 0; diff --git a/interface/forms/dictation/save.php b/interface/forms/dictation/save.php index a58955f1d..916219c45 100644 --- a/interface/forms/dictation/save.php +++ b/interface/forms/dictation/save.php @@ -1,19 +1,19 @@ $var) { -$_POST[$k] = mysql_escape_string($var); -echo "$var\n"; -} if ($encounter == "") $encounter = date("Ymd"); if ($_GET["mode"] == "new"){ $newid = formSubmit("form_dictation", $_POST, $_GET["id"], $userauthorized); addForm($encounter, "Speech Dictation", $newid, "dictation", $pid, $userauthorized); }elseif ($_GET["mode"] == "update") { -sqlInsert("update form_dictation set pid = {$_SESSION["pid"]},groupname='".$_SESSION["authProvider"]."',user='".$_SESSION["authUser"]."',authorized=$userauthorized,activity=1, date = NOW(), dictation='".$_POST["dictation"]."', additional_notes='".$_POST["additional_notes"]."' where id=$id"); +sqlInsert("update form_dictation set pid = ?,groupname=?,user=?,authorized=?,activity=1, date = NOW(), dictation=?, additional_notes=? where id=?",array($_SESSION["pid"],$_SESSION["authProvider"],$_SESSION["authUser"],$userauthorized,$_POST["dictation"],$_POST["additional_notes"],$_GET["id"])); } $_SESSION["encounter"] = $encounter; formHeader("Redirecting...."); diff --git a/interface/forms/dictation/table.sql b/interface/forms/dictation/table.sql deleted file mode 100644 index 2701f232e..000000000 --- a/interface/forms/dictation/table.sql +++ /dev/null @@ -1,12 +0,0 @@ -CREATE TABLE IF NOT EXISTS `form_dictation` ( -id bigint(20) NOT NULL auto_increment, -date datetime default NULL, -pid bigint(20) default NULL, -user varchar(255) default NULL, -groupname varchar(255) default NULL, -authorized tinyint(4) default NULL, -activity tinyint(4) default NULL, -dictation longtext, -additional_notes longtext, -PRIMARY KEY (id) -) ENGINE=MyISAM; diff --git a/interface/forms/dictation/view.php b/interface/forms/dictation/view.php index 2404435d5..6bbac5ae1 100644 --- a/interface/forms/dictation/view.php +++ b/interface/forms/dictation/view.php @@ -1,5 +1,9 @@ @@ -12,15 +16,15 @@ $returnurl = $GLOBALS['concurrent_layout'] ? 'encounter_top.php' : 'patient_enco include_once("$srcdir/api.inc"); $obj = formFetch("form_dictation", $_GET["id"]); ?> - @@ -51,19 +55,19 @@ var mypcc = ''; diff --git a/interface/forms/note/print.php b/interface/forms/note/print.php index 671b53767..d7a90cb2b 100644 --- a/interface/forms/note/print.php +++ b/interface/forms/note/print.php @@ -1,9 +1,13 @@

-

- +

+

- +
-

- "> + ">

- + ' + value='' />

diff --git a/interface/forms/note/report.php b/interface/forms/note/report.php index b4cf7bc4d..c0fd017d1 100644 --- a/interface/forms/note/report.php +++ b/interface/forms/note/report.php @@ -43,10 +43,10 @@ function note_report( $pid, $encounter, $cols, $id) { print("\n"); print("\n"); if ($key == "Note Type") { - print "" . xl($key) . ": " . xl($value) . ""; + print "" . xlt($key) . ": " . xlt($value) . ""; } else { - print "" . xl($key) . ": $value"; + print "" . xlt($key) . ": " . text($value) . ""; } $count++; if ($count == $cols) { diff --git a/interface/forms/note/save.php b/interface/forms/note/save.php index e1425ba05..c206c7b41 100644 --- a/interface/forms/note/save.php +++ b/interface/forms/note/save.php @@ -16,6 +16,10 @@ Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. --> '; function PrintForm() { - newwin = window.open("","mywin"); + newwin = window.open("","mywin"); } @@ -60,44 +64,44 @@ function PrintForm() { -" name="my_form" id="my_form"> -

+" name="my_form" id="my_form"> +

- - - + + +

- +
- +

- "> + ">

- + ' - title='' + value='' + title='' onkeyup='datekeyup(this,mypcc)' onblur='dateblur(this,mypcc)' />

'> + title=''>

- - - + + +

diff --git a/library/api.inc b/library/api.inc index c4b14cf71..b91b25f35 100644 --- a/library/api.inc +++ b/library/api.inc @@ -38,7 +38,7 @@ function formSubmit ($tableName, $values, $id, $authorized = "0") // the variable escaping method. global $sanitize_all_escapes; - $sql = "insert into $tableName set pid = {$_SESSION['pid']},groupname='".$_SESSION['authProvider']."',user='".$_SESSION['authUser']."',authorized=$authorized,activity=1, date = NOW(),"; + $sql = "insert into " . escape_table_name($tableName) . " set pid = ?,groupname=?,user=?,authorized=?,activity=1, date = NOW(),"; foreach ($values as $key => $value) if (strpos($key,"openemr_net_cpt") === 0) { //code to auto add cpt code @@ -61,7 +61,7 @@ function formSubmit ($tableName, $values, $id, $authorized = "0") else { if (isset($sanitize_all_escapes) && $sanitize_all_escapes) { // using new security method, so escape the key and values here - $sql .= " " . add_escape_custom($key) . " = '" . add_escape_custom($value) . "',"; + $sql .= " " . escape_sql_column_name($key,array($tableName)) . " = '" . add_escape_custom($value) . "',"; } else { // original method (rely on code to escape values before using this function) @@ -69,19 +69,30 @@ function formSubmit ($tableName, $values, $id, $authorized = "0") } } $sql = substr($sql, 0, -1); - return sqlInsert($sql); + return sqlInsert($sql,array($_SESSION['pid'],$_SESSION['authProvider'],$_SESSION['authUser'],$authorized)); } function formUpdate ($tableName, $values, $id, $authorized = "0") { - $sql = "update $tableName set pid = {$_SESSION['pid']},groupname='".$_SESSION['authProvider']."',user='".$_SESSION['authUser']."',authorized=$authorized,activity=1, date = NOW(),"; + // Bring in $sanitize_all_escapes variable, which will decide + // the variable escaping method. + global $sanitize_all_escapes; + + $sql = "update " . escape_table_name($tableName) . " set pid = ?,groupname=?,user=?,authorized=?,activity=1, date = NOW(),"; foreach ($values as $key => $value) - $sql .= " $key = '$value',"; + if (isset($sanitize_all_escapes) && $sanitize_all_escapes) { + // using new security method, so escape the key and values here + $sql .= " " . escape_sql_column_name($key,array($tableName)) . " = '" . add_escape_custom($value) . "',"; + } + else { + // original method (rely on code to escape values before using this function) + $sql .= " $key = '$value',"; + } $sql = substr($sql, 0, -1); - $sql .= " where id=$id"; + $sql .= " where id=?"; - return sqlInsert($sql); + return sqlInsert($sql,array($_SESSION['pid'],$_SESSION['authProvider'],$_SESSION['authUser'],$authorized,$id)); } @@ -96,7 +107,8 @@ function formJump ($address = "0") function formFetch ($tableName, $id, $cols="*", $activity="1") { - return sqlQuery ( "select $cols from `$tableName` where id='$id' and pid = '{$GLOBALS['pid']}' and activity like '$activity' order by date DESC LIMIT 0,1" ) ; + // Note that tableName is hard-coded, so no need to run through escape_table_name() function (at least not yet). + return sqlQuery ( "select $cols from `" . $tableName . "` where id=? and pid = ? and activity like ? order by date DESC LIMIT 0,1", array($id,$GLOBALS['pid'],$activity) ) ; } function formGetIds ($tableName, $cols = "*", $limit='all', $start=0, $activity = "1") diff --git a/library/formdata.inc.php b/library/formdata.inc.php index 21c348d6a..f1113b3ec 100644 --- a/library/formdata.inc.php +++ b/library/formdata.inc.php @@ -62,40 +62,120 @@ function escape_limit($s) { * @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]; + return escape_identifier(strtolower($s),array("asc","desc")); +} + +/** + * Escape/sanitize a table sql column name for a sql query.. + * + * This will escape/sanitize the sql column name for a sql query. It is done by whitelisting + * all of the current sql column names in the openemr database from a table(s). Note that if + * there is no match, then it will die() and a error message will be sent to the screen and + * the error log. This function should not be used for escaping tables outside the openemr + * database (should use escape_identifier() function below for that scenario) + * + * @param string $s sql column name variable to be escaped/sanitized. + * @param array $tables The table(s) that the sql columns is from (in an array). + * @param boolean $long Use long form (ie. table.colname) vs short form (ie. colname). + * @return string Escaped table name variable. + */ +function escape_sql_column_name($s,$tables,$long=FALSE) { + + // If the $tables is empty, then process them all + if (empty($tables)) { + $res = sqlStatementNoLog("SHOW TABLES"); + $tables = array(); + while ($row=sqlFetchArray($res)) { + $keys_return = array_keys($row); + $tables[]=$row[$keys_return[0]]; + } + } + + // First need to escape the $tables + $tables_escaped = array(); + foreach ($tables as $table) { + $tables_escaped[] = escape_table_name($table); + } + + // Collect all the possible sql columns from the tables + $columns_options = array(); + foreach ($tables_escaped as $table_escaped) { + $res = sqlStatementNoLog("SHOW COLUMNS FROM ".$table_escaped); + while ($row=sqlFetchArray($res)) { + if ($long) { + $columns_options[]=$table_escaped.".".$row['Field']; + } + else { + $columns_options[]=$row['Field']; + } + } + } + + // Now can escape(via whitelisting) the sql column name + return escape_identifier($s,$columns_options,TRUE); +} + +/** + * Escape/sanitize a table name for a sql query.. + * + * This will escape/sanitize the table name for a sql query. It is done by whitelisting + * all of the current tables in the openemr database. Note that if there is no match, then + * it will die() and a error message will be sent to the screen and the error log. This + * function should not be used for escaping tables outside the openemr database (should + * use escape_identifier() function below for that scenario) + * + * @param string $s sql table name variable to be escaped/sanitized. + * @return string Escaped table name variable. + */ +function escape_table_name($s) { + $res = sqlStatementNoLog("SHOW TABLES"); + $tables_array = array(); + while ($row=sqlFetchArray($res)) { + $keys_return = array_keys($row); + $tables_array[]=$row[$keys_return[0]]; + } + + // Now can escape(via whitelisting) the sql table name + return escape_identifier($s,$tables_array,TRUE); } /** * 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 + * This will escape/sanitize a sql identifier. There are two options provided by this + * function. + * The first option is done by whitelisting ($whitelist_items is used) 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 + * there is no match, then it will either default to the first item in the $whitelist_items + * (if $die_if_no_match is FALSE) or it will die() and send an error message to the screen + * and log (if $die_if_no_match is TRUE). + * The second option is done by sanitizing ($whitelist_items is not used) 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. + * @param string $s Sql identifier variable to be escaped/sanitized. + * @param array $whitelist_items Items used in whitelisting method (See function description for details of whitelisting method). + * @param boolean $die_if_no_match If there is no match in the whitelist, then die and echo an error to screen and log. + * @return string Escaped/sanitized sql identifier variable. */ -function escape_identifier($s,$whitelist_flag=FALSE,$whitelist_items) { - if ($whitelist_flag) { +function escape_identifier($s,$whitelist_items,$die_if_no_match=FALSE) { + if (is_array($whitelist_items)) { // Only return an item within the whitelist_items - // (if no match, then it will return the first item in whitelist_items) + if ( $die_if_no_match && !(in_array($s,$whitelist_items)) ) { + // There is no match in the whitelist and the $die_if_no_match flag is set + // so die() and send error messages to screen and log + error_Log("ERROR: OpenEMR SQL Escaping ERROR of the following string: ".$s,0); + die("
".xlt("There was an OpenEMR SQL Escaping ERROR of the following string")." ".text($s)."
"); + } $ok = $whitelist_items; $key = array_search($s,$ok); return $ok[$key]; } else { // Return an item that has been "cleaned" up - // (this is currently experimental) + // (this is currently experimental and goal is to avoid using this) return preg_replace('/[^a-zA-Z0-9_.]/','',$s); } } diff --git a/library/forms.inc b/library/forms.inc index a366dad77..3b8b13b8e 100644 --- a/library/forms.inc +++ b/library/forms.inc @@ -59,15 +59,20 @@ function addForm($encounter, $form_name, $form_id, $formdir, $pid, { if (!$user) $user = $_SESSION['authUser']; if (!$group) $group = $_SESSION['authProvider']; + + $arraySqlBind = array(); $sql = "insert into forms (date, encounter, form_name, form_id, pid, " . "user, groupname, authorized, formdir) values ("; - if($date == "NOW()") + if($date == "NOW()") { $sql .= "$date"; - else - $sql .= "'$date'"; - $sql .= ", '$encounter', '$form_name', '$form_id', '$pid', '$user', " . - "'$group', '$authorized', '$formdir')"; - return sqlInsert( $sql ); + } + else { + $sql .= "?"; + array_push($arraySqlBind,$date); + } + $sql .= ", ?, ?, ?, ?, ?, ?, ?, ?)"; + array_push($arraySqlBind,$encounter,$form_name,$form_id,$pid,$user,$group,$authorized,$formdir); + return sqlInsert($sql,$arraySqlBind); } function authorizeForm($id, $authorized = "1") diff --git a/library/pnotes.inc b/library/pnotes.inc index d8e6550c7..1ecb47518 100644 --- a/library/pnotes.inc +++ b/library/pnotes.inc @@ -47,9 +47,6 @@ 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 "; @@ -78,7 +75,7 @@ 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 ".escape_identifier($sortby,TRUE,$sortbyoptions). + $sql .= " order by ".escape_sql_column_name($sortby,array('users','patient_data','pnotes'),TRUE). " ".escape_sort_order($sortorder). " limit ".escape_limit($begin).", ".escape_limit($listnumber); } -- 2.11.4.GIT