2
function Query()
{
    $args = func_get_args ();

    if (sizeof ($args) > 0)
    {
         $query = $args[0];

         for ($i = 1; $i < sizeof ($args); $i++)
                $query = preg_replace ("/\?/", "'" . mysql_real_escape_string ($args[$i]) . "'", $query, 1);
    }
    else
    {
          return FALSE;
    }

I have a function like this. Basically, I make a query like this:

$this->Query('SELECT * FROM USERS WHERE Username = ? AND Points < ?', $username, $points);

It currently supports deprecated mysql functions, but adapting to mysqli will be as easy as replacing mysql with mysqli in my class.

Is this a safe approach to rely on against SQL Injection attacks? Every single question mark is getting sanitized automatically by mysql_real_escape_string and I never had problems before, but should I use mysqli_real_escape_string for sanitization?

I know about prepared statements of mysqli but using bindParam for each variable seems a little overkill to me.

What do you think?

Andy Lester
  • 91,102
  • 13
  • 100
  • 152
Lisa Miskovsky
  • 896
  • 2
  • 9
  • 18
  • 4
    *"Seems a little overkill"?!* It's "overkill" to have a *simple, reliable, hard to screw up* mechanism to ensure the validity of your query syntax and prevent SQL injections?! – deceze Mar 13 '13 at 15:55
  • *“but using `bindParam` for each variable seems a little overkill to me”* What? Seriously? It's never overkill. – Waleed Khan Mar 13 '13 at 15:55
  • 1
    mysql_real_escape_string can't be relied upon (it's possible to do a "little bobby tables" without relying on embedding strings); prepared statements might seem overkill.... right up to the point where your database is compromised – Mark Baker Mar 13 '13 at 15:55
  • @Mark `mysql_real_escape_string` *can* be relied upon *if it's used properly*. Unfortunately it's easy enough to screw it up in practice. – deceze Mar 13 '13 at 15:57
  • 2
    You don't need to use `bindParam` for each variable. Pass the array directly in the `execute()` call. – hjpotter92 Mar 13 '13 at 15:57
  • 1
    possible duplicate of [Shortcomings of mysql\_real\_escape\_string?](http://stackoverflow.com/questions/12703420/shortcomings-of-mysql-real-escape-string) – deceze Mar 13 '13 at 15:58
  • Bind params is also much faster than your solution using `preg_replace`. Your solution would also replace any `?` in your data even if you wanted to insert it as part of an argument. – Cfreak Mar 13 '13 at 15:59
  • Why bother with the escaping? Be lazy and let existing debugged and tested code take care of it all. Learn to use bind parameters. http://bobby-tables.com/php has examples. – Andy Lester Mar 13 '13 at 15:59
  • @deceze - people don't use it properly because they don't use it for numeric values (where you wouldn't normally use it... "who needs to escape a numeric"), but if they haven't validated their inputs as numerics, then you have the last security breach I had to identify (they did actually validate their numeric inputs and did a page redirect to report the error, but without an exit so the SQL query was still fired off) - it's the (mis)understandings of mysql_real_escape_string that's the real problem; but it can't be relied upon in isolation and causes even more problems with magic quotes – Mark Baker Mar 13 '13 at 16:04
  • Your current approach to this is *broken* and also the reason why one should not do stuff like this by yourself unless you really know what you're doing. Currently you will also replace `?` from a previous insertion, so one could break you protection by having `abc?def` as one param and then ` some exploit code ` as the second. It would be assembled to `"abc" some exploit code "def"`. – NikiC Mar 13 '13 at 16:18

3 Answers3

2

Using binded parameters is not overkill and should be required. It will more efficiently escape and prepare your parameters.

$stmt = mysqli_prepare($link, "INSERT INTO CountryLanguage VALUES (?, ?, ?, ?)");
mysqli_stmt_bind_param($stmt, 'sssd', $code, $language, $official, $percent);

$code = 'DEU';
$language = 'Bavarian';
$official = "F";
$percent = 11.2;

/* execute prepared statement */
mysqli_stmt_execute($stmt);

Does that really seem like overkill?

Documentation

Kermit
  • 33,827
  • 13
  • 85
  • 121
  • Sure it does. it took you 7 lines of code while it takes **one** for the OP. And it will be twenty for a dozen fields but still remain one for the OP. – Your Common Sense Mar 13 '13 at 19:36
1

A really great day today - second good attempt to create a sensible database abstraction layer in a row.

should I use mysqli_real_escape_string for sanitization?

Nope.
Just because this function doesn't sanitize anything.

But to format SQL string literals this function is a must and cannot be avoided or replaced.
So, you are using this function exactly the right way, formatting strings only and formatting them unconditionally.
So, you have you queries perfectly safe, as long as you can use a ? mark to substitute the actual data (and - to make even nitpick complains idle - as long as you set SQL encoding using mysql(i)_set_charset() function).

If someone calls your approach broken - just ask them for the complete snippet of proof-code to show the certain vulnerability.

However, let me draw your attention to a couple of important things.

  1. Dynamic SQL query parts are not limited to strings only. For example, these 2 queries won't work with your function:

    SELECT * FROM table LIMIT ?,?
SELECT * FROM table ORDER BY ?

    just because numbers and identifiers require different formatting.
    So, it's better to use type-hinted placeholders, to tell your function, which format to apply

  2. To run a query is only a part of the job. You need to get results as well. Why not to get them already, without bloating your code with unnecessary calls?
  3. There should be a way to insert literal ? marks into query without parsing them.

Please, take a look at my class, which built on the very same principle as yours but with improvements I mentioned above. I hope you will find it useful or at least worth to borrow an idea or two.

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • Okay. I read your reply but couldn't understand some parts so I will appreciate it if you can reply once again. a. mysqli_real_escape_string() doesn't sanitize anything. Why? Doesn't it secure whatever I pass as parameter? 1. Query with LIMITS works because I can send two integer parameters. (I believe I used limit in my functions quite often.) 2. I do get results with mysql_row_assoc but I removed them from this function because they were unrelated. 3. You mean I should find a way to skip legit question marks in parameters? – Lisa Miskovsky Mar 13 '13 at 21:26
  • For some reason I missed your comment yesterday, but here you go. a. http://stackoverflow.com/a/9296858/ 1. With present code it doesn't. May be you have another condition there - then yes. 2. great, but I hope you don't automate the returned type 3. not in parameters but in the query. It's not a big deal though. I'd be much obliged if you post the whole code - so, I'll need no assumptions. – Your Common Sense Mar 14 '13 at 10:40
  • So, a. My approach is safe as this and there is no injection issues? 1. You mean things like ?Integer, ?String, ?Identifier instead of ? so they all get different handling? 3. If there is a way to contact you, I can send you the whole class I've written. Not only you can see the whole function, but I'd appreciate if an expert would comment on my class. Ps. Thank you alot, by the way. You've replied like all of my stackoverflow questions with great responses. I really appreciate it. – Lisa Miskovsky Mar 14 '13 at 11:59
  • @LisaMiskovsky you can find my email in the profile, I'd like to see what you've done so far. I am interesting in writing such classes myself (and quite discouraged by the fact that only few people on Stackoverflow ever care of them). To tell if your approach is safe or not I'd prefer after reviewing the code. – Your Common Sense Mar 14 '13 at 12:05
0

if you use now mysqli instead of mysql. It would be better to use mysqli_real_escape_string. Beware the params order has been modified. (% and _ are still not escaped)

Vincent MAURY
  • 253
  • 2
  • 5