views:

206

answers:

4

I have been bothered for so long by the MySQL injections and was thinking of a way to eliminate this problem all together. I have came up with something below hope that many people will find this useful.

The only Draw back I can think of this is the partial search: Jo =>returns "John" by using the like %% statement.

Here is a php solution:

<?php
function safeQ(){
   $search= array('delete','select');//and every keyword...
   $replace= array(base64_encode('delete'),base64_encode('select'));
   foreach($_REQUEST as $k=>$v){
      str_replace($search, $replace, $v);
   }
}
safeQ();

function html($str){
   $search= array(base64_encode('delete'),base64_encode('select'));
   $replace= array('delete','select');//and every keyword...
   str_replace($search, $replace, $str);
}

//example 1
...
...
$result = mysql_fetch_array($query);
echo html($result[0]['field_name']);

//example 2
$select = 'SELECT * FROM $_GET['query'] '; 

//example 3
$insert = 'INSERT INTO .... value( $_GET['query'] )'; 


?>

I know, I know that you still could inject using 1=1 or any other type of injections... but this I think could solve half of your problem so the right mysql query is executed.

So my question is if anyone can find any draw backs on this then please feel free to comment here.

PLEASE GIVE AN ANSWER only if you think that this is a very useful solution and no major drawbacks are found OR you think is a bad idea all together...

+11  A: 

This has already been implemented correctly in PHP:

However, you should start by reading the PHP manual entry on SQL injection.


To answer your actual question, the primary drawbacks of your approach are:

  1. It doesn't work
  2. Just to be clear, it doesn't work, and will leave you wide open to SQL injection.
  3. It also prevents you entering any data which clashes with a MySQL keyword.

If you're going to reinvent the wheel, I would recommend looking at pre-existing wheels.

Colin Pickard
mysql_real_escape_string only ads slashes "\" to escape key characters. it does not encode keywords.the good thing about my code above is that you can still search key words.
Val
Your code does not prevent SQL injection. It just doesn't work. I don't even understand what advantage you are trying to describe?
Colin Pickard
It doesn't stop bullets, but the good thing about my bulletproof vest is the range of fetching colours...
Colin Pickard
@Val but it is unnecessary to do something with keywords! They ARE no harm at all!
Col. Shrapnel
I have NOT tested it but just giving an idea of how it should work... why is everyone so negative on this website no one answers as a human bein, dont be harsh whats wrong with you. I think it's straight forward... give me an injection that could by pass it.
Val
@Val it's nothing personal. We've just seen people get hurt by this before, and it's important to us to be very clear on how to do it properly. Experimenting with code is a great thing; it's how we do what we do; but there is a danger here that you must be alerted to. It's like walking into a firestation break room and saying "I'm thinking of making my own fireworks, I've got 50kg of gunpowder, a carrier bag and a match - what do you guys think?"... you shouldn't be surprised if the reaction is somewhat negative
Colin Pickard
These blog posts have some excellent examples of how you can work around this type of code, _including_ keyword filteringhttp://websec.wordpress.com/2010/03/19/exploiting-hard-filtered-sql-injections/http://websec.wordpress.com/2010/05/07/exploiting-hard-filtered-sql-injections-2-conditional-errors/
Paolo
@Val: There are well-known, simple, tested and efficient solutions to SQL sanitization. Yours is neither simple, or tested, or efficient. If you choose to interpret the fact that "somebody already thought of this before" as "people are being mean to me", that's obviously your choice, but doesn't have much to do with programming.
Piskvor
+5  A: 

Reinventing the wheel and reinventing it the Wrong Way (TM).

  • First of all, there are parametrized queries (available for PHP in MySQLi extension); if that's not an option, there's mysql_real_escape_string. This is the main issue - check for already available options before deciding to implement them on your own.
  • Second, you are trying to call PHP functions in SQL, what you wanted was probably something like 'SELECT * FROM ' . safeQ($_GET['query'])
  • Third, you've broken all indexing and search on data containing your "evil words", say hello to performance problems and crazy workarounds.

Edit: To address the example you're giving in comments:

$v="1; DROP tbl;\";DROP tbl" // oh look, an SQL injection attempt!
$s = 'SELECT * FROM tbl WHERE ID='.$v; // SQL injection, no doubt

// if ID is an integer field, make it an integer. Simple, secure, and fast.
$s = 'SELECT * FROM tbl WHERE ID='.(int)$v; 
// $s == 'SELECT * FROM tbl WHERE ID=1' // see PHP manual for explanation of type casting

