Will this protect from SQL injection?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
User avatar
Sindarin
Forum Regular
Posts: 521
Joined: Tue Sep 25, 2007 8:36 am
Location: Greece

Will this protect from SQL injection?

Post by Sindarin »

When the user submits the form I replace the following characters (I use wysiwyg editor),
$string = str_replace("'",":apos;",$string);
$string = str_replace("`",":apos;",$string);
$string = str_replace('"',":quote;",$string);
$string = str_replace("%",":percnt;",$string);
$string = str_replace("\",":backsl;",$string);
$string = str_replace("/",":slash;",$string);
$string = str_replace("|",":macro;",$string);
$string = str_replace("*",":wildc;",$string);
$string = str_replace("$",":dollar;",$string);
$string = str_replace("<",":ltag;",$string);
$string = str_replace(">",":rtag;",$string);
$string = str_replace(";",":lineb;",$string);
$string = str_replace(",",":comma;",$string);
and when I display the entries, I replace back the characters so nothing gets erased, e.g. :backsl; becomes \

Are there any holes to this method?
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Will this protect from SQL injection?

Post by Mordred »

Will this protect from SQL injection?
No.

Reading this might help:
http://www.logris.org/security/the-unex ... -injection
User avatar
Sindarin
Forum Regular
Posts: 521
Joined: Tue Sep 25, 2007 8:36 am
Location: Greece

Re: Will this protect from SQL injection?

Post by Sindarin »

mysql_real_escape_string() does not work on my host. magic_quotes are on so I have to do it myself. Can you elaborate on what I am doing wrong in the above code?
User avatar
papa
Forum Regular
Posts: 958
Joined: Wed Aug 27, 2008 3:36 am
Location: Sweden/Sthlm

Re: Will this protect from SQL injection?

Post by papa »

Read Example #3. You can probably use something similar to this:

Code: Select all

 
 if(get_magic_quotes_gpc()) {
            $product_name        = stripslashes($_POST['product_name']);
            $product_description = stripslashes($_POST['product_description']);
        } else {
            $product_name        = $_POST['product_name'];
            $product_description = $_POST['product_description'];
        }
 
http://us.php.net/manual/ro/function.my ... string.php
User avatar
Sindarin
Forum Regular
Posts: 521
Joined: Tue Sep 25, 2007 8:36 am
Location: Greece

Re: Will this protect from SQL injection?

Post by Sindarin »

stripslashes
doesn't it just remove the slashes? What is more than just using str_replace/ereg to do it manually for all the illegal characters?
And what is more to mysql_real_escape_string() ?
User avatar
Eran
DevNet Master
Posts: 3549
Joined: Fri Jan 18, 2008 12:36 am
Location: Israel, ME

Re: Will this protect from SQL injection?

Post by Eran »

magic_quotes can also be set using ini_set() or through .htaccess, so it's very easy to turn off. Also, if the mysql extension is installed mysql_real_escape_string will be available. Make sure you are creating a database connection using mysql_connect, since it is needed for running mysql_real_escape_string.
User avatar
papa
Forum Regular
Posts: 958
Joined: Wed Aug 27, 2008 3:36 am
Location: Sweden/Sthlm

Re: Will this protect from SQL injection?

Post by papa »

As I said, read the example: :)

Code: Select all

 
<?php
 
if (isset($_POST['product_name']) && isset($_POST['product_description']) && isset($_POST['user_id'])) {
    // Connect
 
    $link = mysql_connect('mysql_host', 'mysql_user', 'mysql_password');
 
    if(!is_resource($link)) {
 
        echo "Failed to connect to the server\n";
        // ... log the error properly
 
    } else {
        
        // Reverse magic_quotes_gpc/magic_quotes_sybase effects on those vars if ON.
 
        if(get_magic_quotes_gpc()) {
            $product_name        = stripslashes($_POST['product_name']);
            $product_description = stripslashes($_POST['product_description']);
        } else {
            $product_name        = $_POST['product_name'];
            $product_description = $_POST['product_description'];
        }
 
        // Make a safe query
        $query = sprintf("INSERT INTO products (`name`, `description`, `user_id`) VALUES ('%s', '%s', %d)",
                    mysql_real_escape_string($product_name, $link),
                    mysql_real_escape_string($product_description, $link),
                    $_POST['user_id']);
 
        mysql_query($query, $link);
 
        if (mysql_affected_rows($link) > 0) {
            echo "Product inserted\n";
        }
    }
} else {
    echo "Fill the form properly\n";
}
?>
 
 
User avatar
Sindarin
Forum Regular
Posts: 521
Joined: Tue Sep 25, 2007 8:36 am
Location: Greece

Re: Will this protect from SQL injection?

Post by Sindarin »

I also found in the doc,
\x00, \n, \r, \, ', " and \x1a
Are those the only characters that can be used in an injection?

I just can't understand why mysql_real_escape_string() is considered best practice than doing it manually. In essence it just removes those characters while my method allows the necessary characters like quote or apostrophe to be encoded and decoded without harming the query therefore with no loss in content.
User avatar
Eran
DevNet Master
Posts: 3549
Joined: Fri Jan 18, 2008 12:36 am
Location: Israel, ME

Re: Will this protect from SQL injection?

Post by Eran »

