patternphpMinor
Is this file image upload safe?
Viewed 0 times
thisfileimageuploadsafe
Problem
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.
```
"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.");
}
}
?>
File
```
"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.");
}
}
?>
File
Solution
It is pretty solid for saving images on a single web server. Some optional/extra things you could do are:
-
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.
php_flag engine off
- 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.
php_flag engine off
Context
StackExchange Code Review Q#35104, answer score: 3
Revisions (0)
No revisions yet.