[RESOLVED] mysql_real_escape_string() problems

drunkencelt

Hi,

I've spent today trying to learn a bit about designing in security particularly using the mysql_real_escape_string(). I've been looking at the guidance manual and have tried to apply the 'best practice' example to protect connections to the DB.

I have used the 'best practice' example to amend a script that I know works but when I use the 'best practice' script, the page will not pull data across from the DB as it should. There are no error codes and the page seems to parse ok, just no data.

The page is an html form which is pre-filled with data from the DB. Clients can change the DB fields by overtyling the form fields.

Can anyone assist please with what might be wrong?

Many thanks

My code:

<body>
<?php

#connect to MySQL
$conn=@mysql_connect( "localhost", "user", "pass" )
		 	or die( "Err:Conn" );

#select the specified database
$rs = @mysql_select_db( "cheviot2000", $conn ) 
			or die( "Err:Db" );

#create the query 
$sql = "select * from race_admin";

#execute the query
$rs = mysql_query( $sql, $conn );

#close connection to mysql
mysql_close();

#assign data to variable
$row = mysql_fetch_array( $rs );

?>

<h1>Web Site Administration</h1>
<form action="update_race_admin.php" method="post">
	<fieldset>
		<label for="race_d">Race Date:</label><input name="race_date" id="race_d" maxlength="10" value="<?php echo( $row["race_date"] ); ?>" /><br />
		<label for="open_d">Entry Open Date:</label><input name="open_date" id="open_d" maxlength="10" value="<?php echo( $row["open_date"] ); ?>" /><br />
		<label for="close_d">Entry Closed Date:</label><input name="close_d" id="close_d" maxlength="10" value="<?php echo( $row["close_date"] ); ?>" /><br />
		<label for="fee">Entry Fee:</label><input name="fee" id="fee" maxlength="3" value="<?php echo( $row["entry_fee"] ); ?>" /><br />
		<label for="record">Course Record Held by:</label><input name="record" id="record" maxlength="100" value="<?php echo( $row["cur_record"] ); ?>" />
		<label for="record_time">Time:</label><input name="record_time" id="record_time" maxlength="20" value="<?php echo( $row["record_time"] ); ?>" />
		</fieldset>
	<fieldset>
		<label for="news">Latest News:</label><textarea name="news" cols="15" rows="10" id="news"><?php echo( $row["news"] ); ?></textarea>
	</fieldset>
	<fieldset>
		<label for="start_details">Race Start Details:</label><input name="start_details" id="start_details" maxlength="80" value="<?php echo( $row["start_details"] ); ?>" /><br />
		<label for="presentation_details">Presentation Details:</label><input name="presentation_details" id="presentation_details" maxlength="80" value="<?php echo( $row["present_details"] ); ?>" />
	</fieldset>
	<fieldset>
		<label for="app_add">Application Address:</label><textarea name="app_add" id="app_add" cols="15" rows="10"><?php echo( $row["app_add"] ); ?></textarea><br />
		<label for="app_form">Application Form:</label><input type="file" name="app_form" id="app_form" /> 
	</fieldset>
	<fieldset>
		<input type="submit" value="Update Site Details" /><input type="reset" value="Reset Form" />
	</fieldset>
</form>
</body>

And the suggested best practice script from this site:

<?php
// Quote variable to make safe
function quote_smart($value)
{
   // Stripslashes
   if (get_magic_quotes_gpc()) {
       $value = stripslashes($value);
   }
   // Quote if not integer
   if (!is_numeric($value)) {
       $value = "'" . mysql_real_escape_string($value) . "'";
   }
   return $value;
}

// Connect
$link = mysql_connect('mysql_host', 'mysql_user', 'mysql_password')
   OR die(mysql_error());

// Make a safe query
$query = sprintf("SELECT * FROM users WHERE user=%s AND password=%s",
           quote_smart($_POST['username']),
           quote_smart($_POST['password']));

mysql_query($query);
?>

laserlight

Dang, we suggested that? It looks suspicious to me since it determines whether or not to quote based on whether the input the numeric. However, numeric input could be received for a column that requires string input, upon which there would be no quoting and there would be an SQL error.

At a glance, it looks like you did not select a database. Could that be the problem?

drunkencelt

Hi,

Thanks for looking, however I have selected the database "cheviot2000" beginning line 6 of my code and I get no errors at all.

Is there a better method for protecting connections that you prefer?

Cheers

burfo

