Page 1 of 1

Very Simple .. Please Check This Code

Posted: Sat Jan 13, 2007 11:42 pm
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);

?>

Posted: Sun Jan 14, 2007 12:03 am
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.

Posted: Sun Jan 14, 2007 12:26 am
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

Posted: Sun Jan 14, 2007 1:16 am
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.

Posted: Sun Jan 14, 2007 8:53 am
by SGMH
Thank You Aloooot I Will Work On Improving It

:)

Posted: Sun Jan 14, 2007 9:09 am
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.

Posted: Sun Jan 14, 2007 9:23 am
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.

Posted: Sun Jan 14, 2007 10:32 am
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.