Page 1 of 1
real_escape_string returning error (sql injection function)
Posted: Thu Feb 15, 2007 11:32 am
by visitor-Q
as the topic noted, i'm writing a function to prevent sql injections. first, i would like some criticism on my function. as in, is there a better way to form the function to better prevent sql injections. basically, the function will be applied to every POSTed variable. some of them will be strings, others will be set via dropdown menu. the ones that are set with a dropdown menu will not be affected by the function. the input fields will be in the form of strings (ex name input, address input, etc). these are the POSTed variables i am worried about, since the input fields must allow the user to enter in values up to 40 characters in length. here is my function:
Code: Select all
/*define function to prevent sql injection*/
function clean($var){
$var = mysql_real_escape_string(strip_tags(trim($var)));
return $var;
}
of course, as you may know, this returns an error:
Code: Select all
Warning: mysql_real_escape_string(): Access denied for user: 'apache@localhost' (Using password: NO)
i looked it up in the manual, and i can't find the place to put the argument, or even what to put as the argument. i tried this:
Code: Select all
/*connect to database*/
@ $db = mysql_connect("xxx", "xxx", "xxx");
if(!$db){
echo "Error: Could not connect to the database. Please try again later.";
exit;
}
/*select database*/
mysql_select_db("my_db", $db);
/*define function to prevent sql injection*/
function clean($var){
$var = mysql_real_escape_string(strip_tags(trim($var)), $db);
return $var;
}
and it returns this error:
Code: Select all
Warning: mysql_real_escape_string() expects parameter 2 to be resource, null given
i'm not sure where to go from here. can i get any suggestions?
also, this is the method i am using to apply my function to my POSTed variables:
Code: Select all
/*clean input values to prevent sql injection*/
foreach($_POST as $key => $val){
clean($val);
}
is this suitable?
Posted: Thu Feb 15, 2007 11:50 am
by Oren
It's seems that for one reason or another, you didn't connect to the database. Try to get rid of the '@' you put there and see what happens.
P.S I don't see any use for this function... Why would you want to strip tags when you just save the data in the database? For example, what if you have a web development blog and one of your users wanted to post a comment with some HTML code in it?
Posted: Thu Feb 15, 2007 11:54 am
by visitor-Q
Oren wrote:It's seems that for one reason or another, you didn't connect to the database. Try to get rid of the '@' you put there and see what happens.
P.S I don't see any use for this function... Why would you want to strip tags when you just save the data in the database? For example, what if you have a web development blog and one of your users wanted to post a comment with some HTML code in it?
striptags is useless, you are correct. i'll take that out of my function. however, as for connecting to the database, i know i'm connecting because i have pre-populated input fields where the values are being retrieved from my database. everything worked fine until i created this function and began applying it to my POSTed variables. in fact, even after the error is displayed, it still populated my input fields with the correct information from the database. maybe i'm supplying the wrong argument in the wrong place?
Posted: Thu Feb 15, 2007 12:14 pm
by shwanky
Code: Select all
/*connect to database*/
@ $db = mysql_connect("xxx", "xxx", "xxx");
if(!$db){
echo "Error: Could not connect to the database. Please try again later.";
exit;
}
/*select database*/
mysql_select_db("my_db", $db);
/*define function to prevent sql injection*/
function clean($var){
global $db;
$var = mysql_real_escape_string(strip_tags(trim($var)), $db);
return $var;
}
try this. If your going to pass the connection by reference to a function then you have to set that variable to gloal within the function so that i isn't a local variable of the function.
Posted: Thu Feb 15, 2007 12:15 pm
by Oren
Well, it's impossible to tell without seeing more code and what exactly you are doing there

Posted: Thu Feb 15, 2007 12:45 pm
by visitor-Q
Oren wrote:Well, it's impossible to tell without seeing more code and what exactly you are doing there