Can you print out the value of $query? That'll help to see what's happening.

drunkencelt

Thanks Burfo but I'm not sure what you are asking for with

Can you print out the value of $query? That'll help to see what's happening.

If you mean what do I get when the code runs I get blank form fields. The page looks like it should but the data does not show as pre-filled field entries as it should. When I run the page without the suggested 'best practice' script I get what I want.

I'll post the code that does work for comparison:

<body>
<?php

#connect to MySQL
$conn=@mysql_connect( "localhost", "user", "password" )
		 	or die( "Err:Conn" );

#select the specified database
$rs = @mysql_select_db( "cheviot2000", $conn ) 
			or die( "Err:Db" );

#create the query 
$sql = "select * from race_admin";

#execute the query
$rs = mysql_query( $sql, $conn );

#close connection to mysql
mysql_close();

#assign data to variable
$row = mysql_fetch_array( $rs );

?>

<h1>Web Site Administration</h1>
<form action="update_race_admin.php" method="post">
	<fieldset>
		<label for="race_d">Race Date:</label><input name="race_date" id="race_d" maxlength="10" value="<?php echo( $row["race_date"] ); ?>" /><br />
		<label for="open_d">Entry Open Date:</label><input name="open_date" id="open_d" maxlength="10" value="<?php echo( $row["open_date"] ); ?>" /><br />
		<label for="close_d">Entry Closed Date:</label><input name="close_d" id="close_d" maxlength="10" value="<?php echo( $row["close_date"] ); ?>" /><br />
		<label for="fee">Entry Fee:</label><input name="fee" id="fee" maxlength="3" value="<?php echo( $row["entry_fee"] ); ?>" /><br />
		<label for="record">Course Record Held by:</label><input name="record" id="record" maxlength="100" value="<?php echo( $row["cur_record"] ); ?>" />
		<label for="record_time">Time:</label><input name="record_time" id="record_time" maxlength="20" value="<?php echo( $row["record_time"] ); ?>" />
		</fieldset>
	<fieldset>
		<label for="news">Latest News:</label><textarea name="news" cols="15" rows="10" id="news"><?php echo( $row["news"] ); ?></textarea>
	</fieldset>
	<fieldset>
		<label for="start_details">Race Start Details:</label><input name="start_details" id="start_details" maxlength="80" value="<?php echo( $row["start_details"] ); ?>" /><br />
		<label for="presentation_details">Presentation Details:</label><input name="presentation_details" id="presentation_details" maxlength="80" value="<?php echo( $row["present_details"] ); ?>" />
	</fieldset>
	<fieldset>
		<label for="app_add">Application Address:</label><textarea name="app_add" id="app_add" cols="15" rows="10"><?php echo( $row["app_add"] ); ?></textarea><br />
		<label for="app_form">Application Form:</label><input type="file" name="app_form" id="app_form" /> 
	</fieldset>
	<fieldset>
		<input type="submit" value="Update Site Details" /><input type="reset" value="Reset Form" />
	</fieldset>
</form>
</body>

This works great but I doubt it's secure.

Is this what you mean?

NogDog

laserlight wrote:
...It looks suspicious to me since it determines whether or not to quote based on whether the input the numeric. However, numeric input could be received for a column that requires string input, upon which there would be no quoting and there would be an SQL error....

A numeric value (one that is recognized as a valid number by MySQL) does not need to be quoted, even if it is to be used in a string column: it will be cast to a string as needed.

drunkencelt wrote:
Thanks Burfo but I'm not sure what you are asking for with

Can you print out the value of $query? That'll help to see what's happening.

What s/he is suggesting is to echo out the value of the query string being used to make sure it looks OK and has the expected values. For debugging, you could do something like:

$result = mysql_query($query) or die("Query failed: " . mysql_error() . "<br />\n$query");
if(mysql_num_rows($result) == 0)
{
   echo "No data returned by query:<br />\n$query";
}

This way if the query fails or does not return any matches, you can see any error message if applicable, plus the query exactly as it was sent to MySQL. (For the production version, you will want to log errors instead of displaying them, and "gracefully" fail, without displaying database details to the world.)

laserlight

A numeric value (one that is recognized as a valid number by MySQL) does not need to be quoted, even if it is to be used in a string column: it will be cast to a string as needed.

Interesting, this sounds like manifest typing, and I thought only SQLite had such a feature (or "misfeature", as the case may be).

burfo

I read the original post again and now I'm confused. You modified your code that stores the data into the DB. Which in turn broke the code that retrieves the data from the DB?

