Sql-injection functions and techniques for escaping(take 3):
authorbradymiller <bradymiller@users.sourceforge.net>
Sat, 2 Mar 2013 07:45:37 +0000 (1 23:45 -0800)
committerbradymiller <bradymiller@users.sourceforge.net>
Tue, 5 Mar 2013 03:12:47 +0000 (4 19:12 -0800)
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)

interface/globals.php
library/formdata.inc.php
library/pnotes.inc

index b1f07e3..078df47 100644 (file)
@@ -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");
 
index fa4706c..21c348d 100644 (file)
 <?php
 /**
+ * Functions to globally validate and prepare data for sql database insertion.
+ *
  * Copyright (C) 2009 Rod Roark <rod@sunsetsystems.com>
  *
- * 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 <http://opensource.org/licenses/gpl-license.php>;.
+ *
+ * @package OpenEMR
+ * @author  Rod Roark <rod@sunsetsystems.com>
+ * @author  Brady Miller <brady@sparmy.com>
+ * @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);
 }
index 5b6e265..d8e6550 100644 (file)
@@ -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;