• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 1196
  • Last Modified:

SQL Vulnerability fix. Escape character hack protection?

Firstly, I have absolutely no knowledge on SQL, so I probably need a beginners guide to fixing this.

We have a form on a website that we have inherited and we have been advised that there is a security issue. The comment was made that the following error happened when someone enters an inverted comma ' in a comment box:

“Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'm interested in your product'

We have been told that his is because our website developer is not ‘escaping’ the characters that are entered, which leaves the side wide open for security breaches (see http://en.wikipedia.org/wiki/SQL_injection).  

This came from someone who used the form and now wants to sell us a fix. His less helpful remark was
"If I wanted, I could hack your database. If you’d like help putting it right, my company will fix the code and maintain the site properly for £££ a month "

I don't react to kindly to extortion so I did not take the offer. But I like to fix this. I just don't know how.

Any help would be appreciated.

Thanks
capt.
0
captain
Asked:
captain
  • 16
  • 13
  • 3
  • +3
3 Solutions
 
antony_kibble<!-8D58D5C365651885FB5A77A120C8C8C6-->Commented:
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.
0
 
Philip PinnellCommented:
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
0
 
captainAuthor Commented:
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?
0
The new generation of project management tools

With monday.com’s project management tool, you can see what everyone on your team is working in a single glance. Its intuitive dashboards are customizable, so you can create systems that work for you.

 
Philip PinnellCommented:
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
0
 
Philip PinnellCommented:
I take it you do have access to the source
0
 
Philip PinnellCommented:
0
 
captainAuthor Commented:
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...

0
 
captainAuthor Commented:
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?
0
 
captainAuthor Commented:
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);
	  }
    }
  }
?>

Open in new window


What changes do I need to make? or are they to be made elsewhere?
0
 
strickddCommented:
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();
0
 
Philip PinnellCommented:
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))

Open in new window

0
 
Philip PinnellCommented:
What strickdd says is true but it is getting further into "tongue" speaking territory.
0
 
grimkinCommented:
Hi there,

assuming you're using MySQL, a quick fix for this form would be to use the mysql_real_escape_string() function where you bring in the $POST variables so:


  $strdeliveryEmailAddress = $_POST["frm_contact_email"];

would become:

$strdeliveryEmailAddress = mysql_real_escape_string($_POST["frm_contact_email"]);


  $to_one = $_POST["frm_contact_email_inf"];

would become:

  $to_one = mysql_real_escape_string($_POST["frm_contact_email_inf"]);

and so on ..

More info:
http://www.php.net/manual/en/function.mysql-real-escape-string.php

HTH
0
 
Philip PinnellCommented:
@grimkin

is it better to apply the mysql_real_escape_string to each variable or just to the resultant query string?
0
 
Guy Hengel [angelIII / a3]Billing EngineerCommented:
>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.
0
 
captainAuthor Commented:
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....
0
 
captainAuthor Commented:
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?
0
 
captainAuthor Commented:
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";

Open in new window

0
 
Philip PinnellCommented:
I would say you need

$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)
      
before

      $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')";
      
0
 
Philip PinnellCommented:
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
0
 
captainAuthor Commented:
<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?
0
 
Guy Hengel [angelIII / a3]Billing EngineerCommented:
>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.

yes.

in regards to this code:
// 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";

Open in new window


you need to either escape each of the $frm_xxx variables, OR escape the $message variable once it is build. but not both, otherwise you get escape characters stored in the db, which you don't want.
0
 
Philip PinnellCommented:
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?
0
 
captainAuthor Commented:
<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?
0
 
Guy Hengel [angelIII / a3]Billing EngineerCommented:
>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


0
 
Philip PinnellCommented:
Sorry, I don't see why you would want to escape the message. This is going into an email, not the database
0
 
captainAuthor Commented:
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.
0
 
captainAuthor Commented:
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?
0
 
captainAuthor Commented:
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);
    }
  }
?>

Open in new window

0
 
captainAuthor Commented:
AHA!

missing ; at the end of each string...
0
 
captainAuthor Commented:
Sent. Now awaiting message coming in to see if escaped...
0
 
Philip PinnellCommented:
doh! soz
0
 
captainAuthor Commented:
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?
0
 
captainAuthor Commented:
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.
0
 
Philip PinnellCommented:
thanks
0

Featured Post

Hire Technology Freelancers with Gigs

Work with freelancers specializing in everything from database administration to programming, who have proven themselves as experts in their field. Hire the best, collaborate easily, pay securely, and get projects done right.

  • 16
  • 13
  • 3
  • +3
Tackle projects and never again get stuck behind a technical roadblock.
Join Now