1

First - I know prepared statements are the "silver bullet" and I "should be using them" - however for this project, it's not feasable. Trust me when I say it cant happen. So, before the preaching starts... ;)

So, I've been reading for 4 hours tonight on SQL injection.

And EVERY rebuttal to using real_escape_string (I'm working with PHP and MySQL) says, basically, that real_escape_string wont stop this:

SELECT * FROM something WHERE id=1 or 1=1;

Right. Got it.

But since I first touched mysql about 15 years ago, every query I have EVER written has ALWAYS single quoted all parameters I send in a query. Like, if I don't single quote parameters, I have this ikky feeling the query will fail. So I ALWAYS single quote everything. All. The. Time.

And:

$_POST['userinput'] is submitted as "1 or 1=1";
$clean = $db->real_escape_string($_POST['userinput']);
$db->query("SELECT * FROM something WHERE id='$clean'");

How can that EVER be broken? Can it? I've never seen an example that shoots down real_escape_string that does NOT use the unquoted version.

It's in quotes. All inner quotes that could break out of the quoting are escaped.

Again: I know prepared statements are the best. Thats not what I'm asking. I'm asking if what I wrote above can be broken?

I dont see a way.

Barmar
  • 741,623
  • 53
  • 500
  • 612
InfernusDoleo
  • 139
  • 9

4 Answers4

2

I believe your code is safe. Since you've wrapped the string in quotes, the only way for the or to be interpreted as a SQL keyword would be for the input to contain an ending quote. But real_escape_string will escape that, so it won't end the string.

What you can also do for values that are required to be integers is 

$clean = (int)$_POST['userinput'];

This will convert the input 1 or 1=1 to 1.

Barmar
  • 741,623
  • 53
  • 500
  • 612
  • +1. OP code is "safe". The `real_escape_function` identifies "dangerous" characters in the input string, and returns a string that is "safe" to be included in a SQL statement, if that is done also done in a "safe" way, as in the OP example, enclosing it in single quotes in the SQL text. – spencer7593 Feb 28 '14 at 02:05
0

use intval() on your integers.

Loïc
  • 11,804
  • 1
  • 31
  • 49
  • 2
    Or avoid the function call by casting to int with `(int)`, if you're in the mood for micro-optimization. – Bill Karwin Feb 28 '14 at 02:14
  • Plain right, though if you are in the mood for micro-optimization you won't be using PHP ;-) – Loïc Feb 28 '14 at 02:15
  • I've always just passed ints to inserts or selects single quoted as well without issue. Its always worked. Is there a performance issue by quoting it? IE: Why cast it as an int and unquote it? – InfernusDoleo Feb 28 '14 at 02:17
  • if you don't use magic_quotes, nor escape the input nor cast it as an integer, then it's vulnerable to sql injection. – Loïc Feb 28 '14 at 02:19
0

Mysql_real_escape_string can be broken in some cases. See here: https://stackoverflow.com/a/12118602/2167896

Complicated vulnerabilities aside, if you're not using prepared statements all it takes is ONE mistake by any programmer on the project and you're hosed. In a business environment this severely limits the number of programmers you can put onto the project and makes quality control of every query an absolute necessity.

If you ever plan to scale up, seriously consider rewriting all of your queries; you can even improve the performance with the latest techniques and mysql enhancements while you're at it, so it's not a total loss.

Community
  • 1
  • 1
Michael Benjamin
  • 2,895
  • 1
  • 16
  • 18
  • I've read the prepared statements actually slow your queries down UNLESS you're running the same query over and over. This is for a database heavy web application where the page loads, does many various sql queries (not repeating any) and then quits. Obviously cannot keep the prepared statements 'live' once the page load quits. So where is the performance improvement? – InfernusDoleo Feb 28 '14 at 02:18
  • Oh and I read that post earlier - and thats basically only if you're using an old version of MySQL and/or using some non-standard character set. I'll never be setting my character set to GBK and my MySQL is modern, so its a non issue. – InfernusDoleo Feb 28 '14 at 02:23
  • Not true, an application that would not see increased performance from parameter queries is extremely unlikely. Here are some articles: http://www.mysqlperformanceblog.com/2006/08/02/mysql-prepared-statements/ https://www.simple-talk.com/sql/t-sql-programming/performance-implications-of-parameterized-queries/ http://codebetter.com/davidhayden/2006/01/05/parameterized-queries-and-performance/ – Michael Benjamin Feb 28 '14 at 02:25
  • Are those relevant/correct anymore? Those articles are 8 years old. Everything I've read says prepared statements are SLOWER if you're only using them once - they are designed for repeated use. One shot statements are always faster than using a prepared statement just once. – InfernusDoleo Feb 28 '14 at 02:32
0

In fact, prepared statement is not a feature but a principle. And one can implement it on their own. Which you have to do, instead of devising excuses.

Your idea of formatting a string literal is right. YET this formatting have to be done via prepared statement.

Manual formatting you are using is separated and detachable. You are adding quotes in one place and escape them in another - this alone is a source of many disasters. And even if you move it in one place - your formatting is still detachable, means it can be moved somewhere away from the actual query execution and forgotten there.

This is why you should always use a placeholder to represent your variable in the query and then format the data when substituting it.

This is why prepared statement a silver bullet, not because someone just told you so.

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345