SQL Vulnerability fix. Escape character hack protection?

at a guess, In the SQL syntax script, change the "I'm interested in your product" to "I am interested in your product."

That inverted comma in I'm is causing issues.

What is happening is the code is creating a query based on the input from the field

Maybe

Update commentTable
set comment = "'" & avariablecontainingthecomment & "'"

now because ' is a string delimitter the query ends up

Update commentTable
set comment = 'I'm interested in your product'

this has unbalanced ' quotes and results in an error.

Now that's the problem, fixing it depends on the code that is being used.

There are a number of ways in which SQL injection vunerabilties can be handled and I am not an expert. There may be many places you have the issue. With luck where the user enters a comment may be the only place.

I listen to what you say before I waffle any further

I am not sure.

It seems that the apostrophe in "I'm" triggered this. So by chance you only notice in comment fields where this is likely unless a name field contains "O'Toole" etc.

So I guess this needs to be fixed across all fields, which is 15 across name, address, email and comment...

My intial question was: What tools do i need and where do I change these things?

So what code runs the site? PHP, perl, .Net, something else.

With the first two I'd say just a text editor. .Net you can get free express Studios for Web dev.

I suspect it is mostl likely PHP and I know little about this.

You might want an open source MySQL administrator. If the solution were to be using stored procedures rather than building the query in code

I take it you do have access to the source

http://www.tizag.com/mysqlTutorial/mysql-php-sql-injection.php

This might be useful

LOL, you speak in tongues.

I know it is PHP, but have no idea were to look for SQL syntax. Looking at the link I am none the wiser as I have got no clue where to start.

I guess I really need a step by step for dummies here...

The only thing I can see is in the source:
<p class="p-textarea">
                        <textarea cols="" rows="" id="frm_contact_comments" name="frm_contact_comments"></textarea>

Do I need to look for the 'frm_contact_comments' or are the changes to be made in here directly?

There's a thought.

I have found: proc_contactfrm.inc.php which contains the code as below...

<?php
include('includes/corefuncs.php');
if (function_exists('nukeMagicQuotes')) {
  nukeMagicQuotes();
  }

// process the email
/*if (array_key_exists('reset', $_POST)) {
    $mailSent = false;
	unset($missing);
	}*/
if (array_key_exists('frm_contact_submit', $_POST)) {
  $emailsubject = 'SomeName.org Web Site Enquiry';
  $strdeliveryEmailAddress = $_POST["frm_contact_email"];
  $to_one = $_POST["frm_contact_email_inf"];
  $to_two = $_POST["frm_contact_email_sup"];
  $to_three = $_POST["frm_contact_email_lic"];
  $to_eins = $_POST["frm_contact_email_eu"];
  $to_zwei = $_POST["frm_contact_email_uk"];
  $to_drei = $_POST["frm_contact_email_us"];  
  $to = "$to_one , $to_two , $to_three , $to_eins , $to_zwei , $to_drei "; 
  
    
  // list expected fields
  $expected = array('frm_contact_fname', 'frm_contact_lname', 'frm_contact_title', 'frm_contact_bname', 'frm_contact_baddress_1', 'frm_contact_baddress_2', 'frm_contact_baddress_3', 'frm_contact_city', 'frm_contact_state', 'frm_contact_country', 'frm_contact_zip', 'frm_contact_phone', 'frm_contact_email', 'frm_contact_comments');
  // set required fields
  $required = array('frm_contact_fname', 'frm_contact_lname', 'frm_contact_bname', 'frm_contact_baddress_1', 'frm_contact_city', 'frm_contact_state', 'frm_contact_country', 'frm_contact_zip', 'frm_contact_email');
 // create empty array for any missing fields
  $missing = array();
  
  // assume that there is nothing suspect
  $suspect = false;
  // create a pattern to locate suspect phrases
  $pattern = '/Content-Type:|Bcc:|Cc:/i';
  
  // function to check for suspect phrases
  function isSuspect($val, $pattern, &$suspect) {
    // if the variable is an array, loop through each element
	// and pass it recursively back to the same function
	if (is_array($val)) {
      foreach ($val as $item) {
	    isSuspect($item, $pattern, $suspect);
	    }
	  }
    else {
      // if one of the suspect phrases is found, set Boolean to true
	  if (preg_match($pattern, $val)) {
        $suspect = true;
	    }
	  }
    }
  
  // check the $_POST array and any sub-arrays for suspect content
  isSuspect($_POST, $pattern, $suspect);
  
  if ($suspect) {
    $mailSent = false;
	unset($missing);
	}
  else {
    // process the $_POST variables
    foreach ($_POST as $key => $value) {
      // assign to temporary variable and strip whitespace if not an array
      $temp = is_array($value) ? $value : trim($value);
	  // if empty and required, add to $missing array
	  if (empty($temp) && in_array($key, $required)) {
	    array_push($missing, $key);
	    }
	  // otherwise, assign to a variable of the same name as $key
	  elseif (in_array($key, $expected)) {
	    ${$key} = $temp;
	    }
	  }
	}
  
  // validate the email address
  if (!empty($frm_contact_email)) {
    // regex to ensure no illegal characters in email address 
	$checkEmail = '/^[^@]+@[^\s\r\n\'";,@%]+$/';
	// reject the email address if it doesn't match
	if (!preg_match($checkEmail, $frm_contact_email)) {
	  array_push($missing, 'frm_contact_email');
	  }
	}
  
  // go ahead only if not suspect and all required fields OK
  if (!$suspect && empty($missing)) {

//insert info into database
	include('includes/db_connect.inc.php');
  
	$sql= "INSERT INTO users (source, firstname, lastname, title, company, address, address2, address3, city, state, country, zip, phone, email, comments) 
	VALUES ('Contact Us', '$frm_contact_fname', '$frm_contact_lname', '$frm_contact_title', '$frm_contact_bname', '$frm_contact_baddress_1', '$frm_contact_baddress_2', '$frm_contact_baddress_3', '$frm_contact_city', '$frm_contact_state', '$frm_contact_country', '$frm_contact_zip', '$frm_contact_phone', '$frm_contact_email', '$frm_contact_comments')";
	

	if (!mysql_query($sql,$con))
    {
    die('Error: ' . mysql_error());
    }
	mysql_close($con);
   
	
	
    // build the message
	$message = "First Name: $frm_contact_fname\n\n";
	$message .= "Last Name: $frm_contact_lname\n\n";
	$message .= "Title: $frm_contact_title\n\n";
	$message .= "Business Name: $frm_contact_bname\n\n";
	$message .= "Business Address: $frm_contact_baddress_1 $frm_contact_baddress_2 $frm_contact_baddress_3\n\n";
	$message .= "City: $frm_contact_city\n\n";
	$message .= "State: $frm_contact_state\n\n";
	$message .= "Country: $frm_contact_country\n\n";
	$message .= "Zip: $frm_contact_zip\n\n";
	$message .= "Phone: $frm_contact_phone\n\n";
	$message .= "Email: $frm_contact_email\n\n";
	$message .= "Comments: $frm_contact_comments\n\n";

    // limit line length to 70 characters
    $message = wordwrap($message, 70);
	
	// create additional headers
	$additionalHeaders = "From: $strdeliveryEmailAddress";
	if (!empty($email)) {
	  $additionalHeaders .= "\r\nReply-To: $frm_contact_email";
	  }
	
    // send it  
    $mailSent = mail($to, $emailsubject, $message, $additionalHeaders);
	if ($mailSent) {
	  unset($missing);
	  }
    }
  }