i'm not sure i understand why. my code was working perfectly. and now, all i'm trying to do is add a small function to prevent sql injections. so, i thought it was safe to assume that the new code was the only code needed to be posted. =\ is that not the case?
i did notice, however, if i remove the foreach that calls the function, my errors go away. which means the error can be identified in my clean() function. that is why i believe the problem is more than likely simple, and it has to do with the supplied argument for mysql_real_escape_string().
shwanky wrote:
try this. If your going to pass the connection by reference to a function then you have to set that variable to gloal within the function so that i isn't a local variable of the function.
thanks for the tip, shwanky! that information will come in handy, for sure. i tried that but unfortunately it brought me the same error =\
Code: Select all
Warning: mysql_real_escape_string() expects parameter 2 to be resource, null given
Posted: Thu Feb 15, 2007 12:56 pm
by Oren
Sorry, but this doesn't help much. I can't help you with the current information you gave.
I also don't think that sql injections should be prevented by one function, by that I mean, that you should sanitize the data on a per-query basis.
What I mean is, if you do:
Code: Select all
SELECT id FROM tbl WHERE id = '$var'
You expect
$var to be an integer and therefore, you would do something like this:
before using it in your query.
Posted: Thu Feb 15, 2007 1:12 pm
by visitor-Q
thank you, Oren. i understand what you mean, however would it matter what my tables have been defined as? for instance, all of my table columns are defined as VARCHAR. even tho numbers are being inserted into my database, they are being inserted as strings, not intigers. so, would it matter if the expected input was an intiger? would it be processed the same?
since all of my data is being inserted as strings, i assume the function could be applied to all user inputted data, accordingly. here is my entire page:
Code: Select all
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<HTML>
<HEAD><TITLE>Edit Couple</TITLE></HEAD>
<BODY>
<CENTER>
<?php $location = "<B>Edit Couple</B>"; ?>
<TABLE BORDER="1" WIDTH="850" CELLPADDING="20">
<TR>
<TD WIDTH="%50" ALIGN="center"><H1>TITLE</H1>
<?php echo $location; ?>
<form method="POST" action="admin1.php?action=view_all">
<input type="SUBMIT" value="View All">
</form>
</TD>
<TD><?php include "admin_search.inc" ?></TD>
</TR>
</TABLE><BR>
<HR WIDTH="300"><BR>
<TABLE WIDTH="750">
<TR>
<TD>
<?php
/*connect to database*/
@ $db = mysql_connect("localhost", "apache", "nopass");
if(!$db){
echo "Error: Could not connect to the database. Please try again later.";
exit;
}
/*select database*/
mysql_select_db("theverythinggifts", $db);
/*update couple information*/
if(isset($_POST)){
/*define function to prevent sql injection*/
function clean($var){
$var = mysql_real_escape_string(trim($var));
}
/*apply clean() to each $_POST variable*/
foreach($_POST as $key => $val){
clean($val);
}
$sql = "UPDATE my_search_table
SET brideFname = '". $_POST['brideFname'] ."'
brideLname = '". $_POST['brideLname'] ."'
groomFname = '". $_POST['groomFname'] ."'
groomLname = '". $_POST['groomLname'] ."'
event_month = '". $_POST['event_month'] ."'
event_day = '". $_POST['event_day'] ."'
event_year = '". $_POST['event_year'] ."'
ship_add = '". $_POST['ship_add'] ."'
ship_city = '". $_POST['ship_city'] ."'
ship_state = '". $_POST['ship_state'] ."'
ship_zip = '". $_POST['ship_zip'] ."'
WHERE uID = '". $_GET['id'] ."'";
@ mysql_query($sql);
/*confirm/disconfirm update*/
echo "<div align=\"center\">\n";
if(!mysql_query($sql)){
echo "<font color=\"ff0000\">Revisions were not successful. Please try again.</font>\n";
}else{
echo "<font color=\"339933\">Updated information was successful</font>\n";
}
echo "</div>\n";
/*unset $_POST variables*/
unset($_POST);
}
/*query database*/
$sql = mysql_query("SELECT * FROM my_search_table WHERE uID = '". $_GET['id'] ."'") or die(mysql_error());
$row = mysql_fetch_array($sql);
/*pre-populate input fields with values from the database.*/
echo "<TABLE WIDTH=\"700\"><TR><TD>\n";
echo "<form action=\"editCouple.php?id=". $_GET['id'] ."\" method=\"post\">\n";
echo "<div align=\"center\"><b>Bride's Name</div>\n";
echo "<B>First:</B> <input type=\"text\" name=\"brideFname\" value=\"". $row['brideFname'] ."\" size=\"25\"><br />\n";
echo "<B>Last:</B> <input type=\"text\" name=\"brideLname\" value=\"". $row['brideLname'] ."\" size=\"25\"><br /><br />\n";
echo "</TD>\n";
echo "<TD>\n";
echo "<div align=\"center\"><b>Groom's Name</div>\n";
echo "<B>First:</B> <input type=\"text\" name=\"groomFname\" value=\"". $row['groomFname'] ."\" size=\"25\"><br />\n";
echo "<B>Last:</B> <input type=\"text\" name=\"groomLname\" value=\"". $row['groomLname'] ."\" size=\"25\"><br /><br />\n";
echo "</TD></TR>\n";
echo "<TR><TD COLSPAN=\"2\">\n";
echo "<b>Event Date:</b> \n";
/*define months array*/
$array_month = array("January" => "01", "February" => "02", "March" => "03",
"April" => "04", "May" => "05", "June" => "06",
"July" => "07", "August" => "08", "Steptember" => "09",
"October" => "10", "November" => "11", "December" => "12");
echo "<SELECT NAME=\"event_month\">\n";
/*prepopulate the dropdown menu for the event month*/
foreach($array_month as $key => $val){
echo "<OPTION VALUE=\"". $val ."\"". (($val == $row['event_month']) ? (" SELECTED") : ("")) .">". $key ."\n";
}
echo "</SELECT>\n";
/*define days array*/
$days = range(1, 31);
echo"<SELECT NAME=\"event_day\">\n";
/*prepopulate the dropdown menu for the event day*/
foreach($days as $day){
if($day >= 1 && $day <= 9){
$day = "0". $day;
}
echo "<OPTION VALUE=\"". $day ."\"". (($day == $row['event_day']) ? (" SELECTED") : ("")) .">". $day ."\n";
}
echo "</SELECT>\n";
/*define years array*/
$years = range(2006, 2015);
echo"<SELECT NAME=\"event_year\">\n";
/*prepopulate the dropdown menu for the event year*/
foreach($years as $year){
echo "<OPTION VALUE=\"". $year ."\"". (($year == $row['event_year']) ? (" SELECTED") : ("")) .">". $year ."\n";
}
echo "</SELECT>\n";
?>
</TD>
</TR>
<TR>
<TD COLSPAN="2"><BR>
<?php
echo "<TABLE WIDTH=\"500\"><TR><TD COLSPAN=\"2\">\n";
echo "<B>Shipping Information:</B>\n";
echo "</TD></TR><TR><TD align=\"right\" valign=\"top\">\n";
echo "<B>Street:</B> <input type=\"text\" name=\"ship_add\" value=\"". $row['ship_add'] ."\" size=\"30\"><br />\n";
echo "<B>City:</B> <input type=\"text\" name=\"ship_city\" value=\"". $row['ship_city'] ."\" size=\"30\"><br />\n";
echo "</TD><TD align=\"right\" valign=\"top\">\n";
echo "<B>State:</B> <input type=\"text\" name=\"ship_state\" value=\"". $row['ship_state'] ."\" size=\"15\"><br />\n";
echo "<B>Zip Code:</B> <input type=\"text\" name=\"ship_zip\" value=\"". $row['ship_zip'] ."\" size=\"15\"><br /><br />\n";
echo "</TD></TR></TABLE>\n";
?>
</TD>
</TR>
<TR>
<TD COLSPAN="2">
<CENTER>
<TABLE WIDTH="500">
<TR>
<TD VALIGN="top">
<input type="submit" value="Save"></form></TD>
<TD VALIGN="top">
<form method="post" action=updateRegistry.php?regID=<?php echo $_GET['id']; ?>>
<input type="submit" value="Continue to Registry >>"></form></TD>
</TR>
</TABLE>
</CENTER>
</TD>
</TR>
</TABLE>
</BODY>
</HTML>
basically this is a page in a content management system that allows the admin to update a NewlyWed couple's information. the form is pre-populated with the values that are already stored in the database. when the admin edits the input fields, and clicks 'Save', the sql query is executed (after POSTed variables have been cleaned().
Posted: Thu Feb 15, 2007 1:37 pm
by Oren
Few things:
First, you don't need to make $db global since you don't even need to pass it to mysql_real_escape_string() - when the link identifier is not specified, the last link opened by mysql_connect() is used.
Second, you did this:
Code: Select all
foreach($_POST as $key => $val)
{
$val = clean($val);
}
This is probably what you really want instead:
Code: Select all
foreach($_POST as $key => $val)
{
$_POST[$key] = clean($val);
}
Now in general, after you established a successful connection to the database, you should be able to use mysql_real_escape_string() without a problem.
What happens if you just use mysql_real_escape_string() instead of clean() in the same script? Replace every call to clean() in your script with mysql_real_escape_string() and see what happens.
Posted: Thu Feb 15, 2007 1:48 pm
by RobertGonzalez
The clean function is not returning a value, so the value is locked in the function scope. Try returning it.
Code: Select all
<?php
function clean($var){
return mysql_real_escape_string(trim($var));
}
?>
Posted: Thu Feb 15, 2007 1:56 pm
by Oren
Everah wrote:The clean function is not returning a value, so the value is locked in the function scope. Try returning it.
Oh yeah... that too

