0

I've read the Secure PHP Upload Scripts thread but I'm having difficulty getting this known good script to accept changes. I want this script to only allow .jpeg, .png, and .gif files. Could someone advise me on how to modify this script to do so?

<?php
$result=0;
if (trim($_POST["action"]) == "Upload File") { //**** User Clicked the Upload File Button

   //*********** Execute the Following Code to Upload File *************
   $imagename = basename($_FILES['image_file']['name']);  // grab name of file 
   $result = @move_uploaded_file($_FILES['image_file']['tmp_name'], $imagename); // upload it 
   if ($result==1) echo("Successfully uploaded: <b>".$imagename."</b>"); // did it work?

} // end if
?>
<?php
if ($result==1) echo("<img src='".$imagename."'>"); // display the uploaded file
?>
Alexk
  • 31
  • 4
  • You could start with something like in [this link](http://webcheatsheet.com/PHP/file_upload.php), modify it to your needs, and if doesn't work come on back and we can help from there. – Unexpected Pair of Colons Apr 26 '13 at 21:44
  • you definitely need to start testing for succesful uploads, doing server-side mime-type checks, and absolutely **MUST** stop using the `@` operator. – Marc B Apr 26 '13 at 21:45
  • You've got HTML injection (leading to XSS), in addition to the inadvisability of using user-supplied filenames, and serving user-supplied files from the same domain as the app. – bobince Apr 28 '13 at 01:39

3 Answers3

1
$filename = $_FILES['image_file']['name'];
$ext = pathinfo($filename, PATHINFO_EXTENSION);
if($ext !== 'jpg' && $ext !== 'png' && $ext !== 'gif') {echo 'error';}

is a very bad idea for validation. 

echo '<pre>';
$filename = 'image.php\0.jpg';

$extension = pathinfo($filename, PATHINFO_EXTENSION);
var_dump($ext);

The var_dump displays jpg

And the php function move_uploaded_file is vulnerable with null bytes \0. After the move_uploaded_file the server will create a image.php file..

Raymond Nijland
  • 56
  • 1
1

If you want to stop the upload before it reaches your server, you can filter it with javascript. See this SO answer for more information: stackoverflow.com/questions/71944/… – Kevin Apr 26 at 22:13

Never never never never neverever put trust in client side validation...

Coding a safe upload is hard. Very hard.

You can't trust file extensions or mime type because clients can change this.

If you only want an upload for gif, jpeg or png you could take these steps. With png you can have trouble because of the encoding that can bypass some of these.

  1. Read the temp file by file_get_contents().
  2. Run strip_tags() on it.
  3. Create new images with the GD library
  4. Serve the image by read() - Don't use include() or require()
  5. Disable php engine on that directory
Community
  • 1
  • 1
Raymond Nijland
  • 56
  • 1
0

For the sake of brevity, i'm not doing any error checking.. but you can evaluate the extension of a file like this:

$filename = $_FILES['image_file']['name'];
$ext = pathinfo($filename, PATHINFO_EXTENSION);
if($ext !== 'jpg' && $ext !== 'png' && $ext !== 'gif') {echo 'error';}
Kevin
  • 523
  • 4
  • 20
  • It looks like that successfully identifies the files but it doesn't prevent files from being uploaded. Is there a way to kill the upload upon identifying a non (png, jpeg, zip) file? – Alexk Apr 26 '13 at 22:06
  • Once you are at the point of this PHP script the files are already on your server. All you can do is decide whether or not you want to run some processes on them. In this case, maybe delete the file and kill the script with die(); – Kevin Apr 26 '13 at 22:10
  • If you want to stop the upload before it reaches your server, you can filter it with javascript. See this SO answer for more information: http://stackoverflow.com/questions/71944/how-do-i-validate-the-file-type-of-a-file-upload – Kevin Apr 26 '13 at 22:13
  • Could I do something like this and delete it once it's on the server? if($ext !== 'jpg' && $ext !== 'png' && $ext !== 'gif') {unlink($filename);} – Alexk Apr 26 '13 at 22:29
  • That would work, but you need to get the file path not just the file name. For additional security, I recommend checking on the client side as well. – Kevin Apr 26 '13 at 22:35