Are you still worried about sql injection ?

2008-05-17

Some tips on how to get rid of sql injection forever.

Firstly, I want to tell you what sql injection is and why it exists and then I will show you how to get rid of this problem forever. Sql injection exists because sql queries that are generated by scripts/programs concatenate user input to the query and not all user input is safe.

For example, consider the following query:

SELECT * FROM users WHERE user = 'John'

If you want to get a row containing user information that a user entered you must replace "John" with the string provided by the user.

In php, it looks like this:

"SELECT * FROM users WHERE user = '$_GET[userName]'"

The problem is simple: if the user enters a specially formated string as a user name then he can modify your query to execute whatever he wishes.

For example, he can enter:

';DROP TABLE users;--

Now, the query will look like this:

SELECT * FROM users WHERE user = '';DROP TABLE users;--'

There are numerous ways in which someone can exploit your database trough sql injection. There are some rudimentary solutions like escaping ' charachters, but they don't always work because there are other ways to alter the query. I won't dive into this now.

There is a very simple solution to this problem. Don't concatenate user input to the query, bind it. Almost all databases support variable binding, mysql has it, oracle has it, postgresql has it and the list can go on. The idea is simple: you provide a placeholder in your query that will get replaced by the actual variable, so no manipulation of the query is possible.

Our previous query can be written like this:

Mysql (in php with the mysqli extension)

SELECT * FROM users WHERE user = ?

Oracle

SELECT * FROM users WHERE user = :user

PHP mysqli extension example (for more information read mysqli_stmt bind_param function

/* get user input */
$userName = $_GET['userName'];
/* prepare the query */
$stmt = $mysqli->prepare('SELECT * FROM users WHERE user = ?');
/* bind the value */
$stmt->bind_param('s', $userName);
/* execute prepared statement */
$stmt->execute();

$stmt->bind_result($name, $surname);
/* fetch user data*/
while ($stmt->fetch()) 
{
  //output name and surname
  echo $name, ' =>  ', $surname , "\n";
}
/* close statement */
$stmt->close();

PHP PDO extension example (for more information read PDO prepare function

/* get user input */
$userName = $_GET['userName'];
/* prepare the query */
$sth = $dbh->prepare('SELECT * FROM users WHERE user = ?');
/* bind and execute prepared query */
$sth->execute($userName);
$row = $sth->fetchAll();

So is easy for us now to avoid query rewriting by avoiding user input to be concatenated to the query, but there are scenarios where you must generate your query based on user input, not just sending data with the query. For example, you want to display a different type of books based on user input. The user can choose to view science fiction books, literature or cooking books.

You may be tempted to write something like this:

SELECT * FROM $_GET[bookCategory]

But, what if the user tries to modify your url/html page/etc and enters 'users' as a category? Then he will be able to view data from users table. The solution to this problem is to check the category provided by the user instead of using the string directly.

In php you may do it like this:

switch ($_GET['bookCategory'])
{
	case 'sf_books':
		$table = 'sf_books';
		break;
	case 'literature_books':
		$table = 'literature_books';
                break; 
	default:
		$table = 'cooking_books';
}

$query = "SELECT * FROM $table";

If you write your script this way, when the user enters 'users' as a category, he will see cooking books, or you can choose to display an error message, that's your choice.

To summarize this article, I would like to say that you should use variable binding when you want to send data to the database that came from user input. And if you construct your queries based on user input, always compare user input to some predefined values and use the default value.

Never concatenate user input to your queries, without exceptions.

Share this with the world

Related

Comments

Konr Ness

That last bit of code with the switch statement has bug. You missed the break; statement after the 'literature_books' case. Without it, $table will be 'cooking_books' even if 'literature_books' was specified.

Posted on 2008-05-21 17:59:07
slink

Probably you missed a "break" in case 'literature_books'.

Posted on 2008-05-21 12:45:39
Wabbitseason

or ...

switch ($_GET['bookCategory'])
{
case 'sf_books':
case 'literature_books':
$table = $_GET['bookCategory'];
break;
default:
$table = 'cooking_books';
}
$query = "SELECT * FROM $table";

or rather:

$allowed = array('cooking_books', 'sf_books', 'literature_books');
if (@in_array($_GET['bookCategory'], $allowed)) {
$table = $_GET['bookCategory'];
} else {
$table = current($allowed);
}
$query = "SELECT * FROM $table";

Posted on 2008-05-21 13:35:16
Jakub Kulhan

The last example (using user input as table name) is non-sens. With good database design you'll never get to this situation. For example suppose that we have these tables: books (id, name, type_id) and types (id, name). And here you can use bindable value, again, to choose type of book:

SELECT * FROM books LEFT JOIN types ON books.type_id = types.id WHERE type.name = ?

I think that it's more secure and for unknown type it won't return anything, which is more expectable.

Posted on 2008-05-20 19:02:27
CodeAssembly

Jakub Kulhan, this is just an example to show that query generation using user input is dangerous.
It's not trying to be an example on how to design your database.

@Konr Ness and @slink thanks for the tip, I forgot to put that break.
@Wabbitseason, you can write your code any way you want, the important thing is not to slip any user input in the query.

Posted on 2008-05-21 19:22:45
Jansen Price

Wouldn't it be easier to do that last bit of code's switch statement thusly:

switch ($_GET['bookCategory'])
{
case 'sf_books':
case 'literature_books':
$table = $_GET['bookCategory'];
break;
default:
$table = 'cooking_books';
}

Posted on 2008-05-22 05:43:29
CodeAssembly

Is safer to put the default value instead of user input because PHP has some problems with UTF8 and the string comparison can be true but the string can be slightly different.

Posted on 2008-05-22 09:19:56
Jasper

You could just use PDO, which automatically escapes the data you pass to it.

Posted on 2008-05-22 12:24:11
Kobra

If SQL Injection works on a website in this day and age, it DESERVES to get hacked. Seriously.

Posted on 2008-05-22 21:36:20
Malcolm

I am still amazed that programmers construct interactions with databases using SQL/Mysql/Oracle/postgresql etc.

Do not do it. Your job is to control/direct the user experience. Use DB2/400 and stop messing about.



Posted on 2008-05-22 20:03:27
buzzknow

use prepare for query its best pratice for all newbie to avoid sql injection :)

Posted on 2010-08-11 19:38:45

Make yourself heard

Categories

Subscribe

All Posts

All Comments

© Copyright CodeAssembly

All code is licensed under LGPL, unless otherwise noted

littlebubu