How can I protect this code from SQL Injection? A bit confused

Question

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>';
    }
  }
}

I'm not a php whiz so I won't 'answer' but this is vulnerable because you aren't sanitizing your input. Basically, user text is being written directly into the query which would allow them to write something like " '; drop table games;" and poof! You table would disappear. You need to run it through one of php's built-in mysql sanitizing functions, it has escape in the name, I just can't htink of exactly what it is. — Chris Thompson
– Chris Thompson, Commented Mar 31, 2010 at 18:01
@Chris Thompson: That sounds like an answer. Please post it as an answer and delete this comment. Then we can upvote your answer properly. — S.Lott
– S.Lott, Commented Mar 31, 2010 at 18:04
Use parameterized queries. You may get a performance improvement over dynamic sql due to utilization of cached execution plan and also don't have to worry about sanitization - us3.php.net/manual/en/mysqli.prepare.php — Chetan S
– Chetan S, Commented Mar 31, 2010 at 18:22
@S. Lott I agree although I'm torn because it is an explanation of what the problem is, I don't have a good answer as to how to specifically address the vulnerability (i.e. I don't remember the function name) — Chris Thompson
– Chris Thompson, Commented Mar 31, 2010 at 18:34

Anthony Forloney · Accepted Answer · 2010-03-31 18:01:49Z

7

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']);

edited Mar 31, 2010 at 18:01

Anthony Forloney

92k14 gold badges119 silver badges116 bronze badges

answered Mar 31, 2010 at 18:00

davethegr8

11.7k5 gold badges41 silver badges61 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Kiada Over a year ago

ahh its that simple? Thanks loads - was expecting it to be something really complicated :)

Your Common Sense Over a year ago

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.

Your Common Sense Over a year ago

@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

Yacoby · Accepted Answer · 2010-03-31 18:16:07Z

1

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

edited Mar 31, 2010 at 18:16

answered Mar 31, 2010 at 18:01

Yacoby

55.6k16 gold badges119 silver badges121 bronze badges

8 Comments

Peter Bailey Over a year ago

This is a worthy answer, but i gave the +1 to davethegr8 because of DRY.

Your Common Sense Over a year ago

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".

Yacoby Over a year ago

@Peter yeah, davethegr8's explanation is far better and definitely deserves the +1. Half my answer is annoyance at the way people format SQL.

Your Common Sense Over a year ago

@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.

Yacoby Over a year ago

@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.

|

Collectives™ on Stack Overflow

How can I protect this code from SQL Injection? A bit confused

2 Answers 2

3 Comments

8 Comments

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

8 Comments

Linked

Related