CODEWITHSUNDEEP
phpmysqlmysqlisql-injection
Lisa Miskovsky
Lisa Miskovsky

Reputation: 926

mysql(i)_real_escape_string, safe to rely on?

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?

Upvotes: 2

Views: 247

Answers (3)

Your Common Sense
Your Common Sense

Reputation: 157828

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.

Upvotes: 1

Vincent MAURY
Vincent MAURY

Reputation: 253

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)

Upvotes: 0

Kermit
Kermit

Reputation: 34054

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

Upvotes: 1

Related Questions