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.