views:

156

answers:

5

PHP:

$SQL = "SELECT goodies FROM stash WHERE secret='" .  
    str_replace("'",'',$_POST['secret']) .  
"'";  

Could an evil genius hacker inject SQL into my SELECT - How ?

+14  A: 

Why won't you use mysql_real_escape_string() or even better - prepared statements? Your solution seems silly.

Hanse
+1 for prepared statement.
Steven Sudit
seconded... especially for prepared statements
prodigitalson
I'm trying to keep it as "lean" as possible...
T4NK3R
Then use a lean prepared statement. Filtering just doesn't work; use parameter binding *always*.
Steven Sudit
I'm building the statement dynamically (for a search with lots of options), so I'd rather keep each key-value pair together, by building the statement into one string as I go along. Still, 6 votes in 15 minutes.. Getting sweaty palms..
T4NK3R
@T4NK3R: is `str_replace` that much leaner than `mysql_real_escape_string`? Use the proper tool for the job -- `str_replace` is a half-assed replacement.
tomlogic
Even if you generate the query dynamically, you can still bind to its parameters instead of relying on an ad-hoc encoding technique.
Steven Sudit
A prepared statement will break my back (lots of fancy short-circuiting with date-parameters) - I need it to be in a single string to keep my sanity. So, I'll just exchange the str_replace() with a mysql_real_escape_string(). - Is everybody happy with that ?
T4NK3R
@T4NK3R: I'm not, but we can't risk your sanity further.
Steven Sudit
A: 

Why just don't use mysql_escape_string? And yes, he could, adding " instead of ' and plus, this query will give you an error, I guess.

hey
An error is OK - as long as it doesn't affect honest users.
T4NK3R
@hey: I would think that the query `SELECT goodies FROM stash WHERE secret='"'` is no problem. The double-quote won't end the string. What sort of an error are you referring to?
Mark Byers
A: 

I think that is safe, however, user can not post/search any string contain ' -> not good

Bang Dao
Not safe at all.
Steven Sudit
you and I has difference way when say `safe`. Anyway, thank for comment.
Bang Dao
Yes, and the difference is that you're mistaken. This is not something you can credibly dispute.
Steven Sudit
@Steven Sudit: it would be helpful to explain how it is not safe. No one seems to have come up with a way for an evil genius to inject SQL here. The worst example we have is ending with backslash, causing a SQL error.
Ned Batchelder
@Ned: With all due respect, I think you have this entirely backwards, in terms of burden of proof. We've had many cases of encoding schemes failing, but no cases of preparation/binding going wrong. Therefore, I don't have to prove this encoding scheme is broken; they would have to prove that it's as good as binding, and I don't see how they could do that. Remember, you must assume that the attacker is at least as smart as you are, and much too motivated. Your own inability to detect a weakness is a statement only about your limitations, not the quality of the technique.
Steven Sudit
@Steven Sudit .. First, prove it can be exploit in this specific case.
h3xStream
@Steven Sudit: I'm with you: there's no need to reinvent wheels. MySQL provides a function that can be used to escape strings, and it's simple, and should be used. But you made a statement with no supporting evidence. "Not safe at all" is a strong statement, and doesn't seem supportable.
Ned Batchelder
@Steven Sudit: (sorry, forgot this point in the last comment). The question was how the technique was insecure, simply asserting that it was bad isn't helping anyone understand the problem.
Ned Batchelder
@Ned: The `mysql_real_escape_string` approach is about as good as the filtering method is going to get, and yet it's not something I would ever consider using. The OP offered something even less reliable. In contrast, binding is reliable because it doesn't depend at all on the contents of the bound values. They're sent boxed up, with no parsing needed or possible.
Steven Sudit
@h3xStream: I've explain why the burden of proof is with the OP, yet you seem to have ignored this inconvenient argument by nonetheless demanding that I provide something I have no obligation to offer. Think this through.
Steven Sudit
A: 

May be. The best way is:

$query = sprintf("SELECT goodies FROM stash WHERE secret='%s'",
addcslashes(mysql_real_escape_string($_POST['secret']),'%_'));
isola009
You don’t need to escape `%` and `_` as you’re not using `LIKE`.
Gumbo
Yes, addcslashes function is optional
isola009
I'm building the statement dynamically (for a search with lots of options), so I'd rather keep each key-value pair together, by building the statement into one string as I go along.
T4NK3R
+5  A: 

I've had a think about this for a while and I can't see any way to inject SQL into this statement.

An SQL string that starts with a single quotes terminates at the next single quote unless it is escaped with a backslash or another quote (\' or ''). Since you are removing all single quotes there cannot be a doubled quote. If you escape the closing quote you will get an error, but no SQL injection.

However this method has a number of drawbacks:

  • Single quotes in the input are ignored.
  • Backslashes in the input aren't handled correctly - they will be treated as escape codes.
  • You get an error if the last character is a backslash.
  • If you later extend the query to add a second parameter, it would allow an SQL injection attack.

For example:

$SQL = "SELECT goodies FROM stash WHERE secret='" .  
    str_replace("'",'',$_POST['secret']) .  
"' AND secret2 = '" .
    str_replace("'",'',$_POST['secret2']) .  
"'";  

When called with parameters \ and OR 1 = 1 -- would result in:

SELECT goodies FROM stash WHERE secret='\' AND secret2=' OR 1 = 1 -- '

Which MySQL would see as something like this:

SELECT goodies FROM stash WHERE secret='...' OR 1 = 1

Even if it's impossible to cause an injection in this case the drawbacks make this unsuitable for a general purpose way to avoid SQL injection.

The solution, as already pointed out, is to use a prepared statement. This is the most reliable way to prevent SQL injection attacks.

Mark Byers
Oops, I do that (a lot!). But exchanging the str_replace with mysql_real_escape_string($_POST['secret']) would cure it (AND the possibility of causing an error) ?
T4NK3R
@T4NK3R: Do what a lot? There is a very rare case where mysql_real_escape_string doesn't work properly: http://ilia.ws/archives/103-mysql_real_escape_string-versus-Prepared-Statements.html. Prepared statements is the best way, but mysql_real_escape_string should work too if done correctly.
Mark Byers
Do: add extra key=value pairs (mostly integers though "hardened" by simple (int) casts). BTW: my php-source is in UTF-8 and the dbConnection also (mysql_set_charset('utf8',$dbCon);)- Off to read your link now!
T4NK3R
@T4NK3R: If you have two strings in the same query and you're using the str_replace method both times then it's unsafe as I have demonstrated. If using mysql_real_escape_string the attack I showed above is not possible.
Mark Byers
@Mark Byers: Right, that was scarry reading, but wouldn't have affected me, and the vulberability is fixed now, anyway : )- Your answer get's my green checkmark. But thanks to all participating!
T4NK3R
@Mark Byers I stand corrected.
Rook