mysql injection

[AviNahum]

hey,

can somebody see any problem in this query?

 

    	$id = intval($ibforums->input['MID']);
    	
    	if ( empty($id) )
    	{
    		$std->Error( array( 'LEVEL' => 1, 'MSG' => 'incorrect_use' ) );
    	}
    	
    	//--------------------------------------------
    	// Prepare Query...
    	//--------------------------------------------
    	
    	$DB->query("SELECT m.*, g.g_id, g.g_title as group_title, g.g_icon, s.login_type, s.location as sesslocation, s.in_forum, s.in_topic 
    				FROM ibf_members m 
    				LEFT JOIN ibf_sessions s on (s.member_id=m.id), ibf_groups g 
    				WHERE m.id='$id' and m.mgroup=g.g_id");

 

it's works great, but hackers still can use this query to inject...

i cant fix this...

 

Thanks in advance!

[cssfreakie]

The idea behind sql injection is that you allow user submitted values without stripping / escaping them from bad things which can end up to shape the query instead of just being a normal value you expect.

 

So in other words: all values you receive from your users should not be trusted and be run through a sanitizing function like:

http://php.net/manual/en/mysqli.real-escape-string.php  <--- examples given there

 

In case you use prepared statements the user submitted values and the query are separated from each other so in that case a similar function is not needed.

 

So find out which are the user submitted values, and sanitize them. IN other words the variables you use in the above query are they user submitted? if so sanitize them.

[AviNahum]

got it!

thank you!

[cssfreakie]

got it!

thank you!

your welcome, If you quickly (and dirty) want to test if your vulnerable just type in a ' if you get an error your vulnerable.

[Psycho]

One addition to cssfreakie's explanation. The mysql_real_escape_string(), as it's name implies, is for string values. That is not always the appropriate method based upon the type of values you need to escape.

 

Looking at your code above, it looks like the only potentially user submitted value is $id. I would "assume" that the $id value is expected to be in integer. So, you should use a validation that ensures it is such.

 

You could use is_int() and reject the input if that returns false or, alternatively, you could use "(int) $id" to force the value to be an integer.

[cssfreakie]

Good point mjdamato 

 

So in a nutshell, always make sure that the values are as expected.