First of all, it doesn't remove the characters - it only escapes them. Second, since it is a native function and compiled in C, it runs much faster than your multiple search and replace functions. Third, it is aware of the character encoding of the database connection. And fourth, binary data can only escaped with this function.

Why shouldn't you use it?
User avatar
Sindarin
Forum Regular
Posts: 521
Joined: Tue Sep 25, 2007 8:36 am
Location: Greece

Re: Will this protect from SQL injection?

Post by Sindarin »

First of all, it doesn't remove the characters - it only escapes them. Second, since it is a native function and compiled in C, it runs much faster than your multiple search and replace functions. Third, it is aware of the character encoding of the database connection. And fourth, binary data can only escaped with this function.
Okay, this answer seems sufficient. Thanks.
User avatar
Sindarin
Forum Regular
Posts: 521
Joined: Tue Sep 25, 2007 8:36 am
Location: Greece

Re: Will this protect from SQL injection?

Post by Sindarin »

Another question would be is there any way to make it more compact than below?

Code: Select all

 
$article_title=mysql_real_escape_string($article_title,$db_connection);
$article_content=mysql_real_escape_string($article_content,$db_connection);
$article_tags=mysql_real_escape_string($article_tags,$db_connection);
$article_date=mysql_real_escape_string($article_date,$db_connection);
$article_time=mysql_real_escape_string($article_time,$db_connection);
$article_shownews=mysql_real_escape_string($article_shownews,$db_connection);
$article_expirehome=mysql_real_escape_string($article_expirehome,$db_connection);
$article_previewimage=mysql_real_escape_string($article_previewimage,$db_connection);
$article_contentimage=mysql_real_escape_string($article_contentimage,$db_connection);
$article_redirect=mysql_real_escape_string($article_redirect,$db_connection);
$article_showhome=mysql_real_escape_string($article_showhome,$db_connection);
$article_importance=mysql_real_escape_string($article_importance,$db_connection);
$article_showrss=mysql_real_escape_string($article_showrss,$db_connection);
$article_expire_date=mysql_real_escape_string($article_expire_date,$db_connection);
$article_expire_time=mysql_real_escape_string($article_expire_time,$db_connection);
and finally, will the above protect against sql injection or do I have to do anything else?
Last edited by Sindarin on Fri Nov 21, 2008 6:03 am, edited 2 times in total.
User avatar
papa
Forum Regular
Posts: 958
Joined: Wed Aug 27, 2008 3:36 am
Location: Sweden/Sthlm

Re: Will this protect from SQL injection?

Post by papa »

Loop through your query.

My not so perfect example maybe can give you some ideas:

mySQLInsert("value1,value2", "value_table");

Code: Select all

 
    function mySQLInsert($values, $mySQLtable) {
        //DB connect
        $link = $this->dbConnect();
        //assoc arrays
        $cols = implode(",", array_keys($values));
        $vals = array_values($values);
        //Create MySQL query string
        foreach($vals as $val) {
            if(get_magic_quotes_gpc()) {
                $val = stripslashes($val);
            }
            //MySQL escape
            $query[] = mysql_real_escape_string($val, $link);
            switch($val) {
                case "NULL":
                $placeholder[] = "NULL";
                break;
                case is_int($val):
                $placeholder[] = "'%d'";
                break;
                default:
                $placeholder[] = "'%s'";
            }
        }
        $sql_values = implode(", ", $placeholder);
        //MySQL query
        $query = array_merge(array("INSERT INTO ".$mySQLtable." ({$cols}) VALUES ({$sql_values})"), $query);
        $query = call_user_func_array("sprintf", $query);
        if(!$result = @mysql_query($query)) {
            $this->errorMSG[] = "Query failed: " . mysql_error();
            return false;
            exit;   
        } else {
        return mysql_insert_id();
        } 
    }
alex.barylski
DevNet Evangelist
Posts: 6267
Joined: Tue Dec 21, 2004 5:00 pm
Location: Winnipeg

Re: Will this protect from SQL injection?

Post by alex.barylski »

Honestly just use PDO it intrinsically protects against SQL injection by both:

1. Escaping according to the driver you have selected
2. Automatically casting types

The latter I have confirmed by just testing passing it a PKID as a string and it still works so I assume PDO has the insight to know that field A is an integer and will convert accordingly. You also do not need to wrap string parameters in quotes so this backs up the claim that it uses native DB field type meta data to properly cast data.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Re: Will this protect from SQL injection?

Post by John Cartwright »

Typically you would create an array to group the related data.

Code: Select all

$article['title'] = 'foobar';
$article['content'] = 'this is the content';
$article['date'] = '01/01/2001';
 
//callback function
$article = array_map('mysql_real_escape_string', $article); 
 
//or a loop by reference
foreach ($article as &$value) {
   $value = mysql_real_escape_string($value, $db_connection);
}
 
//or loop by key
foreach ($article as $key => $value) {
   $article[$key] = mysql_real_escape_string($value, $db_connection);
}
User avatar
Sindarin
Forum Regular
Posts: 521
Joined: Tue Sep 25, 2007 8:36 am
Location: Greece

Re: Will this protect from SQL injection?

Post by Sindarin »

Ah, arrays. They always confuse me, but I have to learn them as they seem really essential.
Post Reply