Is this code safe?

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
frozen
Forum Newbie
Posts: 8
Joined: Fri Feb 22, 2008 11:43 pm

Is this code safe?

Post by frozen »

Hi guys.

I have written a page to get id from the URL. As im really new to php and worried that I haven't got the security code written right.

Could you have a look at my code and let me know if I haven't written it right and if it's not secure.

Here's my code

Code: Select all

<?php
 
$dbhost = '';
$dbuser = '';
$dbpass = '';            
 
// Get id
if(isset($_GET['pid'])) {
$id = ($_GET['pid']);
 
// Remove unwanted characters
$tokill = array (
                '/([!+ =-?-_;.,$^*])/',
                '/([a-zA-Z])/'      
                );
          
           
$fix = '';   
       
$id = preg_replace($tokill, $fix, $id);
 
 
function check_input($id)
{
 
 
// strip slashes
if (get_magic_quotes_gpc())
  {
  $id = stripslashes($id);
  }
 
// Quote non numeric characters
if (!is_numeric($id))
  {
  $id = "'" . mysql_real_escape_string($id) . "'";
  }
 
return $id;
}
 
// Connect to database
$conn = mysql_connect($dbhost, $dbuser, $dbpass) or die
                     ('Error connecting to mysql');
 
$dbname = 'storedb';
mysql_select_db($dbname);                    
                     
// My query
     $query = "SELECT *FROM products WHERE ID= '$id'";
     $result = mysql_query($query)
                or die('Error : ' . mysql_error()); 
     $nrow = mysql_num_rows($result);
 
// If the row don't exist show 404
if ($nrow < 1)
{
    include '404.php'; }
    {
 
// Write results
while($row = mysql_fetch_array($result)) {
 
    echo "{$row['product_title']} <br>" .
         "{$row['product_image']} <br><br>";
        
    
}
}
}
 
// If no ID show 404
else {
include '404.php';
}
 
?>
Thanks

Bradley
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Is this code safe?

Post by Christopher »

Secure maybe, written right ... hmmmm ...

I am guessing that your $pid is an integer. if that is the case then you could more easily do:

Code: Select all