drunkencelt

People,

I do feel such an idiot as I have managed to get things to work now.

I had not fully apreciated that the example given as 'best practice' related to the user and password details being passed from a login page. Once I changed the POST data in the example (below) to variables containing a test user and password it worked.

$query = sprintf("SELECT * FROM users WHERE user=%s AND password=%s", 
           quote_smart($_POST['username']), 
           quote_smart($_POST['password']));

Because of your tips I was able to reason the problem out for myself which I suppose is part of the game.

NogDog - I used your suggestion for printing out the query/result which helped although strangely it echos the reply whether data is returned or not. I understand it should only echo where no data is returned. I know data is being returned because I can see it on the page where it should be.

This is what I have:

$query = sprintf("SELECT * FROM race_admin WHERE user=%s AND password=%s",
           quote_smart($user),
           quote_smart($password));

$result = mysql_query($query,$conn)or die("Query failed: " . mysql_error() . "<br />\n $query"); 
if(mysql_num_rows($result) == 0);
 {
 echo "No data returned by query:<br />\n$query";
 }

And this is what it produces in the browser:

No data returned by query:
SELECT * FROM race_admin WHERE user='username' AND password='test'
Web Site Administration <--- rest of the page showing the form and prefilled data etc

Burfo - Sorry. Maybe I didn't explain what the page does clearly enough. This page is in effect a form which a client will use to change dates and other details for a fell race on a website. The form fields will be pre-populated from the database using the 'value' attribute which shows the client what the current data is. There will only ever be one record in the database as all the client will do is update and never be able to add. They can overwrite the data in the form fields - which will update the DB - which will in turn update the relevant part of the website. It all worked okay before I tried to make it more secure by using the 'best practice' code. That's what I meant.

ALL - Is there a better way to make queries secure? Any other advice or articles you would recommend?

Thanks again for your time. Every problem solved / examined helps me to learn and I'm going to crack this lark if it takes me the rest of the year - although I suspect you never actually stop learning!
🙂

NogDog

The only reason I can think of that the mysql_num_rows() would act like that would be if it were returning a boolean false for some reason, which would be evaluated as a 0 in the comparison. This might happen if the $result variable was invalid, such as a typo, or if you had released the DB result or the DB connection, but none of those appear to be the case based on the code sample provided.

If you want to debug it further, you could change the check to:

$rows = mysql_num_rows($result);
if($rows === false)   // note use of "===" instead of "=="
{
   echo "Ay Carumba! What happened here?";
}
elseif($rows === 0)
{
   echo "No results returned, etc....";
}

It might also be useful to turning on all errors while debugging, if you have not already done so:

<?php
ini_set('display_errors', 1);
error_reporting(E_ALL | E_STRICT);

drunkencelt

Hi NogDog,

I've tried what you suggested and it seems to work ok now - No warnings. I've changed nothing else other than to make sure errors are turned on as you suggested.

$query = sprintf("SELECT * FROM race_admin WHERE user=%s AND password=%s",
           quote_smart($user),
           quote_smart($password));

$result = mysql_query($query,$conn)or die("Query failed: " . mysql_error() . "<br />\n $query"); 
$rows = mysql_num_rows($result); 
if($rows === false)   // note use of "===" instead of "==" 
{ 
   echo "Ay Carumba! What happened here?"; 
} 
elseif($rows === 0) 
{ 
   echo "No data returned by query:<br />\n$query"; 
}

Is my understanding of this check correct? If the result returns 'false' echo 'No data etc'. If the result is 'no value' or '0', echo 'No date etc'?

What does the "===" mean?

A little strange is it not, because if the previous check returned a false or a 0 as thought then you would think that at least one of the echo's would be triggered?!

Never mind. Seems ok now.

Many thanks

laserlight

What does the "===" mean?

Equivalent, i.e., equal in both value and type. The idea is that 0 can be evaluated as false, yet is not a boolean value.

burfo

drunkencelt wrote:
What does the "===" mean?

"==" tests "is equal to", whereas "===" tests "is identical to" (equal with the same type).

if (false == 0)   //true
if (false === 0)  //false

http://us.php.net/operators.comparison

drunkencelt

Ah I understand.

You've all kindly resolved just about everything here for me now.

Many thanks to one and all and have a great festive season!

Probably leave this stuff alone now for a few days....it eats up all my time and I forget to eat!...well almost!

Cheers 😃