Page 1 of 2

SQL Injection Prevention

Posted: Thu Aug 16, 2007 11:33 pm
by kkonline
Hi guys,

I have listed some points below[Related to SQL Injection Only]

1> Use of only escape string like mysql_real_escape_string() is necessary and sufficient.

2> You should never use htmlentities or htmlspecialchars as these are encodings for a specific document format. You could not do searching or matching on that data and displaying it in any other document format would take extra code, and could not be done directly from the database.

3> Use of trim() may be used if no spaces in beginning or end are required.

4> You should never need to use stripslashes on output from a database. Aside from security, escaping quotes for instance, is solely for the benefit of the SQL parser. This is no different than escaping quotes in PHP strings, in order to not confuse the PHP parser and get an error. If you echo the string, it will not contain the escape characters, they are there solely to tell the parser what language characters are intended to be part of the string, and which ones are to be interpreted as actual code. The SQL parser in a database is no different.

5> Magic quotes and addslashes are vulnerable to character set differences and should not be used. If you can't turn it off, run code to defeat it, which is the proper use of stripslashes.

There are two codes which i found on the forum please compare them and tell which one is better? Looking at only the input data which is sent to db.

What should be the code/precaution when extracting data from db and displaying them. code for them???

All the above and below things are with respect to sql injection.
Please also discuss the above points and let me know if i have something wrong in above 5 points

Code1

Code: Select all

function cleaner($input)
{
    if(is_array($input))
    {
        $ret = array();
        foreach($input as $key=>$value)
        {
            $ret[$key] = cleaner($value);
        }
        return $ret;
    }
    else
    {
        if(!is_numeric($input))
        {
            if(get_magic_quotes_gpc($input))
            {
                $input = stripslashes($input);
            }
            $input = mysql_real_escape_string($input);
        }
        return $input;
    }
}
$dbData = cleaner($_POST);

why do we have if(get_magic_quotes_gpc($input))
and not if(get_magic_quotes_gpc()) ?

Code 2

Code: Select all

function safeEscapeString($string){

       if (get_magic_quotes_gpc()) {

             $string = stripslashes($string); 



         //defeating magic quote is required! 



             return $string;

       } else {

          return mysql_real_escape_string($string);

       }

  }

  function cleanVar($string){

      $string = trim($string);

      $string = safeEscapeString($string);

      $string = strip_tags($string); 

    

// [b]i think strip_tags should be removed[/b]



      return $string;

  }

  foreach($_POST as $name => $value){

      $_POST[$name] = cleanVar($value);

  } 

  foreach($_GET as $name => $value){

      $_GET[$name] = cleanVar($value);

  } 

  foreach($_COOKIE as $name => $value){

      $_COOKIE[$name] = cleanVar($value);

  } 

  foreach($_REQUEST as $name => $value){

      $_REQUEST[$name] = cleanVar($value);

  }
[s]Pls[/s] please let me know if the above points i summarized are correct.

I have been on this forum for past 5-6 days and I must say that all your prompt replies with elaborate explanation really helping enthusiasts and increases level of confidence in them. Hats off to you.

Waiting for a reply which covers all my doubts...