$pid = intval($_GET['pid']);
// or 
$pid = preg_replace('/[^0-9]/', '', $_GET['pid']);
I would also recommend rearranging things a little to promote Defense in Depth. For example, remove slashes from the entire Request (if magic_quotes is set) early in the script to make sure nothing slips through. Likewise, I would do the database escaping right before the query code so you can be sure it is getting done. If code is scattered, it is possible to make unintended changes later.
(#10850)
frozen
Forum Newbie
Posts: 8
Joined: Fri Feb 22, 2008 11:43 pm

Re: Is this code safe?

Post by frozen »

Ok thanks alot, I'll have a mess around with it and see if I can make it better.

What does the ^ do in the preg_replace? Does it mean anything that isn't 0-9?
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Is this code safe?

Post by Christopher »

frozen wrote:Ok thanks alot, I'll have a mess around with it and see if I can make it better.
Great, show us what you come up with.
frozen wrote:What does the ^ do in the preg_replace? Does it mean anything that isn't 0-9?
Yep. That's a pretty standard filtering technique in PHP.
(#10850)
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Re: Is this code safe?

Post by Benjamin »

I have to disagree with using intval because I am in the mindset that a variable shouldn't be forced to be something it isn't. I believe this mindset could potentially prevent unintended side effects. I would encourage the use of preg_match in combination with trim to validate that it is an integer.

With the example below you can also check for min and max len so it's win win.

Code: Select all

 
function post($x)
{
    return isset($_POST[$x]) ? trim($_POST[$x]) : null;
}
 
$id = post('id');
 
if (!preg_match('#^[0-9]{1,10}$#', $id))
{
    # id isn't valid
}
 
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Is this code safe?

Post by Christopher »

I don't use intval(), but I don't see the potential unintended side effects. Are there any real differences other than the amount of code required?
(#10850)
frozen
Forum Newbie
Posts: 8
Joined: Fri Feb 22, 2008 11:43 pm

Re: Is this code safe?

Post by frozen »

Thanks Astions, i'll have a read up on trim and preg_match. I assume that the {1,10} part of it is the min and max length settings yeah? What does the two # and $ sign do in the preg_match?


This is what I came up with

(Had to edit because I missed out part of the mysql_real_escape. I did not work when I tested it without the filtering before it. So I fixed it and it now works)

Code: Select all

<?php
// Get id
if (isset($_GET['pid'])) {
$pid = ($_GET['pid']);
 
 
// strip slashes
if (!get_magic_quotes_gpc()) {
$pid = stripslashes($pid);
}
 
 
// Remove all non numeric characters
$pid = preg_replace('/[^0-9]/', '', $_GET['pid']);
 
 
// Database connection
include 'db.php';
 
 
// Quote non numeric characters
function check_input($pid) {
if (!is_numeric($pid)) {
$pid = "'" . mysql_real_escape_string($pid) . "'";
}
return $pid;
}
 
 
// My query
     $query = "SELECT *FROM products WHERE ID= '$pid'";
     $result = mysql_query($query)
                or die('Error : ' . mysql_error()); 
     $nrow = mysql_num_rows($result);
 
 
// If the row don't exist show 404
if ($nrow < 1)
{
    include '404.php'; }
    {
 
 
// Write results
while($row = mysql_fetch_array($result)) {
 
    echo "{$row['product_title']} <br>" .
         "{$row['product_image']} <br><br>";
}
}
}
 
// If no ID show 404
else {
include '404.php';
 
} 
  
?>
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Re: Is this code safe?

Post by Benjamin »

@frozen: Looks good, you're getting the hang of it.

@arborint: Yes, I think there is a fundamental difference between modifying something to make it fit and verifying that it will fit. I'm sure there are a lot of for and against arguments for either, and a lot of hypothetical situations as well. At the end of the day though it just makes more sense to throw an error if the data is of the wrong type. It would certainly indicate a higher level problem, to me at least.
frozen
Forum Newbie
Posts: 8
Joined: Fri Feb 22, 2008 11:43 pm

Re: Is this code safe?

Post by frozen »

Thanks alot for the encouragement astions.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Is this code safe?

Post by Mordred »

frozen wrote:

Code: Select all

$pid = preg_replace('/[^0-9]/', '', $_GET['pid']);

Code: Select all

$pid = preg_replace('/[^0-9]/', '', $pid);
User avatar
stakes
Forum Commoner
Posts: 48
Joined: Tue Jun 12, 2007 12:05 pm

Re: Is this code safe?

Post by stakes »

From a "lazy safe" perspective. Wouldn't just escaping the input be enough? I mean if somone enters a non int to compare to the ID(int) column mysql would just return 0 rows?
jmut
Forum Regular
Posts: 945
Joined: Tue Jul 05, 2005 3:54 am
Location: Sofia, Bulgaria
Contact:

Re: Is this code safe?

Post by jmut »

astions wrote:...
@arborint: Yes, I think there is a fundamental difference between modifying something to make it fit and verifying that it will fit. I'm sure there are a lot of for and against arguments for either, and a lot of hypothetical situations as well. At the end of the day though it just makes more sense to throw an error if the data is of the wrong type. It would certainly indicate a higher level problem, to me at least.
For integers in particular it makes not difference whatsoever I think. You can easily pass any id anyhow....sow it wouldn't make a difference.
frozen
Forum Newbie
Posts: 8
Joined: Fri Feb 22, 2008 11:43 pm

Re: Is this code safe?

Post by frozen »

I'm going to implement paging and a where clause to pick products in a category.

I have been reading around about security for these two bits and I would just like to see if I'm on the right track.

For the paging I'm going to filter for only numbers. Use a calculation in the script, so that if someone did get anything else through. It should bring up an error. And am I right in thinking that I should quote the variable in the LIMIT of the SQL script but not use mysql real esscape strings?

For the categories I was thinking of doing an SQL query to get all the categories and automaticly create a variable with the name of the category and with the value of the ID number. And then with the GET I would filter for numbers and only take 3 digits. And if that number matches one of the variable values of the categories, then the script will then take that variable and use that in the where clause with real esscape strings.

How do they sound? I am right in thinking that I can automaticly make variables from an sql command and match it to a GET imput ain't I?

I'm also planning to have reviews for products once the site is up and running. I was thinking of just using a mySQL account that can only write to this table and not read or update etc. This is a good idea yes? I still need to do some research in how add security to this because I don't want to block everything that isn't a number or character, so if anyone has any links they can give me that can help me with this I would be grateful.

Thanks

Bradley
Post Reply