Another SQL injection question / check my function

[mo]

I pass all user input fields through the below function before I use said field(s) in a query. Does this function look OK. Should I add some more checks? I believe I copied this function from another site.

 

The is also basic form validation on my user fields before they get to this function, like checking for numeric values, etc.

 

function checkSQLinject($string){

if(get_magic_quotes_gpc()) {
	$string = stripslashes($string);
}
if(function_exists("mysql_real_escape_string")) {
	$string = mysql_real_escape_string($string);
}
else {
	$string = addslashes($string);
}
return $string;		
}

[mrMarcus]

things look ok to me .. best way to tell is trying it out in different scenarios so you can see for yourself what it's doing.

 

persnoally, with functions, i like to keep my var names different, for example:

 

function checkSQLinject($input){

   if(get_magic_quotes_gpc()) {
      $output = stripslashes($input);
   }
   if(function_exists("mysql_real_escape_string")) {
      $output = mysql_real_escape_string($output);
   }
   else {
      $output = addslashes($input);
   }
   return $output;      
}

this way you can keep track of what's coming in and what's going out.

[mo]

Thanks for the tip on variable names. I updated the function.

 

Do you see an issue with me doing the following when validating fields?

 

$_POST['UserName'] = checkSQLinject($_POST['UserName']);

or should I use new variables?

$UserName = checkSQLinject($_POST['UserName']);

[mrMarcus]

Thanks for the tip on variable names. I updated the function.

 

Do you see an issue with me doing the following when validating fields?

 

$_POST['UserName'] = checkSQLinject($_POST['UserName']);

or should I use new variables?

$UserName = checkSQLinject($_POST['UserName']);

really depends on what your doing at the time.

 

if you will be using that variable several times over, then yes, rename it to something easier to work with .. but consider looking into sprintf(), that way assigning $_POST values to alternate names isn't necessary, and eliminates extra typing. ie.

 

$sql = mysql_query(sprintf("SELECT * table WHERE id='%d' AND name='%s'", (int)$_SESSION['id'], checkSQLinject($_POST['UserName'])));

 

where '%d' and '%s' are the values $_SESSION['id'] and $_POST['UserName'] .. this method allows for a clean, functional, and more secure way of processing forms with a database.  very easy to use too, just read up on the documentation provded via the link.

[Maq]

If you really want to test it then you can make a test input field with your function called on it and run SQL Inject Me on it.

[Prismatic]

things look ok to me .. best way to tell is trying it out in different scenarios so you can see for yourself what it's doing.

 

persnoally, with functions, i like to keep my var names different, for example:

 

function checkSQLinject($input){

   if(get_magic_quotes_gpc()) {
      $output = stripslashes($input);
   }
   if(function_exists("mysql_real_escape_string")) {
      $output = mysql_real_escape_string($output);
   }
   else {
      $output = addslashes($input);
   }
   return $output;      
}

this way you can keep track of what's coming in and what's going out.

 

It also won't work. You only set $input to $output if magic quotes is on. If it's not, mysql_real_escape_string ends up escaping an empty $output variable.

[mrMarcus]

function sanitize($input)
{
$search = array(
	'@<script[^>]*?>.*?</script>@si',   // Strip out javascript
	'@<[\/\!]*?[^<>]*?>@si',            // Strip out HTML tags
	'@<style[^>]*?>.*?</style>@siU',    // Strip style tags properly
	'@<![\s\S]*?--[ \t\n\r]*>@'         // Strip multi-line comments
);
    $output = preg_replace($search, '', $input);
    return $output;
}

function checkSQLinject($input)
{
    if (is_array($input))
{
        foreach($input as $var=>$val)
	{ $output[$var] = checkSQLinject($val); }
    }
    else {
        if (get_magic_quotes_gpc())
	{ $input = stripslashes($input); }
        $input  = sanitize($input);
        $output = mysql_real_escape_string($input);
    }
    return $output;
}

that's a decent sanitize function(s);

 

i don't scrutinize every bit of code i post on here .. i give the jist if not 100% with the expectation that it will be tested and played around with to ensure that it works.

 

i was just giving a pointer as to go about not confusing the heck outta yourself when dealing with larger-scale function and such .. 'cause if everything's got the same name, it's hard to keep track sometimes.

[Mchl]

Why checking if mysql_real_escape_string exists? If it doesn't neither does mysql_query...