views:

155

answers:

2

I've read various sources but I'm unsure how to implement them into my code. I was wondering if somebody could give me a quick hand with it? Once I've been shown how to do it once in my code I'll be able to pick it up I think! This is from an AJAX autocomplete I found on the net, although I saw something to do with it being vulnerable to SQL Injection due to the '%$queryString%' or something? Any help really appreciated!

if ( isset( $_POST['queryString'] ) )
{
  $queryString = $_POST['queryString'];
  if ( strlen( $queryString ) > 0 )
  {
    $query = "SELECT game_title, game_id FROM games WHERE game_title LIKE '%$queryString%' || alt LIKE '%$queryString%' LIMIT 10";
    $result = mysql_query( $query, $db ) or die( "There is an error in database please contact [email protected]" );
    while ( $row = mysql_fetch_array( $result ) )
    {
      $game_id = $row['game_id'];
      echo '<li onClick="fill(\'' . $row['game_title'] . '\',' . $game_id . ');">' . $row['game_title'] . '</li>';
    }
  }
}
+7  A: 

The injection vulnerability is that you're passing user supplied data straight into a query without sanitizing it. In particular, this line is problematic:

$queryString = $_POST['queryString'];  

If you use the function mysql_real_escape_string() around $_POST['queryString'], that will prevent users from being able to insert their own code.

$queryString = mysql_real_escape_string($_POST['queryString']); 
davethegr8
ahh its that simple? Thanks loads - was expecting it to be something really complicated :)
Craig Whitley
Right answer but wrong point of view. User supplied data does not need any "sanitisation". The purpose of mysql_real_escape_string() is merely delimiter escaping. So, any data we enclose in quotes, must be processed with mysql_real_escape_string(), no matter it's user input or anything else.
Col. Shrapnel
@Craig yeah, SQL injections is more rumors than real. If your syntax is correct, nothing can harm you. And mysql_real_escape_string() is part of syntax you must always follow. But it can help with data only. For the other parts of query there is something more complicated
Col. Shrapnel
A: 

Use mysql_real_escape_string() on all values from untrusted sources before concatenating the value into the query string. (As a general rule, if you didn't hard code the value into the query string, escape it). For example:

$queryString = mysql_real_escape_string($_POST['queryString']);
$query =  "SELECT game_title, game_id "
        . "FROM games "
        . "WHERE game_title LIKE '%".$queryString."%'" 
        . "|| alt LIKE '%".$queryString."%' "
        . "LIMIT 10";

It is often easier to use a mysql adaptor that supports prepared statements which makes forgetting to sanitize input a lot harder. For example PHP has pdo or mysqli

Yacoby
This is a worthy answer, but i gave the +1 to davethegr8 because of DRY.
Peter Bailey
Actually mysql_real_escape_string() should be used for the every value treated as a string, no matter what source. It is sintax issue, not "protection".
Col. Shrapnel
@Peter yeah, davethegr8's explanation is far better and definitely deserves the +1. Half my answer is annoyance at the way people format SQL.
Yacoby
@Yacoby column names DO cause SQL injection. And you don't escape it not because won't cause a SQL injection, but because SQL syntax doesnt allow quotes around identifiers.
Col. Shrapnel
@Col. (Clarified version of previous comment) I agree that untrusted sources is a bit vague. I basically meant that there is no need to escape the *hardcoded* column names as I know they won't cause a SQL injection.
Yacoby
@Col. So as column names can contain special characters (See the MySQL manual) they should be run through `mysql_real_escape_string`? It is protection to prevent SQL injection.
Yacoby
@Yacoby oh no. You've got everything wrong. column names added to the query *dynamically*, e.g. `SELECT * FROM table ORDER BY $columd` **are** danger. And mysql_real_escape_string won't help with it. Hardcoded column names are safe, as any other static part of query.
Col. Shrapnel
@Col. I was just clarifying. I think we were misunderstanding each other. :)
Yacoby