Am I saving myself from sql injections?

Question

Am I doing this right? Will this help avoid sql injections?

$deleteid = htmlspecialchars(strip_tags(mysql_real_escape_string($_POST['listid'])));

mysql_send("DELETE FROM stage where listid='$deleteid'");

Answer 1

Yes, using mysql_real_escape_string on values that are intended to be used in string declarations in MySQL statements will prevent you from SQL injections. That’s the exact purpose of that function.

But you don’t need the other two functions strip_tags and htmlspecialchars. Because these functions are used to either remove (HTML) tags and replace the HTML special characters by character references respectively. They are not intended to protect you against SQL injections.

In fact, using strip_tags and/or htmlspecialchars after mysql_real_escape_string could break the escaping under some certain instances (e.g. when using non-US-ASCII based character sets, see also addslashes() Versus mysql_real_escape_string()). So make sure that you use that function right before inserting its returned value into the SQL statement.

Apart from encoding the output using mysql_real_escape_string you could also validate the input using ctype_digit:

if (ctype_digit($_POST['listid'])) {
    mysql_send("DELETE FROM stage where listid='".$_POST['listid']."'");
} else {
    // invalid value
}

This validation ensures that only (positive) integer values are used in the query that don’t need to be escaped.

Answer 2

No.

You should call nothing but mysql_real_escape_string.

The htmlspecialchars and strip_tags functions are used to encode strings to be displayed as HTML.
They should not be used with SQL

Answer 3

It may prevent SQL injection attacks, but its a poor way to approach it. Use prepared queries instead.

Since your comment says you're systematically making changes to your whole site, go with the better approach. While you're at it, you may want to move to a non-MySQL-specific database API, in case you want to switch to another backend later.

Answer 4

Apart from the mentioned suggestions (only mysql_real_escape_string but even better prepared statements), I would like to add that it is always useful to analyse exactly the value you are trying to clean / make safe.

If your ID is supposed to be an integer, I would simply use intval($_POST['listid']) to make sure the result is an integer and reserve mysql_real_escape_string for strings (although I personally would use prepared statements / PDO).

Answer 5

use stored procedures and grant execute permissions only to your app db user (plus the myriad of other benefits sprocs bring)

Answer 6

I always use a database helper class on all my projects.. that way I don't have to use mysql_real_escape_string keeping code clean and easy to read you'll be safe from injections if you forget to use it.

Below is an example the one I use...

