Please critique my upload script.

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Please critique my upload script.

Post 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.
Last edited by Benjamin on Tue Jun 23, 2009 10:11 am, edited 1 time in total.
Reason: Added [code=php] tags.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
waseem83
Forum Commoner
Posts: 54
Joined: Tue Jun 23, 2009 3:51 am
Contact:

Re: Please critique my upload script.

Post by waseem83 »

Code seems to be right.
But infront of $target you should only write folder name.
waseem83
Forum Commoner
Posts: 54
Joined: Tue Jun 23, 2009 3:51 am
Contact:

Re: Please critique my upload script.

Post 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();}
    
}
Last edited by Benjamin on Tue Jun 23, 2009 10:12 am, edited 1 time in total.
Reason: Added [code=php] tags.
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Please critique my upload script.

Post 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 ...
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
waseem83
Forum Commoner
Posts: 54
Joined: Tue Jun 23, 2009 3:51 am
Contact:

Re: Please critique my upload script.

Post by waseem83 »

Check the code i sent u above ,its 100% correct and tested......
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Please critique my upload script.

Post 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.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
waseem83
Forum Commoner
Posts: 54
Joined: Tue Jun 23, 2009 3:51 am
Contact:

Re: Please critique my upload script.

Post by waseem83 »

Have you tested my code?
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Please critique my upload script.

Post 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'] ?>" >
;)
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Please critique my upload script.

Post by social_experiment »

Thanks for the critique VladSun, how can i improve the script to avoid it leading to RCE?
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Please critique my upload script.

Post by VladSun »

There are 10 types of people in this world, those who understand binary and those who don't
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Please critique my upload script.

Post 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.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Please critique my upload script.

Post 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.
Last edited by Benjamin on Wed Jun 24, 2009 1:15 pm, edited 1 time in total.
Reason: Added [code=php] tags.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
Darhazer
DevNet Resident
Posts: 1011
Joined: Thu May 14, 2009 3:00 pm
Location: HellCity, Bulgaria

Re: Please critique my upload script.

Post 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 :)
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: Please critique my upload script.

Post 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()
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Please critique my upload script.

Post 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.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
Post Reply