current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
1

Is this image uploading script safe? I have spent my day researching this and it would be very much appreciated if you could point out any errors or ways to make it safer.

<?php

$max_file_size = 1048576; // expressed in bytes
                          // 10240 = 10 KB
                          // 102400 = 100 KB
                          // 1048576 = 1 MB
                          // 10485760 = 10 MB

$upload_errors = array(
  // http://www.php.net/manual/en/features.file-upload.errors.php
  UPLOAD_ERR_OK         => "No Errors",
  UPLOAD_ERR_INI_SIZE   => "Larger than upload_max_filesize",
  UPLOAD_ERR_FORM_SIZE  => "Larger than form MAX_FILE_SIZE",
  UPLOAD_ERR_PARTIAL    => "Partial upload",
  UPLOAD_ERR_NO_FILE    => "No file",
  UPLOAD_ERR_NO_TMP_DIR => "No temporary directory",
  UPLOAD_ERR_CANT_WRITE => "Can't write to disk",
  UPLOAD_ERR_EXTENSION  => "File upload stopped by extension"
  );

$image_types = array (
  IMAGETYPE_GIF         => "gif",
  IMAGETYPE_JPEG        => "jpg",
  IMAGETYPE_PNG         => "png"
  );


$image_whitelist = array(IMAGETYPE_JPEG, IMAGETYPE_PNG, IMAGETYPE_GIF);

$message = "";
if (isset($_POST['submit'])) {

  // This would store the actual user id
  $userId = 1;

  // store uploaded file's temp path.
  $tmp_file = $_FILES['file_upload']['tmp_name'];

  if($_FILES['file_upload']['error'] > 0){
    $error = $_FILES['file_upload']['error'];
    die($upload_errors[$error]);
  }

  // check to see if file is an image, if not die.
  if(!getimagesize($tmp_file)) {
    die('Please ensure you are uploading an image.');
  }

  // check to see if image is a valid type, if not die.
  if (!in_array(exif_imagetype($tmp_file), $image_whitelist)) {
    die('Please upload a valid image. (.jpg, .png or .gif)');
  }

  $target_file = md5($userId . date('U')) . "." . $image_types[exif_imagetype($tmp_file)];
  $upload_dir = "uploaded";

  // check to see if file already exsits if it does die.
  if (file_exists($upload_dir."/".$target_file)) {
    die('file already exists');
  }

  //move_uploaded file will return false if $tmp_file is not a valid upload file
  // or if it cannot be moved for any other reason
  if (move_uploaded_file($tmp_file, $upload_dir."/".$target_file)) {
    $message = "File uploaded successfully.";
  } else {
    die("move_uploaded_file() error.");
  }
}
?>

<html>
<head>
  <title>File Upload</title>
</head>
<body>
  <?php if(!empty($message)) { echo "<p>{$message}</p>"; } ?>
  <form action="fileupload.php" enctype="multipart/form-data" method="POST">
    <input type="hidden" name="MAX_FILE_SIZE" value="<?php echo $max_file_size; ?>">
    <input type="file" name="file_upload">
    <input type="submit" name="submit" value="Upload">
  </form>
</body>
</html>

Many Thanks

share|improve this question
add comment

2 Answers

up vote 3 down vote accepted

It is pretty solid for saving images on a single web server. Some optional/extra things you could do are:

  • Host the images on a CDN. That way if there was any PHP or JS injected in, it would not be able to access any cookies or directories on the web server you use to serve content.

  • Your code would block out PHP files in disguise from being uploaded because of the getimagesize() and the exif_imagetype(). You can add this bit to an htaccess file in the images directory as well to disable PHP files from being executed on their own. This wouldn't stop the file from being executed though if your code automatically included the "image" in the code somewhere.

    <IfModule mod_php5.c>
    php_flag engine off
    </IfModule>

share|improve this answer
add comment
up vote 2 down vote

I think Levi covered your question fairly well, +1 to him. The following does not deal with security so much as good practice.

You might consider returning early. If you reverse your if logic you can remove the indent level of your whole script by 1. However, this wont work in your case as it appears that you have combined your form with your submission. It might be beneficial to abstract the two, but that is entirely dependent upon the application.

//returning early
if( ! isset( $_POST[ 'submit' ] ) ) {
    return;
}

//abstraction
if( isset( $_POST[ 'submit' ] ) ) {
    include '/path/to/fileupload_submit.php';
}

Comments should be used to explain complex code. When you use comments to explain everything then you are cluttering your workspace. Use self-documenting code to make your code easier to read, though that doesn't seem to be a problem here. Consider removing most of these comments for enhanced legibility. For instance, you don't need a commented list of bytes to KB and MB conversion. You can explain that you are limiting your uploads to 1MB, but the rest are pointless and easily looked up.

There are more elegant ways of terminating your code than to return, exit, or die. You took the trouble of converting the error definitions to readable text, you should present this on the page in a pleasant error box, thus presenting them with the opportunity to correct the issue. You could then even use the previous submission to repopulate the fields, assuming of course there were more than the one.

//die($upload_errors[$error]);
$message = 'There were errors with your submission';
$errors[] = $upload_errors[ $error ];

if( ! empty( $errors ) ) {
    //don't upload
}

You are not using the hidden input field "MAX_FILE_SIZE". Don't worry, this is a good thing. Relying on hidden input fields to restrict access is just asking for trouble. Hidden fields, despite the name, are still visible and modifiable by users. For instance, using Chrome or Firefox, I could right click on your form, choose inspect element, change the value of that hidden field, and presumably bypass the max file size restriction. But, since it is not being used and the restriction is hard coded into your script, you can just remove this field and not worry about it.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.