Is this file image upload safe?

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(
UPLOAD_ERR_FORM_SIZE  => "Larger than form MAX_FILE_SIZE",
UPLOAD_ERR_CANT_WRITE => "Can't write to disk",
);

$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)) {
}

//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; ?>">
</form>
</body>
</html>


Many Thanks

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>

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' ] ) ) {
}


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 ) ) {