Title: Patching a time based sql injection
Tags: php;mysql;sql;security
Question: I recently performed a code audit on client, I found these lines of code that was subjected to a blind SQL injection
Code snippet:
<?php
if ($_GET['id'] != ''){
$groepsindeling = $_GET['id'];
} else {
$groepsindeling = $_GET['group'];
breadcrumb($groepsindeling);
<?php
$groep = "SELECT * FROM `menubalk` WHERE `webnr` LIKE '%$webnr-%' AND `hoofdgroep` = '$_GET[id]' ORDER BY `naamt1` ASC";
$groepres = mysql_query($groep);
while ($groeprij = mysql_fetch_array($groepres)){
I patched it immediately by using the mysqli_real_escape_string() function.
New code:
<?php
if ($_GET['id'] != ''){
$groepsindeling = mysqli_real_escape_string($_GET['id']);
settype($groepsindeling, "integer");
else {
$groepsindeling = mysqli_real_escape_string($_GET['group']);
breadcrumb($groepsindeling);
<?php
$groep = "SELECT * FROM `menubalk` WHERE `webnr` LIKE '%$webnr-%' AND `hoofdgroep` = '$_GET[id]' ORDER BY `naamt1` ASC";
$groepres = mysql_query($groep);
while ($groeprij = mysql_fetch_array($groepres)){
I did further testing to see if the code would work it worked against the boolean based blind SQL injection, but it seems like a failure as a time based blind SQL injection is still applicable here.
All suggestions are appreciated.
Comment: @CapitalC But can i  still use the function inside the query?

Comment: @Shadow Yes i know that the mysql api is deprecated, but they didnt want to change the code. So that is why i am stuck here.. need to find a way to patch this without alot of changes of the code. Have already suggested a WAF(web application firewall), but they declined ofcourse.

Comment: @CapitalC Thank you very much! i can now see the fault. Appreciate the help!!

Comment: 1. You are mixing mysql and mysli apis. 2. You should not be using mysql api any longer. It has been deprecated ages ago and was removed in php v7. 3. Simple escaping is not enough against all type of sql injections. Either use parametrized queries (not available in mysql api, only in mysqli or pdo), or you have to beef-up the escaping code.

Comment: @epidrollic - I would caution just replacing `$_GET['id']` with `$groepsindeling`, see my answer for an explanation....

Comment: You are still using the non escaped $_GET['id'] in your query.

Comment: Just replace `$_GET['id']` with `$groepsindeling` in your query. The idea of escaping is to sanitize untrusted input which is the $_GET['id'] parameter in this case and use the sanitized parameter in your query.

Comment: @Shadow some clients dont want to change their code... ;(

Here is the accepted answer: As others noted you still have the escaped $_GET['id'] in the query,  However $groepsindeling may not be equivalent, to the get var, strictly speaking.  Essentially what you are saying is this
$groepsindeling = $_GET['id']  or $_GET['group'];
Represnted here:
if ($_GET['id'] != ''){
$groepsindeling = $_GET['id'];
} else {
$groepsindeling = $_GET['group'];
Which is not what is represented in the original query.
"SELECT * FROM `menubalk` WHERE `webnr` LIKE '%$webnr-%' AND `hoofdgroep` = '$_GET[id]' ORDER BY `naamt1` ASC";
The query is strictly using the $_GET['id'], and as I don't see $_GET['group'] in there. Then this leaves two choices,
the original code is flawed
the original code is correct ( in this case $Id should be a new variable )
So like this
$id = mysqli_real_escape_string($_GET['id']);
"SELECT * FROM `menubalk` WHERE `webnr` LIKE '%$webnr-%' AND `hoofdgroep` = '$id' ORDER BY `naamt1` ASC";
Without seeing the entire code for this page, it's impossible to tell if that $groepsindeling is even related to the query or just used  somewhere else.  The other possibility as I mentioned is that they are the same and the orignal code is bugged.
All that said, given this breadcrumb($groepsindeling); I would argue that it's unrelated to the query, and leave the code as is.  Changing it and including the $_GET['group'] value in the query may result in unpredictable behavior, especially if that query relies on $_GET['id'] = '' for anything.
Make sense.
As a side note, in current PHP versions this $_GET[id] from the original query will likely throw a undefined constant warning, I've seen this used before in pre 5.3 PHP code....  It should be $_GET['id'].  It still works because PHP will give an undefined constant the string representation of its name.  ( just a way to tell how old the code is :-p )
And one last note is you are using mysql_query with mysqli_real_escape_string so that i in there has some meaning, or the lack of it in this mysql_query.  The mysql_* functions are deprecated and If I recall removed as of PHP 7, which means this code is not future proof beyond PHP 5x.  I am not sure if there is any significant difference between  mysql_real_escape_string and mysqli_real_escape_string

Here is another answer: As said above, you use $_GET['id'] in you're query which means that people can still SQL inject the query string:
$groep = "SELECT * FROM `menubalk` WHERE `webnr` LIKE '%$webnr-%' AND `hoofdgroep` = '$_GET[id]' ORDER BY `naamt1` ASC";
Try to do:
$groep = "SELECT * FROM `menubalk` WHERE `webnr` LIKE '%$webnr-%' AND `hoofdgroep` = '{$groepsindeling}' ORDER BY `naamt1` ASC";
Then you use the "$groepsindeling" variable that you used real_escape_string on.
You should also read http://php.net/manual/en/mysqli.real-escape-string.php - as it says, when you use the "mysqli_real_escape_string" you also need to set the MySQLi object in the method:
string mysqli_real_escape_string ( mysqli $link , string $escapestr )
Also, you should keep to the MySQLi or MySQL (do not recommend this one).
Comment for this answer: Thank you very much, it got resolved now! appreciate the help!!
Comment for this answer: @epidrollic - the original code is flawed, you realize this right?  in the original code ` $_GET['group'];` is never used in the query.  By updating that you are putting it in the query, which may or may not be what was originally intended.  In other words assuming that `$groepsindeling ` = `$_GET[id]` is incorrect.  A new variable should be used.

{'source': 'StackExchangeDataset', 'subset': 'stackoverflow.com', 'metadata': {'AnswerCount': 2, 'CommentCount': 8, 'ContentLicense': 'CC BY-SA 3.0', 'CreationDate': '2016:00:09/10/16', 'FavoriteCount': None, 'PostTypeId': 1, 'Tags': '<php><mysql><sql><security>', 'Title': 'Patching a time based sql injection', 'ViewCount': 634, 'id': 39428199, 'user_AboutMe': '', 'user_AccountId': 5310905.0, 'user_DisplayName': 'epidrollic', 'user_Reputation': 53.0, 'user_Views': 16.0, 'user_id': 4237276.0}}