// if ID is a string field, escape it. Simple, secure, and still plenty fast.
$s = 'SELECT * FROM tbl WHERE ID="'.mysql_real_escape_string($v) . '"';
// $s == 'SELECT * FROM tbl WHERE ID="1; DROP tbl;\";DROP tbl"'; 
// See? No injection, as the quote is *escaped*
Piskvor
ok except that $v would look something like this $v= "1; ?????? tbl;\";??? tbl" if you look at base64_encoding i think youd feel pretty stupid right about now with ur "I AM TOO SMART AND YOU ARE TOO STUPID ANSWERS".
Val
BTW there have been case where mysql_real_escape_string() has been by passed (cant remember exactly how but by using something like x0XXX ) which gives you a double escape `"DROPtbl;\\";DROP tbl"`
Val
@Val no, mysql_real_escape_string() cannot be bypassed :) You just confused something. You'd better start to learn instead of whining and complaining. Nothing personal. Just friendly advise
Col. Shrapnel
@Val: I have said "that's a wrong way to approach the problem". That is something completely different from "you are stupid for suggesting that". As for the bypass, the function you're thinking of is `addslashes()`; `mysql_real_escape_string()` was made to counter the Unicode hack you mention, and is not vulnerable to it.
Piskvor
@col But I think it was a suggestion to what i thought there was a problem on the first place... However this could be a lesson for myself and some people out there who over think SQL Injects and who are lazy to well impliment their SQL... It has been a helpful topic for me ...
Val
+1  A: 

Look, you're thinking of it from the wrong point of view.
Though many people do. There are 2 strong misbeliefs according to SQL injections:

  1. There is some special attack, called "SQL injection".
  2. To prevent it, input data must be threaten some special way.

Both of them wrong.

There is not such thing as "SQL injection" when you follow SQL syntax.

Go figure.

If you follow SQL syntax rules (and you must follow it anyway, despite of injections!) you are safe, just as a side effect.

Any string data put into SQL query must be treated with 2 rules:
1. all special characters in the string must be escaped.
2. the string must be enclosed in quotes.

There are also issues with non-textual data. Not a big deal too.
But if you feel it too complicated, you can always go for the prepared statements - it is always safe and allow no thinking.

Though there are still issues with non-data parts of the query. That's most danger part indeed. And the only issue have the right to be called "SQL injection".
To avoid it, all dynamic non-data parts of the query must be hardcoded in your script. That's the only possible way.

Col. Shrapnel
+2  A: 

It is a bad idea.

There is already a proper, correct, and working, solution to SQL injection in every language, platform, runtime, engine, library, whateveryoumighthave.

It's called parameters.

Instead of placing the value into the SQL as a literal constant, you add a parameter placeholder, and then provide the paramter value alongside the query, and not using any formatting tricks to get it back into the SQL either.

Every other solution which tries to "fix" the problem by looking at the SQL or pieces of it and attempting to determine if it is correct is sub-par and most of them has other bugs and problems that you can't see, don't know about, and don't really want to have.

So stop beating around the bush, and do it the right way to begin with. Switch to using parameters.

In PHP, talking to MySQL, there are multiple ways, but using the mysqli library:

$stmt = $db->prepare('SELECT * FROM products WHERE id = ?');
$stmt->bind_param('i', $id);
$stmt->execute();
.. bind for results
$stmt->fetch();

If the $id variable here contains unsafe code, it will make the SQL Execution crash, because if id is an integer, it cannot be compared to anything but another integer, and if it is a string, it will be compared against the string you provide, not trying to execute the string as part of the SQL.

Now, the comment brings out a point, sometimes you have to change the SQL anyway, because you will provide dynamic ordering (ie. ordering picked by the user), or dynamic filtering (ie. which filters to apply are picked by the user).

In this case, the solution is simple: Do not take any user-provided text and place it into the SQL. Instead, you hard-code the constants to add to your SQL into your code, and let the users choices guide the code in which of those constants to add, in which order, and where.

Lasse V. Karlsen
though this solution is not complete and sometimes we have to look at the pieces anyway. in case of dynamic ordering for example
Col. Shrapnel
you should still not add any user-provided text into the SQL. If you need to have dynamic ordering, you would inject hard-coded constants from your code, based on the input of the user, so that if the user provides you with bad data in an attempt to inject it into the SQL, the most he'll be able to do is confuse or break the code that pickes the right column names or expressions for the ordering.
Lasse V. Karlsen