Does my image upload look safe enough?

Question

I have an image upload script that uploads images to the server:

<?php
define("UPLOAD_DIR", "uploads/");
$fileType = exif_imagetype($_FILES["myFile"]["tmp_name"]);
$allowed = array(IMAGETYPE_GIF, IMAGETYPE_JPEG, IMAGETYPE_PNG);
if (!in_array($fileType, $allowed)) {
 header('Location: edit.php');
 exit();
} 
if (!empty($_FILES["myFile"])) {
 $myFile = $_FILES["myFile"];
 if ($myFile["error"] !== UPLOAD_ERR_OK) {
 echo "<p>An error occurred.</p>";
 exit;
 }
 // ensure a safe filename
 $name = preg_replace("/[^A-Z0-9._-]/i", "_", $myFile["name"]);
 // don't overwrite an existing file
 $i = 0;
 $parts = pathinfo($name);
 while (file_exists(UPLOAD_DIR . $name)) {
 $i++;
 $name = $parts["filename"] . "-" . $i . "." . $parts["extension"];
 }
 // preserve file from temporary directory
 $success = move_uploaded_file($myFile["tmp_name"],
 UPLOAD_DIR . $name);
 if (!$success) { 
 echo "<p>Unable to save file.</p>";
 exit;
 }
 // set proper permissions on the new file
 chmod(UPLOAD_DIR . $name, 0644);
}

Is it safe? Are there any good tips to protect myself against attacks via the image upload?

Here's my markup, if that counts:

<form action="upload.php" method="post" enctype="multipart/form-data">
<input type="file" name="myFile" id="file" capture="camera" accept="image/*"><br>
<input type="submit" name="submit" value="Submit">
</form>

• CodeX: I would check for !empty && in_array just to be safe, otherwise, it looks pretty good
• CodeX: Just noticed that you use exit(); then exit; is exit() a function?
• user2981256: That's just to kill the script php.net/manual/en/function.exit.php @CodeX
• Alex L: @codex you should have written an answer!
• CodeX: @AlexL It was late and I was on my mobile

Answer 1

The Security Aspect

Good to see that you're not just checking for a file extension in the name!!

Basically, yes, it's the minimal security needed. There aren't too many steps needed to make an image uploading script safe, however, doing it wrong could present some future issues.

Here's some notes though:

• When renaming the file, what's the point in changing the name to something similar but with characters replaced? You could (not recommended) keep the original name, or you could completely alter the name to something unpredictable, and then use a look-up table (i.e. database) to point the user in the correct path. Instead of simply swapping out illegal characters with underscores, give the file a name that has no resemblance.

• The permissions on your directory do look okay. It is however, not a bad idea to simply remove the images from the public directories, place them in say var/www/uploads/ and let a script get them for you. You don't want these images being in a directory where PHP (or any other server scripts) may run.

• Simply appending $parts["extension"] onto the file seems risky to me. You did check the image type, however I'm not sure how it responds to files with multiple extensions. Someone with more knowledge here could extend this...

Non-Security Related

• exit(); and exit; are used throughout. Please don't! Two things: be consistent, they're aliases (along with die); don't use them, they leave the end-user hangin'! These kill the page with no clean way to display an error.

• Using names such as myFile are quite book/tutorial copy-and-pasted. Choose names that have meaning to your application.

• echo "<p>An error occurred.</p>"; Lot's of things I could say here. 1) An error!? No kidding! You kill the page right after this without even saying what the user should do to correct the error. (Doesn't matter if you're the only person using this)

• $name, $parts, $success - more unhelpful variable names!

Other than that, it's a simple piece of code, and there's already tons of people who have done this, plus more. It's usually safer to work with a community run library than to create your own!

• user2981256: I actually just have one more question. What if I was to store the images on a subdomain? Would that make it any safer?
• Alex L: If i understand what you mean, no. The public can still access a subdomain. That'd be more obfuscation, a weak form of security