Page 1 of 1

Good Protection?

Posted: Sat Aug 04, 2007 1:11 pm
by SidewinderX
Well I'm about to make my script public and I'm just spending some time commenting the code and beefing up the security. I was wondering if this is good enough protection to prevent SQL Injections?

$cid is suppose to be a number

Code: Select all

$cid = $_GET['cid'];
$cid = addslashes($cid);
$cid = htmlentities($cid);
if(!is_numeric($cid)) {
	echo "<center><b>$cid is not numeric and is not a valid.</b></center>";
} 

else {
	$sql = mysql_query("SELECT * FROM `".$db_prefix."tl_comments` WHERE `cid`='".mysql_escape_string($cid)."'");
}

Posted: Sat Aug 04, 2007 1:47 pm
by matthijs
Addslashes is not needed when you use mysql_real_escape_string(), it will only lead to too many slashes which you have to remove again.
Htmlentities is a function used to escape data for output to HTML, so shouldn't be used before input in a db. Look the functions up in the manual for more explanations.
You might also look up is_numeric, is_int, and related functions and study the (sometimes subtle) differences.

Also, you can cast the input to a numeric type by using
$validinput = (int) $_GET['cid'];

Posted: Fri Aug 10, 2007 11:06 pm
by SidewinderX
So you suggest not using stripslashes and htmlentities before entering data into a mysql database. Assuming I'm expecting a string to be inputed - that leaves mysql_real_escape_string as the only "input" protection, which allows for XSS.

Is it wise to use some other kind of "input" protection, or use striptags in the "output?"

Posted: Fri Aug 10, 2007 11:56 pm
by s.dot
Something along the lines of this would be secure..

Code: Select all

if (!empty($_GET['cid']) {
   $cid = $_GET['cid'];

   if (get_magic_quotes_gpc()) {
      $cid = stripslashes($cid);
   }

   if(!is_numeric($cid))
   {
      //error
   }

   $cid = mysql_real_escape_string(htmlentities($cid, ENT_QUOTES));
} else
{
   //error
}

Posted: Sat Aug 11, 2007 12:06 am
by John Cartwright
SidewinderX wrote:So you suggest not using stripslashes and htmlentities before entering data into a mysql database.
I would recommend stripslashes()'ing when inputting into the database only because typically this is an unwanted affect from magic quotes. Different environments obviously have different php settings so it should generally be common practce to apply stripslashes()
Assuming I'm expecting a string to be inputed - that leaves mysql_real_escape_string as the only "input" protection, which allows for XSS.
When inputting into your database we do not care about XSS, we care about SQL injection. The goal of mysql_real_escape_string() it to prevent that kind of attack (as well as legitamate strings that may contain characters that will affect our query unintentionally). On the other hand, if you are expecting a numerical input, you can replace mysql_real_escape_string() with typcasting (int) or intval()
Is it wise to use some other kind of "input" protection, or use striptags in the "output?"
It is my preference to stripslashes() on input to have to avoid a seperate step to acheiving clean output. Although, htmlentities should almost always be applied to any output, to prevent XSS as well as generating valid markup.

Posted: Sat Aug 11, 2007 12:34 am
by Benjamin
This would be just as secure..

Code: Select all

<?php
$cid = trim($_GET['cid']);

if (preg_match('#^\d{1,}$#', $cid))
{
    $sql = mysql_query("SELECT * FROM {$db_prefix}tl_comments WHERE cid = $cid");
} else {
    echo "<center><b>$cid is not numeric and is not a valid.</b></center>";
}

Posted: Sat Aug 11, 2007 9:13 am
by superdezign
I would still say that is_numeric() is a better function choice, though, but seeing as you use the {1,} quantifier, I'm sure you'd agree.
Also, I assumed he used quotation marks around the $cid because maybe the id was a numeric string rather than a number.

Posted: Sat Aug 11, 2007 10:15 am
by Benjamin
is_numeric would be fine if it's ok for the data to be negative, decimal, hexadecimal or exponential.

I prefer regex as I have had issues with the cytpe family of functions telling me that posted digits are not numbers.

Posted: Sat Aug 11, 2007 10:31 am
by superdezign
astions wrote:is_numeric would be fine if it's ok for the data to be negative, decimal, hexadecimal or exponential.
Whoa.. I was definitely thinking is_numeric() was more along the lines of ctype_digit().
astions wrote:I prefer regex as I have had issues with the cytpe family of functions telling me that posted digits are not numbers.
Really? That's odd. Was it like, URL-encoded or something?

Posted: Sat Aug 11, 2007 10:43 am
by Z3RO21
astions wrote:is_numeric would be fine if it's ok for the data to be negative, decimal, hexadecimal or exponential.

I prefer regex as I have had issues with the cytpe family of functions telling me that posted digits are not numbers.
I also prefer regex because it gives you more control over the input validation. Regex is essential in my opinion to data validation.

Posted: Sat Aug 11, 2007 10:54 am
by Benjamin
superdezign wrote:Really? That's odd. Was it like, URL-encoded or something?
It was the number 8 posted from a dropdown menu with no whitespace before or after it.

Posted: Sat Aug 11, 2007 11:16 am
by superdezign
astions wrote:It was the number 8 posted from a dropdown menu with no whitespace before or after it.
Oh, the famous PHP 5.1.1 glitch with the number 8 and ctype_digit() where it confuses the number 8 with the bbCode symbol for glasses... Gets me every time. ;)

I'm halfway sure there was something affecting it, but haven't the slightest clue what. However, if regex cured it, then I'm baffled.