Page 1 of 2
Please critique my upload script.
Posted: Tue Jun 23, 2009 6:11 am
by social_experiment
I created this simple script for file uploading. It only allows upload of image files ( more specifically .jpg ) and these files have to be =< to 100kb.
Here is the HTML code that creates the form
Code: Select all
<form enctype="multipart/form-data" method="post" action="<?php $_SERVER['PHP_SELF'] ?>" >
<input type="hidden" name="MAX_FILE_SIZE" value="102400" />
<input type="file" name="userimage" />
<input type="submit" value="Submit" />
</form>
For testing purposes it uses itself to process the php.
Below is the PHP code.
Code: Select all
<?php
if ( (($_FILES['userimage']['type'] == 'image/jpg') || ($_FILES['userimage']['type'] == 'image/pjpeg') || ($_FILES['userimage']['type'] == 'image/jpeg')) ) {
# if upload file type matches one of the following criteria, continue
# creating a filename for move_uploaded_file.
$target = "nTest/".$_FILES['userimage']['name']."";
# creating a destination for move_uploaded_file.
$source = $_FILES['userimage']['tmp_name'];
move_uploaded_file( $source, $target );
# confirmation message if the file is uploaded.
echo "<p>".$_FILES['userimage']['name']." has been uploaded </p>";
}
# if the file is not a .jpg / jpeg or exceeds the filesize limit set in the hidden file ( 100kb )
else {
echo '<p>Your file has not been uploaded</p>';
}
?>
I tested it a few times and everything thing seems to be working fine. I would like to know if it can be improved upon and if so, where.
Thanks.
Re: Please critique my upload script.
Posted: Tue Jun 23, 2009 6:40 am
by waseem83
Code seems to be right.
But infront of $target you should only write folder name.
Re: Please critique my upload script.
Posted: Tue Jun 23, 2009 6:44 am
by waseem83
Here is code to upload file its tested and 100% correct.
Code: Select all
//to upload file
if(isset($_FILES['flLangFlag']['name']) && !empty($_FILES['flLangFlag']['name']))
{
$fileName=$_FILES['flLangFlag']['name'];
$strRealPathFile = "Adminlogin";
$strDestFolder = "UploadedFiles";
$strImagePrefix =str_replace(" ","",microtime());
$fileName=$strImagePrefix."_".$fileName;
$strValidExtension = "gif,jpg,jpeg,png,bmp,psd,tiff,doc";
$strControlName = "flLangFlag";
$intMaxSize=104857610485761048576;
$strUploaded = UploadFile($strRealPathFile, $strDestFolder, $strImagePrefix, $strValidExtension, $strControlName, $intMaxSize);
if(strpos($strUploaded,"_")>-1)
{
$strLangFlag = $strUploaded;
}
else
{
$strLangFlag = "";
}
}
Code: Select all
////function to upload
function UploadFile($strRealPathFile, $strDestFolder, $strImagePrefix, $strValidExtension, $strControlName, $intMaxSize=104857610485761048576, $intMaxWidth=0, $intMaxHeight=0)
{
$strInvalidMsg = "";
$mypath = realpath ($strRealPathFile);
$strUploadDir = $strDestFolder;//echo "|||".$strUploadDir;exit;
if (strpos($mypath,"\\") > -1 )
$strFinalPath = substr($mypath,0,strrpos($mypath,"\\")+1).$strUploadDir."\\";
else
$strFinalPath = substr($mypath,0,strrpos($mypath,"/")+1).$strUploadDir."/";
$intImageSize = $_FILES["".$strControlName.""]['size'];
$strImageName = $_FILES["".$strControlName.""]['name'];
$strImageName = str_replace(" ","-",$strImageName);
$strImageName = str_replace("+","",$strImageName);
$strImageName = str_replace("&","-",$strImageName);
$strTempPath = $_FILES["".$strControlName.""]['tmp_name'];
$strImageExt = strtolower(getFileExt($strImageName));
if($intMaxWidth != 0 && $intMaxHeight !=0)
{
list($intWidth, $intHeight, $strType, $strAttr) = getimagesize($strTempPath);
if($intWidth > $intMaxWidth) {$strInvalidMsg="InvalidWidth"; return $strInvalidMsg; exit();}
else if($intHeight > $intMaxHeight) {$strInvalidMsg="InvalidHeight"; return $strInvalidMsg; exit();}
}
if($intImageSize > $intMaxSize) {$strInvalidMsg="InvalidSize"; return $strInvalidMsg; exit();}
if(strpos($strValidExtension,",")>0)
{
$intExtCheck = 0;
$strExtArr = explode(",",$strValidExtension);
for($counter=0;$counter < count($strExtArr);$counter++)
{
if($strExtArr[$counter] == $strImageExt)
{
$intExtCheck = 1;
break;
}
}
if($intExtCheck == 0) {$strInvalidMsg="InvalidExtension"; return $strInvalidMsg; exit();}
}
else if($strImageExt != $strValidExtension)
{$strInvalidMsg="InvalidExtension"; return $strInvalidMsg; exit();}
$strImageCompName = $strImagePrefix."_".$strImageName;
$strDestination = $strFinalPath.$strImageCompName;
$intIsUploaded = move_uploaded_file($strTempPath,$strDestination);
if($intIsUploaded) {$strInvalidMsg=$strImageCompName; return $strInvalidMsg; exit();}
else {$strInvalidMsg="UploadErr"; return $strInvalidMsg; exit();}
}
Re: Please critique my upload script.
Posted: Tue Jun 23, 2009 7:29 am
by social_experiment
Thanks for the critique,
If you place only the name of the directory in $target ( $target = "nTest"; ), you get the following errors:
Warning: move_uploaded_file(nTest) [function.move-uploaded-file]: failed to open stream: Permission denied in ...
Warning: move_uploaded_file() [function.move-uploaded-file]: Unable to move ...
Re: Please critique my upload script.
Posted: Tue Jun 23, 2009 7:37 am
by waseem83
Check the code i sent u above ,its 100% correct and tested......
Re: Please critique my upload script.
Posted: Tue Jun 23, 2009 10:20 am
by social_experiment
Im sure the code is 100%, i actually referenced my script with those two errors after testing your suggestion of placing only the destination directory in the $target, without the filename.
Again, thank you for the critique.
Re: Please critique my upload script.
Posted: Wed Jun 24, 2009 1:49 am
by waseem83
Have you tested my code?
Re: Please critique my upload script.
Posted: Wed Jun 24, 2009 2:17 am
by VladSun
You have several security issues in your script - one of them is critical and leads to RCE.
This :
Code: Select all
if ( (($_FILES['userimage']['type'] == 'image/jpg') || ($_FILES['userimage']['type'] == 'image/pjpeg') || ($_FILES['userimage']['type'] == 'image/jpeg')) ) {
is trivial to bypass - anyone can fake the HTTP headers and upload a PHP file.
Code: Select all
$target = "nTest/".$_FILES['userimage']['name']."";
Due to security reasons, you should not use the original file name. It's better to rewrite the name of the file plus parsing and adding a proper file extension.
Code: Select all
echo "<p>".$_FILES['userimage']['name']." has been uploaded </p>";
Never echo user generated input.
Code: Select all
<form enctype="multipart/form-data" method="post" action="<?php $_SERVER['PHP_SELF'] ?>" >
This will lead to XSS - try opening:
http://yoursite.com/upload.php/<script> ... ')</script>
PS:
Code: Select all
<form enctype="multipart/form-data" method="post" action="<?php $_SERVER['PHP_SELF'] ?>" >
should be
Code: Select all
<form enctype="multipart/form-data" method="post" action="<?php echo $_SERVER['PHP_SELF'] ?>" >

Re: Please critique my upload script.
Posted: Wed Jun 24, 2009 2:55 am
by social_experiment
Thanks for the critique VladSun, how can i improve the script to avoid it leading to RCE?
Re: Please critique my upload script.
Posted: Wed Jun 24, 2009 4:03 am
by VladSun
Re: Please critique my upload script.
Posted: Wed Jun 24, 2009 4:25 am
by social_experiment
Thank you, ill take a look at it.
waseem83, i looked at your code. I used some of the ideas in there to create a new filename and the extension i want.
Once i have a better script ill paste it for more critique.
Re: Please critique my upload script.
Posted: Wed Jun 24, 2009 4:52 am
by social_experiment
Here is the code i created this morning
Code: Select all
<?php
if ( isset($_FILES['userimage']) ) {
# create random number to be used as a filename
$randomNumber = '123456';
$number = mt_rand(10000, $randomNumber);
# create the filename that will be given to the uploaded file
$sub_fileName = $number;
$validExtension = '.jpg';
$new_filename = $sub_fileName.$validExtension;
# create the variables for move_uploaded_file function
$source = $_FILES['userimage']['tmp_name'];
$target = "nTest/$new_filename";
$move = move_uploaded_file( $source, $target );
}
?>
It changes the name of the file then adds the extension ( $validExtension ) so that no matter what file i try to upload ( still within the 100kb size range ) gets a new name and the defined extension.
Thank you for the url VladSun, im going to read it again ( thoroughly ) to get a better grasp on file uploading scripts and how to make them as secure as i can.
I still want critique on the code, there is always room for improvement imho.
Re: Please critique my upload script.
Posted: Mon Jul 13, 2009 3:51 pm
by Darhazer
If you want to preserve the original file name, use:
Code: Select all
$name = basename($_FILES[<some-identifier>]['name']);
Always check the value of $_FILES[<some-identifier>]['error'] - it should be UPLOAD_ERR_OK
Sorry if those are included in the PDF, I can't open the PDF here

Re: Please critique my upload script.
Posted: Sat Aug 08, 2009 1:08 pm
by josh
VladSun wrote:Code: Select all
<form enctype="multipart/form-data" method="post" action="<?php $_SERVER['PHP_SELF'] ?>" >
This will lead to XSS - try opening:
http://yoursite.com/upload.php/<script> ... ')</script>
PS:
Code: Select all
<form enctype="multipart/form-data" method="post" action="<?php $_SERVER['PHP_SELF'] ?>" >
should be
Code: Select all
<form enctype="multipart/form-data" method="post" action="<?php echo $_SERVER['PHP_SELF'] ?>" >

htmlentities()
Re: Please critique my upload script.
Posted: Sun Aug 09, 2009 7:58 am
by social_experiment
This is the latest upload script i created
Code: Select all
<html>
<!-- create the upload form -->
<form method="post" action="<?php echo $_SERVER['PHP_SELF'] ?>" enctype="multipart/form-data" >
<input type="hidden" name="MAX_FILE_SIZE" value="102400" />
<input type="file" name="fileUpload" />
<input type="submit" value="upload file" />
</form>
</html>
and the php code
Code: Select all
<?php
if ( $_FILES['fileUpload']['name'] == '' ) {
echo 'no file selected for upload';
}
//
else {
// create a new, random file name
$number = '1234567890';
$random = mt_rand(10000, $number);
// the extension
$validExtension = '.jpg';
// create the new file name consisting of the random new name and the
// valid extension
$newFile = $random.$validExtension;
// target for move_uploaded_file()
$target = "uploaded/$newFile";
// source for move_uploaded_file()
$source = $_FILES['fileUpload']['tmp_name'];
// assign move_uploaded_file to a variable
$move = move_uploaded_file($source, $target);
// if the file has not been uploaded
if ( !$move ) {
// get the error [ if any ], if the error is != to 0 it means the
// file has not been upload, issue confirmation message
$error = $_FILES['fileUpload']['error'];
if ( $error != 0 ) { echo 'File has not been uploaded'; }
}
// if the file has been uploaded, issue conformation message
else { echo 'Your file has been successfully uploaded.'; }
}
?>
I tried using basename() as follows :
Code: Select all
<?php
$source = basename($_FILES['fileUpload']['tmp_name']);
?>
but then it doesn't upload the file. Once i rm'ed it the script worked fine.