I actually didn't read the code so there might be even more problems with it. It's your job to go through it and make sure everything is ok

Posted: Thu Feb 15, 2007 2:14 pm
by Oren
Oh and by the way,
array_walk() may be of interest.
Posted: Thu Feb 15, 2007 2:21 pm
by visitor-Q
ok, so a few things to note.
@ Oren, the foreach loop you provided, as far as i know, will do the same exact thing as mine. i simply prefer to distinguish the key and value of my arrays before i loop through them. it still applies the function for each index in the array whether i specify the key or not. also, when i took out the return statement, i stopped getting errors. thanks for the tip for array_walk! that seems like a much more efficient way of applying my function. it looks a little bit beyond my skills at the moment, but i'll definitely play around with it in this script.
@ everah, i originally had the function return a value, as shown in my first post, i simply wrote that one from scratch in my later posts. i have revised my script to include the return statement.
now, however, i have made some other revisions to my script, and i am getting an entirely different error that has nothing to do with my function (yet). i will start a new thread explaining this new error, and when i get that figured out, i'll post the status of my function in here, again. thanks for all your help so far!
p.s. everyone (including myself) completely overlooked my $sql string... it would have never worked... i was missing all the commas (,) after each set. hah

Posted: Thu Feb 15, 2007 2:30 pm
by Oren
visitor-Q wrote:the foreach loop you provided, as far as i know, will do the same exact thing as mine.
That's not true. Your loop will simply do... nothing. Check it out
