Short Solution for SQL Injection Attacks with MySQLI

Question

I've gotten into the habit of writing the following sort of code:

$q = mysqli_query($mysqli,"SELECT * FROM table WHERE a='$a', b=$b;");
while ($row = mysqli_fetch_array($q)) {
    // do something
    }

Where $a is a string entered by the user (gotten through $_GET) and $b is a user-entered integer.

Obviously the code I have above is vulnerable to SQL injection attacks, so my habit is to rewrite it like this:

$q = mysqli_query($mysqli,"SELECT * FROM table WHERE a='".str_replace("'","",$a)."', b=".($b+0).";");

But this of course has problems if $a needs to have apostrophes (or quotation marks when quotation marks are used to mark the string).

Recently I learned about prepared statements in mysqli and started playing around with them. I wrote the following function to make it easier to make calls without having to change much of my code:

function safequery($a,$b,$c) {
    global $mysqli;
    $q = mysqli_prepare($mysqli,$a);
    $e = "mysqli_stmt_bind_param(\$q,\$b";
    $i = 0;
    while ($i < count($c)) {
        $e.=",";
        $e.="\$c[$i]";
        $i++;
        }
    $e.=");";
    eval($e);
    mysqli_stmt_execute($q);
    return $q;
    }

safequery("SELECT * FROM table WHERE a=? AND b=?;","si",array("unsafestring",37));

But what is returned from this function turns out not to be a mysqli_result and thus doesn't work with the first bit of code above. After some more research, I found an alternative, but it would require a complete rethink of how I write my code. Is this necessary or is it possible to protect against MySQL injection attacks with only small changes to the first bit of code (no new lines, same output style, etc.)?

I have looked around on StackOverflow and the rest of the web but I can't find a good simple solution; all of them require the edition of at least three more lines for every call and a different way of reading each row. I'd prefer to do this procedural-y...

Answer 1

Don't think half-measures are going to solve this problem. Commit to expunging all of the interpolation bugs from your code and be disciplined about using prepared statements. Your proposed fix only makes things worse, it gives you a false sense of security. It's also considerably more work than using prepared statements so I'm not sure why you'd even bother doing it this way.

One way to make this a lot easier to do is switch from using double quotes " to single quotes ' on your queries to disable interpolation. Any escaping errors become syntax problems, and if your editor highlights those you'll be able to spot them from across the room, and if something does by fluke work you'll be inserting harmless things like $a instead of actual data.

Another thing to consider is if you should be using an ORM like Doctrine or Propel given what you know about the sophistication of your application. These can make things considerably easier from an implementation perspective.

The code you have there is a ticking time bomb, get rid of it as soon as you can. Don't think replacing quotes is enough, that solves just one issue, there's actually a number of other methods your application can be vulnerable to injection bugs. Tools like SQLMap have an entire arsenal of things they can try to break your code and if you look at the list of things it can do if it finds a flaw you'll probably want to fix these problems right away.

One way you can find issues is using a tool like grep:

grep query `find -name '*.php'` | grep '\$'

That's not bulletproof, but it should probably turn up a lot of code you should fix right away.

Also as @ceejayoz suggests, purge that function with eval in it from your computer and never, ever do that again.

Answer 2

After talking with the commenters and looking at some of the other questions and things they linked to and suggested, I've rewritten the third code snippet to both solve the problem in question and fix the security holes that several commenters pointed out (if there are any remaining ones, please tell me).

First on my use of eval(). Though I can't see any immediate way it could cause problems (user strings are not executed as code in the eval()) it is certainly a round about and stupid way of solving my problem. @TadMan suggested call_user_func_array() which, once worked out, looks something like this:

function refValues($arr){
    if (strnatcmp(phpversion(),'5.3') >= 0) //Reference is required for PHP 5.3+
    {
        $refs = array();
        foreach($arr as $key => $value)
            $refs[$key] = &$arr[$key];
        return $refs;
    }
    return $arr;
}
function safequery($a,$b,$c) {
    global $mysqli;
    $q = mysqli_prepare($mysqli,$a);
    call_user_func_array("mysqli_stmt_bind_param",refValues(array_merge(array($q,$b),$c)));
    mysqli_stmt_execute($q);
    return $q;
}

safequery("SELECT * FROM table WHERE a=? AND b=?;","si",array("unsafestring",37));

It turns out that mysqli_stmt_bind_param() takes only references, thus the new refValues() function (run into and solved before on StackOverflow: https://stackoverflow.com/a/16120923/5931472).

While this removes eval() and makes my code easier to understand, it still doesn't solve the original problem of returning the query response in a way that mysqli_fetch_array() can use. It turns out that the proper function to do this is mysqli_stmt_get_result() so the last line of safequery() is rewritten from:

return $q;

To:

return mysqli_stmt_get_result($q);

The result of safequery() is then a mysqli_result which can be used by mysqli_fetch_array().