Good Protection?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
SidewinderX
Forum Contributor
Posts: 407
Joined: Fri Jul 16, 2004 9:04 pm
Location: NY

Good Protection?

Post 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)."'");
}
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post 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'];
SidewinderX
Forum Contributor
Posts: 407
Joined: Fri Jul 16, 2004 9:04 pm
Location: NY

Post 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?"
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post 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
}
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post 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.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post 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>";
}
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post 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.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post 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.
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post 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?
Z3RO21
Forum Contributor
Posts: 130
Joined: Thu Aug 17, 2006 8:59 am

Post 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.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post 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.
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post 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.
Post Reply