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.
<?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);
?>
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
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.
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.
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
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.