Solved

SQL Vulnerability fix. Escape character hack protection?

Posted on 2011-03-08
38
1,156 Views
Last Modified: 2012-05-11
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
Comment
Question by:captain
  • 16
  • 13
  • 3
  • +3
38 Comments
 
LVL 12
ID: 35067885
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
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35067928
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
 
LVL 30

Author Comment

by:captain
ID: 35068003
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
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35068099
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
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35068111
I take it you do have access to the source
0
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35068198
0
 
LVL 30

Author Comment

by:captain
ID: 35068239
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
 
LVL 30

Author Comment

by:captain
ID: 35068256
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
 
LVL 30

Author Comment

by:captain
ID: 35068273
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
 
LVL 28

Expert Comment

by:strickdd
ID: 35068421
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
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35068437
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
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35068445
What strickdd says is true but it is getting further into "tongue" speaking territory.
0
 
LVL 14

Accepted Solution

by:
grimkin earned 250 total points
ID: 35068536
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
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35068613
@grimkin

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

Expert Comment

by:Guy Hengel [angelIII / a3]
ID: 35068841
>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
 
LVL 30

Author Comment

by:captain
ID: 35068971
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
 
LVL 30

Author Comment

by:captain
ID: 35069006
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
Enabling OSINT in Activity Based Intelligence

Activity based intelligence (ABI) requires access to all available sources of data. Recorded Future allows analysts to observe structured data on the open, deep, and dark web.

 
LVL 30

Author Comment

by:captain
ID: 35069023
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
 
LVL 13

Assisted Solution

by:Philip Pinnell
Philip Pinnell earned 150 total points
ID: 35069194
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
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35069214
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
 
LVL 30

Author Comment

by:captain
ID: 35069317
<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
 
LVL 142

Assisted Solution

by:Guy Hengel [angelIII / a3]
Guy Hengel [angelIII / a3] earned 100 total points
ID: 35069434
>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
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35069658
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
 
LVL 30

Author Comment

by:captain
ID: 35069771
<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
 
LVL 142

Expert Comment

by:Guy Hengel [angelIII / a3]
ID: 35069833
>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
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35069861
Sorry, I don't see why you would want to escape the message. This is going into an email, not the database
0
 
LVL 30

Author Comment

by:captain
ID: 35069908
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
 
LVL 30

Author Comment

by:captain
ID: 35070011
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
 
LVL 30

Author Comment

by:captain
ID: 35070038
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
 
LVL 30

Author Comment

by:captain
ID: 35070074
AHA!

missing ; at the end of each string...
0
 
LVL 30

Author Comment

by:captain
ID: 35070131
Sent. Now awaiting message coming in to see if escaped...
0
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35070183
doh! soz
0
 
LVL 30

Author Comment

by:captain
ID: 35071578
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
 
LVL 30

Author Closing Comment

by:captain
ID: 35071625
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
 
LVL 13

Expert Comment

by:Philip Pinnell
ID: 35073156
thanks
0

Featured Post

Maximize Your Threat Intelligence Reporting

Reporting is one of the most important and least talked about aspects of a world-class threat intelligence program. Here’s how to do it right.

Join & Write a Comment

Although it can be difficult to imagine, someday your child will have a career of his or her own. He or she will likely start a family, buy a home and start having their own children. So, while being a kid is still extremely important, it’s also …
Ever wondered why sometimes your SQL Server is slow or unresponsive with connections spiking up but by the time you go in, all is well? The following article will show you how to install and configure a SQL job that will send you email alerts includ…
Via a live example, show how to shrink a transaction log file down to a reasonable size.
Via a live example, show how to setup several different housekeeping processes for a SQL Server.

758 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

22 Experts available now in Live!

Get 1:1 Help Now