?>

Select allOpen in new window

What changes do I need to make? or are they to be made elsewhere?

Cleaning the input is a prevention that is not very effective. The best way to stop SQL Injection is to use parameterized SQL.

http://unixwiz.net/techtips/sql-injection.html

$preparedStatement = $db->prepare('SELECT * FROM employees WHERE name = :name');

$preparedStatement->execute(array(':name' => $name));

$rows = $preparedStatement->fetchAll();

Sorry I was typing and EE went down

I was saying it might be difficult to locate all areas of code that might need changing.  

Searching for "mysql_query" (the command that executes a query) might help.

From what you have posted it looks like the change below might be sufficient.


No disrespect taken, I would welcome more "expert" expert's opinion here

REPLACE

//insert info into database
	include('includes/db_connect.inc.php');
  
	$sql= "INSERT INTO users (source, firstname, lastname, title, company, address, address2, address3, city, state, country, zip, phone, email, comments) 
	VALUES ('Contact Us', '$frm_contact_fname', '$frm_contact_lname', '$frm_contact_title', '$frm_contact_bname', '$frm_contact_baddress_1', '$frm_contact_baddress_2', '$frm_contact_baddress_3', '$frm_contact_city', '$frm_contact_state', '$frm_contact_country', '$frm_contact_zip', '$frm_contact_phone', '$frm_contact_email', '$frm_contact_comments')";
	

	if (!mysql_query($sql,$con))

WITH

//insert info into database
	include('includes/db_connect.inc.php');
  
	$sql= "INSERT INTO users (source, firstname, lastname, title, company, address, address2, address3, city, state, country, zip, phone, email, comments) 
	VALUES ('Contact Us', '$frm_contact_fname', '$frm_contact_lname', '$frm_contact_title', '$frm_contact_bname', '$frm_contact_baddress_1', '$frm_contact_baddress_2', '$frm_contact_baddress_3', '$frm_contact_city', '$frm_contact_state', '$frm_contact_country', '$frm_contact_zip', '$frm_contact_phone', '$frm_contact_email', '$frm_contact_comments')";
	
        $sql = mysql_real_escape_string($sql)

	if (!mysql_query($sql,$con))

Select allOpen in new window

What strickdd says is true but it is getting further into "tongue" speaking territory.

@grimkin

is it better to apply the mysql_real_escape_string to each variable or just to the resultant query string?

>is it better to apply the mysql_real_escape_string to each variable or just to the resultant query string?
actually, doing the escape on the full sql string will give eventually completely bad results/sql, so you have to escape on the values as such.

the alternative is to use bindings, but that will require a full rewrite of most of the code.

Thanks a3, to recap I

