Page 1 of 1

Is this code safe?

Posted: Fri Feb 29, 2008 12:10 pm
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

Re: Is this code safe?

Posted: Fri Feb 29, 2008 4:34 pm
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.

Re: Is this code safe?

Posted: Fri Feb 29, 2008 6:23 pm
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?

Re: Is this code safe?

Posted: Fri Feb 29, 2008 6:57 pm
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.

Re: Is this code safe?

Posted: Fri Feb 29, 2008 7:50 pm
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
}
 

Re: Is this code safe?

Posted: Fri Feb 29, 2008 8:48 pm
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?

Re: Is this code safe?

Posted: Fri Feb 29, 2008 8:53 pm
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';
 
} 
  
?>

Re: Is this code safe?

Posted: Fri Feb 29, 2008 11:24 pm
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.

Re: Is this code safe?

Posted: Mon Mar 03, 2008 10:12 am
by frozen
Thanks alot for the encouragement astions.

Re: Is this code safe?

Posted: Mon Mar 03, 2008 12:04 pm
by Mordred
frozen wrote:

Code: Select all

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

Code: Select all

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

Re: Is this code safe?

Posted: Tue Mar 04, 2008 4:35 am
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?

Re: Is this code safe?

Posted: Tue Mar 04, 2008 11:25 am
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.

Re: Is this code safe?

Posted: Tue Mar 25, 2008 2:15 pm
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