How is this MySQL query vulnerable to SQL injection?

Question

In a comment on a previous question, someone said that the following sql statement opens me up to sql injection:

select
    ss.*,
    se.name as engine,
    ss.last_run_at + interval ss.refresh_frequency day as next_run_at,
    se.logo_name    
from 
    searches ss join search_engines se on ss.engine_id = se.id
where
    ss.user_id='.$user_id.'
group by ss.id
order by ss.project_id, ss.domain, ss.keywords

Assuming that the $userid variable is properly escaped, how does this make me vulnerable, and what can I do to fix it?

Answer 1

Assuming it is properly escaped, it doesn't make you vulnerable. The thing is that escaping properly is harder than it looks at first sight, and you condemn yourself to escape properly every time you do a query like that. If possible, avoid all that trouble and use prepared statements (or binded parameters or parameterized queries). The idea is to allow the data access library to escape values properly.

For example, in PHP, using mysqli:

$db_connection = new mysqli("localhost", "user", "pass", "db");
$statement = $db_connection->prepare("SELECT thing FROM stuff WHERE id = ?");
$statement->bind_param("i", $user_id); //$user_id is an integer which goes 
                                       //in place of ?
$statement->execute();

Answer 2

Every SQL interface library worth using has some kind of support for binding parameters. Don't try to be clever, just use it.

You may really, really think/hope you've escaped stuff properly, but it's just not worth the time you don't.

Also, several databases support prepared statement caching, so doing it right can also bring you efficiency gains.

Easier, safer, faster.

Answer 3

If it is properly escaped and validated, then you don't have a problem.

The problem arises when it is not properly escaped or validated. This could occur by sloppy coding or an oversight.

The problem is not with particular instances, but with the pattern. This pattern makes SQL injection possible, while the other pattern makes it impossible.

Answer 4

If $user_id is escaped, then you should not be vulnerable to SQL Injection.

In this case, I would also ensure that the $user_id is numeric or an integer (depending on the exact type required). You should always limit the data to the most restrictive type you can.

Answer 5

I think 'Properly Escaped' here is the keyword. In your last question, I'm making the assumption that your code is copy pasted from your production code, and since you asked question about three tables join, I also make the assumption that you didn't do proper escaping, hence my remark on SQL Injection attack.

To answer your question, as so many people here has described, IF the variable has been 'Properly Escaped', then you have no problem. But why trouble yourself by doing that? As some people have pointed out, sometimes Properly Escaping is not a straightforward thing to do. There are patterns and library in PHP that makes SQL Injection impossible, why don't we just use that? (I also deliberately make assumption that your code is in fact PHP). Vinko Vrsalovic answer may give you ideas on how to approach this problem.

Answer 6

That statement as such isn't really a problem, its "safe", however I don't know how you are doing this (one level up on the API stack). If $user_id is getting inserted into the statement using string operations (like as if you are letting Php automatically fill out the statement) then its dangerous.

If its getting filled in using a binding API, then your ready to go.

Answer 7

All answers are good and right, but I feel I need to add that the prepare/execute paradigm is not the only solution, either. You should have a database abstraction layer, rather than using the library functions directly and such a layer is a good place to explicitly escape string parameters, whether you let prepare do it, or you do it yourself.