Is there a security issue?
How do I go about fixing it?

If I understand correctly I need to replace the code in the PHP to include the statement:
 mysql_real_escape_string(*)  whereof * is the $_post value in existence.

As I have 15 fields I need to replace this for each field, which then should not create the said error from the Q.

I will try that, please scream and shout if i run in the wrong direction trying this....

OK I am stuck again as the only two lines I find are the ones cited by grimkin, but I cannot see the same for the other fields. Would this be in the PHP posted or should I look elsewhere?

Is the code to be changed the message code below?:

// build the message
	$message = "First Name: $frm_contact_fname\n\n";
	$message .= "Last Name: $frm_contact_lname\n\n";
	$message .= "Title: $frm_contact_title\n\n";
	$message .= "Business Name: $frm_contact_bname\n\n";
	$message .= "Business Address: $frm_contact_baddress_1 $frm_contact_baddress_2 $frm_contact_baddress_3\n\n";
	$message .= "City: $frm_contact_city\n\n";
	$message .= "State: $frm_contact_state\n\n";
	$message .= "Country: $frm_contact_country\n\n";
	$message .= "Zip: $frm_contact_zip\n\n";
	$message .= "Phone: $frm_contact_phone\n\n";
	$message .= "Email: $frm_contact_email\n\n";
	$message .= "Comments: $frm_contact_comments\n\n";

Select allOpen in new window

Ah. Hold on.

You probably want to move that message code above where you use mysql_real_escape_string or you will end up with escaped characters in your message.

Either that or have seperate variables to hold the escaped strings

<scratches head>

I am not sure what you mean, but that may not surprise you...

I assume you mean to NOT change anything but to INSERT your code suggestion above the "$sql= "INSERT INTO" string.

Why do you think the //message builder needs moving? Doesn't the variable setting for the mysql_real_escape specify the way the data is handled?

Sorry not following here

For the $sql= ...

insert statement each of the $frm_xxx variable should be escaped, however if you do
$frm_contact_fname = mysql_real_escape_string($frm_contact_fname) etc

This should be ok for the insert into the Db.

$message  is a string that is put in an email and is built from these variables and you would not want the escaped variables in the message.

I was suggesting building the message before escaping each of the variables.

... or have I lost the plot?

<mini eureka>

So you say the message needs to be built before the characters are escaped. makes sense. Does it apply to this or is there a reason to assume that the message will be built using the input data regardless?

>So you say the message needs to be built before the characters are escaped. makes sense.

I actually said that you do EITHER way:
* escape the message, but not the variables
* escape the variables, but not the message

Sorry, I don't see why you would want to escape the message. This is going into an email, not the database

The plot thickens;

At the top it says 'nukeMagicQuotes', looking into  this there is a corefunctions php that has stripslashes functions included. Is this related?

Well I will try the suggestions above and see if it makes a difference.

OK, if I insert the

$frm_contact_fname = mysql_real_escape_string($frm_contact_fname)
$frm_contact_lname = mysql_real_escape_string($frm_contact_lname)
$frm_contact_title = mysql_real_escape_string($frm_contact_title)
$frm_contact_bname = mysql_real_escape_string($frm_contact_bname)
$frm_contact_baddress_1 = mysql_real_escape_string($frm_contact_baddress_1)
$frm_contact_baddress_2 = mysql_real_escape_string($frm_contact_baddress_2)
$frm_contact_baddress_3 = mysql_real_escape_string($frm_contact_baddress_3)
$frm_contact_city = mysql_real_escape_string($frm_contact_city)
$frm_contact_state = mysql_real_escape_string($frm_contact_state)
$frm_contact_country = mysql_real_escape_string($frm_contact_country)
$frm_contact_zip = mysql_real_escape_string($frm_contact_zip)
$frm_contact_phone = mysql_real_escape_string($frm_contact_phone)
$frm_contact_email = mysql_real_escape_string($frm_contact_email)
$frm_contact_comments  = mysql_real_escape_string($frm_contact_comments)

anywhere the PHP form does not load anymore.

The message is indeed an email. So we don't want to change the message only what is put into the db.

What am I doing wrong inserting the above?

just for completeness, here is the corefunction code relating to MagicQuote nuking:

<?php
function nukeMagicQuotes() {
  if (get_magic_quotes_gpc()) {
    function stripslashes_deep($value) {
      $value = is_array($value) ? array_map('stripslashes_deep', $value) : stripslashes($value);
      return $value;
      }
    $_POST = array_map('stripslashes_deep', $_POST);
    $_GET = array_map('stripslashes_deep', $_GET);
    $_COOKIE = array_map('stripslashes_deep', $_COOKIE);
    }
  }
?>

Select allOpen in new window

AHA!

missing ; at the end of each string...

Sent. Now awaiting message coming in to see if escaped...

doh! soz

So it seems working well, I take if i don't get the error message anymore I can be sure that the exploit is removed?

Thanks to all, I have selected the answers most appropriate to my solution. I trust that other solutions may have also worked but would have needed rewrites or a different approach.

This simply worked after using the cleaned up code after the message builder.
capt.

thanks