Patching a time based sql injection

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.

You are still using the non escaped $_GET['id'] in your query. — Capital C, Sep 10 '16 at 16:35
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. — Shadow, Sep 10 '16 at 16:36
@CapitalC But can i still use the function inside the query? — epidrollic, Sep 10 '16 at 16:38
@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. — epidrollic, Sep 10 '16 at 16:45
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. — Capital C, Sep 10 '16 at 16:52
@CapitalC Thank you very much! i can now see the fault. Appreciate the help!! — epidrollic, Sep 10 '16 at 16:56
@epidrollic - I would caution just replacing `$_GET['id']` with `$groepsindeling`, see my answer for an explanation.... — ArtisticPhoenix, Sep 10 '16 at 18:22

SimmeD · Answer 1 · 2016-09-10T16:58:01.070

1

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).

edited Sep 10 '16 at 16:58

answered Sep 10 '16 at 16:49

SimmeD

179
13

Thank you very much, it got resolved now! appreciate the help!! – epidrollic Sep 10 '16 at 16:57
@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. – ArtisticPhoenix Sep 10 '16 at 18:09

ArtisticPhoenix · Accepted Answer · 2016-09-10T18:37:09.863

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

Patching a time based sql injection

2 Answers2