If there are some loopholes in the above code then can you tell me the easiest and the most secure code against sql injection.
[url=http://forums.devnetwork.net/viewtopic.php?t=30037]Forum Rules[/url] Section 1.1 wrote:11. Please use proper, complete spelling when posting in the forums. AOL Speak, leet speak and other abbreviated wording can confuse those that are trying to help you (or those that you are trying to help). Please keep in mind that there are many people from many countries that use our forums to read, post and learn. They do not always speak English as well as some of us, nor do they know these aberrant abbreviations. Therefore, use as few abbreviations as possible, especially when using such simple words.

Some examples of what not to do are ne1, any1 (anyone); u (you); ur (your or you're); 2 (to too); prolly (probably); afaik (as far as I know); etc.

Posted: Fri Aug 17, 2007 5:47 am
by Mordred
2-5 are correct.
1 is not: mysql_real_escape_string() is necessary but not sufficient. I have written an article on the subject which awaits publication soon, but in short, you also need to write correctly quoted SQL and not do a couple of silly things coders sometimes do.

Both code snippets you posted are wrong, the first assumes that $input comes from one of the gpc arrays, while the second apart from not handling arrays also has other problems which you've already noticed.

Posted: Fri Aug 17, 2007 10:07 am
by kkonline
Mordred wrote:2-5 are correct.
1 is not: mysql_real_escape_string() is necessary but not sufficient.

Both code snippets you posted are wrong, the first assumes that $input comes from one of the gpc arrays, while the second apart from not handling arrays also has other problems which you've already noticed.
I would like you to see another snippet i got from the a friend.
Please verify it against different attacks/injection attacks.

Code: Select all

<?php
function sql_safe($value, $allow_wildcards = false, $detect_numeric = false){
    // Taken from the PHP site and modified for wildcards and automatic formatting for numbers/strings.
   // Reverse magic_quotes_gpc/magic_quotes_sybase effects on those vars if ON.
   if (get_magic_quotes_gpc()){
        if(ini_get('magic_quotes_sybase')){
            $value = str_replace("''", "'", $value);
            }else{
            $value = stripslashes($value);
            }
        }

    // Escape wildcards for SQL injection protection on LIKE, GRANT, and REVOKE commands.
   if (!$allow_wildcards){
        $value = str_replace('%', '\%', $value);
        $value = str_replace('_', '\_', $value);
        }

    // Quote if $value is a string and detection enabled.
   if ($detect_numeric){
        if (!is_numeric($value)){
            return mysql_real_escape_string($value);
            }
        }

    return mysql_real_escape_string($value);
    }
 $var = " ' ";
var_dump(sql_safe($var));
?>
And secondly where can I a correct code for preventing sql injection attack/xss injection and other attacks and not corrupting user data too!

If you suggest a snippet i would be happy.

Posted: Fri Aug 17, 2007 10:18 am
by superdezign
You can't expect to handle all data through one function. You have to treat each piece of data as it's own.

Posted: Fri Aug 17, 2007 10:32 am
by kkonline
superdezign wrote:You can't expect to handle all data through one function. You have to treat each piece of data as it's own.
Ok fine first about sql injection please verify the code i have posted if it is good enough or has loopholes!

Posted: Fri Aug 17, 2007 10:36 am
by superdezign
kkonline wrote:Ok fine first about sql injection please verify the code i have posted if it is good enough or has loopholes!
Magic quotes should not be handled in the function. All data that you escape for your queries does not need to come from user input.

Posted: Fri Aug 17, 2007 11:51 pm
by kkonline
superdezign wrote: Magic quotes should not be handled in the function.
Can you please explain the drawback of handling magic quotes in function.

So according to you what changes should i make to the following code or you'll suggest some other code.

Code: Select all

<?php
function sql_safe($value, $allow_wildcards = false, $detect_numeric = false){
    // Taken from the PHP site and modified for wildcards and automatic formatting for numbers/strings.
   // Reverse magic_quotes_gpc/magic_quotes_sybase effects on those vars if ON.
   if (get_magic_quotes_gpc()){
        if(ini_get('magic_quotes_sybase')){
            $value = str_replace("''", "'", $value);
            }else{
            $value = stripslashes($value);
            }
        }

    // Escape wildcards for SQL injection protection on LIKE, GRANT, and REVOKE commands.
   if (!$allow_wildcards){
        $value = str_replace('%', '\%', $value);
        $value = str_replace('_', '\_', $value);
        }

    // Quote if $value is a string and detection enabled.
   if ($detect_numeric){
        if (!is_numeric($value)){
            return mysql_real_escape_string($value);
            }
        }

    return mysql_real_escape_string($value);
    }
 $var = " ' ";
var_dump(sql_safe($var));
?>
Code 2

I have written the code below manually then what do you have to say about it.

Code: Select all

function clean($i){
if (!get_magic_quotes_gpc()){
$i = addslashes($i);
}
$i = rtrim($i);
$look = array('&', '#', '<', '>', '"', '\'', '(', ')');
$safe = array('&', '&#35', '<', '>', '"', '&#39', '(', ')');
$i = str_replace($look, $safe, $i);
return $i;
}
I just want sql injection to be prevented, without corrupting the data which goes into the db.

Posted: Sat Aug 18, 2007 9:01 am
by superdezign
kkonline wrote:Can you please explain the drawback of handling magic quotes in function.
I already did.

kkonline wrote:So according to you what changes should i make to the following code or you'll suggest some other code.
Do you not pay attention? Handle the data as I needs to be handled in the context that it is used, not through some "does-it-all" function. You are bound to run into errors, or just waste your time.

kkonline wrote:I have written the code below manually then what do you have to say about it.

Code: Select all

function clean($i){
if (!get_magic_quotes_gpc()){
$i = addslashes($i);
}
$i = rtrim($i);
$look = array('&', '#', '<', '>', '"', '\'', '(', ')');
$safe = array('&', '&#35', '<', '>', '"', '&#39', '(', ')');
$i = str_replace($look, $safe, $i);
return $i;
}
What do I have to say about it? It fakes a bit of mysql_escape_string() and performs a pseudo-htmlentities(). It's generally useless, as you should never need to do both of those at the same time anyway.

kkonline wrote: I just want sql injection to be prevented, without corrupting the data which goes into the db.
Then stop giving these wasteful solutions. mysql_real_escape_string() going into the database, htmlspecialschars() coming out.

Posted: Sat Aug 18, 2007 9:14 am
by kkonline
superdezign wrote: Then stop giving these wasteful solutions. mysql_real_escape_string() going into the database, htmlspecialschars() coming out.
You mean to use mysql_real_escape_string() on every value that goes INTO the database and htmlspecialschars() to every value that COMES OUT of the db and is displayed; correct ? I am a newbie so just clarifying my mind!

Posted: Sat Aug 18, 2007 9:35 am
by superdezign
kkonline wrote:You mean to use mysql_real_escape_string() on every value that goes INTO the database
Yes. Otherwise, you're prone to SQL injection.
kkonline wrote:and htmlspecialschars() to every value that COMES OUT of the db and is displayed
Only if you don't want to allow HTML formatting in the output.

Posted: Sat Aug 18, 2007 9:58 am
by jmut
In short:

1. All data used in sql query should be mysql_real_escape_string ...if mysql is the DB you use of course. Another alternative is using prepared queries - supporeted from mysql 4.1+ and most other DBMS. And this is 100% enough to prevent sql injection. If it is not then it is php/mysql bug
and not your fault.

2. Not sure what Mordred said by not sufficient - I guess he meant the magic quotes thing. That for your code to be portable it should take
it into consideration. Usually this is cleared with one function called in beginning of request to clear $_POST , $_GET, $_COOKIES, $_FILES super globals from slashes in case magic quotes is on.

In that sense I personally use stripslashes() only that time...and addslashes() never ever!

All other functions are well commented as useless.
Have fun.

Posted: Sat Aug 18, 2007 10:06 am
by superdezign
jmut wrote:2. Not sure what Mordred said by not sufficient - I guess he meant the magic quotes thing.
Well, mysql_real_escape_string fights general SQL injection. There are things that could happen that is beyond the scope of the function, like a user using '%' when your SQL uses 'LIKE.' That's something that the function can't fight because it can't decide whether or not you allow that, and because it alters the way the SQL is performed (unless you allow it), it is, indeed, SQL injection.

Posted: Sat Aug 18, 2007 10:13 am
by kkonline
superdezign wrote:
kkonline wrote:You mean to use mysql_real_escape_string() on every value that goes INTO the database
Yes. Otherwise, you're prone to SQL injection.
kkonline wrote:and htmlspecialschars() to every value that COMES OUT of the db and is displayed
Only if you don't want to allow HTML formatting in the output.
I really appreciate the straightforward way you gave the reply. It becomes very clear to newbies.

One last doubt it have for Mr. superdezign reply is :

Is it required to check condition of magic quotes (on/off)?

If on then use stripslashes() to reverse it's effect

If it is off then directly mysql_real_escape_string()


Or we can do prevention without thinking about the magic quote condition.

Please be specific as you were in the last post; i'll appreciate that.

Posted: Sat Aug 18, 2007 10:21 am
by superdezign
Magic quotes should be handle separately.

Posted: Sat Aug 18, 2007 10:48 am
by kkonline
superdezign wrote:Magic quotes should be handle separately.
Can you please elaborate a little on what you mean and how?