<?php 
    final class DatabaseException extends Exception {
        function __construct($strErrMessage, $intErrCode) {
            parent::__construct($strErrMessage, $intErrCode);   
        }
    }

    class Database {
        protected $host     = ""; //database server
        protected $user     = ""; //database login name
        protected $pass     = ""; //database login password
        protected $database = ""; //database name
        protected $prefix   = ""; //table prefix        
        protected $connected = false;
        protected $db = null;   
        protected $record = array();
        protected $error = "";
        protected $errno = 0;

                //table name affected by SQL query
        protected $field_table= "";

        //number of rows affected by SQL query
        protected $affected_rows = 0;

        protected $link_id = 0;
        protected $query_id = array(); 

        function __construct($server, $user, $pass, $database, $pre='') {
            $this->connected = false;
            $this->host = $server;
            $this->user = $user;
            $this->pass = $pass;
            $this->database = $database;
            $this->prefix = $pre;
            $this->connect();
        }

        function __destruct() {
            //mysql_close($this->link_id); 
        }

        public function connect() {
            if ($this->link_id > 0 && $this->connected && mysql_ping($this->link_id)) { return; }

            $this->link_id = mysql_pconnect($this->host, $this->user, $this->pass);
            if (!$this->link_id) { //open failed
                throw new DatabaseException("mysql_pconnect failed",0);    
            }

            if(!@mysql_select_db($this->database, $this->link_id)) {//no database
                throw new DatabaseException("mysql_select_db failed",0); 
            }
            $this->server='';
            $this->user='';
            $this->pass='';
            $this->database=''; 
            $this->connected = true;
            $this->query("SET time_zone = '".Settings::get('db.timezone_offset')."';",TRUE);
        }

        public function escape($string) {
            if(get_magic_quotes_gpc()) 
                $string = stripslashes($string);
            return mysql_real_escape_string($string);
        }

        public function insert($table,$data,$tbl_key='id') {
            $v=''; 
            $n='';
            foreach($data as $key=>$val) {
                $n.="`$key`, ";
                if(strtolower($val)=='null') 
                    $v.="NULL, ";
                elseif(strtolower($val)=='now()') 
                    $v.="NOW(), ";
                elseif(strcmp(substr($val,0,7),'**ESC**') == 0) 
                    $v .= str_replace('**ESC**','',$val);
                else 
                    $v.= "'".$this->escape($val)."', ";
            }

            if ($v=='' || $n=='') 
                return false;

            $q  = "INSERT INTO `".$this->prefix.$table."` ";
            $q .= "(". rtrim($n, ', ') .") VALUES (". rtrim($v, ', ') .");";
            if($this->query($q)){
                $id=mysql_insert_id();
                if ($id === 0) {  // The ID generated for an AUTO_INCREMENT column by the previous INSERT query on success, 
                                  // 0 if the previous query does not generate an AUTO_INCREMENT value, or FALSE if no MySQL 
                                  // connection was established.
                    return TRUE;
                } 
                return $id;
            }
            else {
                return false;
            }
        }

        public function replace($table,$data,$tbl_key='id') {
            $v=''; 
            $n='';
            foreach($data as $key=>$val) {
                $n.="`$key`, ";
                if(strtolower($val)=='null') 
                    $v.="NULL, ";
                elseif(strtolower($val)=='now()') 
                    $v.="NOW(), ";
                elseif(strcmp(substr($val,0,7),'**ESC**') == 0) 
                    $v .= str_replace('**ESC**','',$val);
                else 
                    $v.= "'".$this->escape($val)."', ";
            }

            if ($v=='' || $n=='') 
                return false;

            $q  = "REPLACE INTO `".$this->prefix.$table."` ";
            $q .= "(". rtrim($n, ', ') .") VALUES (". rtrim($v, ', ') .");";

            if($this->query($q)){
                $id=mysql_insert_id();
                if ($id === 0) {  // The ID generated for an AUTO_INCREMENT column by the previous INSERT query on success, 
                                  // 0 if the previous query does not generate an AUTO_INCREMENT value, or FALSE if no MySQL 
                                  // connection was established.
                    return TRUE;
                } 
                return $id;
            }
            else {
                return false;
            }
        }

        public function update($table,$data,$where='1') {
            $q = "UPDATE `".$this->prefix.$table."` SET ";
            foreach($data as $key=>$val) {
                if(strtolower($val)=='null') $q .= "`$key` = NULL, ";
                elseif(strtolower($val)=='now()') $q .= "`$key` = NOW(), ";
                elseif(strcmp(substr($val,0,7),'**ESC**') == 0) $q .=  "`$key` = ".str_replace('**ESC**','',$val);
                else $q.= "`$key`='".$this->escape($val)."', ";
            }
            $q = rtrim($q, ', ') . ' WHERE '.$where.';';
            $result = $this->query($q); 

            if ($result) {
            }
            return $result;
        }

        public function search($table, $field, $value, $exact=FALSE)
        {
                    $value = escape($value);
            if (!$exact) {      
                $q = "select * from $table where $field like '%$value%';";
            } else {
                $q = "select * from $table where $field = '$value';";
            }
            return $this->query($q);
        }

        public function delete($table,$where='1') {
            $q  = "DELETE FROM `".$this->prefix.$table."` ";
            $q .= " WHERE ".$where.";";
            $result = $this->query($q);             

            if ($result) {
            }
        }

        public function query($sql,$reset=FALSE) {
            //echo "<pre>$sql</pre>";
            $this->connect();
            $command = strtok(trim($sql)," \n\t");
            switch (strtoupper(trim($command))) {
                case "SELECT":
                case "SHOW":
                case "DESCRIBE":
                case "EXPLAIN":
                    if (isset($this->query_id[md5($sql)]) && $reset==FALSE) {
                        $row = mysql_fetch_array($this->query_id[md5($sql)], MYSQL_ASSOC);
                        if ($row == FALSE) {
                            unset($this->query_id[md5($sql)]);
                            return FALSE;
                        } else {
                            return $row;
                        }
                    } else {    
                        $this->query_id[md5($sql)] = @mysql_query($sql, $this->link_id);
                        if (!$this->query_id[md5($sql)]) {
                            throw new DatabaseException(mysql_error($this->link_id),mysql_errno($this->link_id));
                        }
                    }
                    $row = mysql_fetch_array($this->query_id[md5($sql)], MYSQL_ASSOC);
                    if ($row == FALSE) {
                        unset($this->query_id[md5($sql)]);
                        return FALSE;
                    } else {
                        return $row;
                    }
                    break;
                default:
                    return @mysql_query($sql, $this->link_id);
                    break;
            }
        }
    }
?>

To create and use the Database Class:

$db = new Database("db.host","db.user","db.pass","db.database");

Getting data from $_POST into your db is super simple if all your form elements are named the same as your table fields.. for example:

$data = $_POST;
$ok = $db->update('mytable', $data, 'something = something_else'); //$ok will be false if something went wrong

Answer 7

in this case

mysql_query("DELETE FROM stage WHERE listid=".intval($_POST['listid']));

full reference: dynamical SQL syntax explained