Very Simple .. Please Check This Code

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
SGMH
Forum Newbie
Posts: 7
Joined: Sat Nov 04, 2006 9:23 pm

Very Simple .. Please Check This Code

Post by SGMH »

This code should take a Post from the user and delete a file after doing some checking

the process

Post Delete Code By User > Check > Search the DB for the File Name Related To This Delete Code > Check > Delete From The DataBase And Server


can you please take a look at it and tell me if there is anything need to be fixed

Code: Select all

<?php

//-------------------------------------- check if the post ( the delete code ) is a number
if ( is_numeric($_POST[dnumber]) )
   {
   	$dnumber = $_POST[dnumber];
   } else {
	die('');
   }
//----------------------------------------------------------------

 // --------------------- connect to the database where we will search for the file that is related to the delete code
$con = mysql_connect("localhost","*****","*****");
if (!$con)
  {
  die('Could not connect: ' . mysql_error());
  }
mysql_select_db("vbpron_Upload", $con);

$query="SELECT COUNT(*) FROM files Where dnumber = '$_POST[dnumber]'";
$result_of_count = mysql_query($query,$con);
$num_of_data = mysql_result($result_of_count,0,0);


if ($num_of_data == 0) {
echo $num_of_data;
exit();
}
else {

$result = mysql_query("SELECT * FROM files Where dnumber = '$_POST[dnumber]'");
$filename = mysql_result($result, 0); 


//--------------------------------------  Check if the 1st 32 strings of the file that is related to the delete code are numbers ( all the files are named as a 32 numberd then the extension ( ex   48682042777251313657382990437535.jpg   )

if ( is_numeric(substr($filename,0,32)) )
   {
   } else {
exit();
   }


$sqll="DELETE FROM files WHERE dnumber = '$_POST[dnumber]'";
mysql_query($sqll,$con);

unlink("files/".$filename);

echo 'Done';

}

mysql_close($con);

?>
User avatar
Burrito
Spockulator
Posts: 4715
Joined: Wed Feb 04, 2004 8:15 pm
Location: Eden, Utah

Post by Burrito »

why don't you tell us what's NOT working...with as much detail as possible.

that will better help us help you.
SGMH
Forum Newbie
Posts: 7
Joined: Sat Nov 04, 2006 9:23 pm

Post by SGMH »

Everything is working good but being a beginner in php i don't feel safe using Unlink or SQl Delete

i tried to read about the methods used to hack php and to learn how to protect my code but i am still a newbie and i know that i may miss major security stuff

So I thought you people could help me checking the code in general, it is not long, it shouldn’t take a min or two going throw it
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Use $dnumber instead of $_POST[dnumber] -- not a security thread, but a good style. Otherwise it's okay (security-wise), assuming that the uploading script is also well-secured. There are lots of things to be improved, but they don't relate to the security of the thing.
SGMH
Forum Newbie
Posts: 7
Joined: Sat Nov 04, 2006 9:23 pm

Post by SGMH »

Thank You Aloooot I Will Work On Improving It

:)
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Post by Kieran Huggins »

Mordred wrote:$dnumber instead of $_POST[dnumber]
That would mean that register globals is turned on, which is a bad idea. Also, it's not very common these days.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Kieran Huggins wrote:
Mordred wrote:$dnumber instead of $_POST[dnumber]
That would mean that register globals is turned on, which is a bad idea. Also, it's not very common these days.
Psst, the code assigns $dnumber.

Unfortunately, it doesn't have quotes around the named array indices. While it does pass it through is_numeric(), it doesn't ensure the data is the same type the field is built to accept. For example it could be scientific E based notation.

Other than those two things, it appears relatively safe.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

Code: Select all

if (!$con)
  {
  die('Could not connect: ' . mysql_error());
  }
if your connection fails you will immediately tell people that you are using MySQL which isn't desirable. A good alternative is to use trigger_error() for these things and die afterwards. The reason being you can disable whether errors are displayed or not

Code: Select all

ini_set('display_errors', false);
and of course in php.ini or even .htaccess

Code: Select all

php_flag display_errors off
The 'could not connect' part is fine actually but its best to avoid sending information from mysql_error() to the world -- obviously whilst your developing it is important to have such information which is why the ability to turn it on and off with trigger_error() is useful.

Also you are trying to unlink using a resource not a string. See the return value of mysql_query(). You should also filter the variable before injecting it into a path such as when using unlink. If you can also use the php ini value open_basedir to restrict PHP's filesystem interactions to a certain area of the filesystem.
Post Reply