Secure upload script

Question

I am creating a social network that let's users upload a profile picture. I just want to know if this is a secure way of doing it. Thanks.

<?php
include 'includes/header.php';
include 'includes/form_handlers/settings_handler.php';
//$userPic = ''; 
$date_time = date('Y-m-d_H-i-s');
if(!empty($userLoggedIn)) {
 if (isset($_FILES['fileToUpload'])) {
 $errors= array();
 $file_name = $_FILES['fileToUpload']['name'];
 $file_size = $_FILES['fileToUpload']['size'];
 $width = 1500;
 $height = 1500;
 $file_tmp = $_FILES['fileToUpload']['tmp_name'];
 $file_type = $_FILES['fileToUpload']['type'];
 $tmp = explode('.',$_FILES['fileToUpload']['name']);
 $file_ext=strtolower (end ($tmp));
 $extensions = array( "jpeg", "jpg", "png", "gif");
 if(in_array($file_ext,$extensions)=== false){
 $errors[]="extension not allowed, please choose a JPEG or PNG file.";
 }
 
 if ($file_size > 8097152) {
 $errors[] = 'File size must be 2 MB';
 }
 
 if ($width > 1500 || $height > 1500) {
 echo"File is to large";
 }
 if(!$errors) {
 $userPic = md5($_FILES["fileToUpload"]["name"]) . $date_time . " " . $file_name;
 $profilePic = move_uploaded_file($file_tmp,"assets/images/profile_pics/" . $userPic);
 $file_path = "assets/images/profile_pics/" . $userPic;
 $stmt = $con->prepare("UPDATE users SET profile_pic = ? WHERE username = ?");
 $stmt->bind_param('ss', $file_path, $username);
 $stmt->execute();
 $stmt->close();
 header('Location: settings.php');
 exit();
 
 }
 }
} else {
 echo "Invalid Username";
}
?>

• pacmaninbw: Rather than add all that code to the question after an answer has already been posted, please post a follow up question with the added code. By updating the code you have invalidated the answer. Please see the help center
• user13477176: @pacmaninbw I just wanted to know if that's how my profile picture code should be
• pacmaninbw: That;s a valid question, but I and 2 other more experienced members of the community were notified about the update that invalidated the answer. The community as a whole prefers that follow up questions be separate questions. You might notice that it was another user that rolled back the question to its original state. The 2nd Monitor chat room has a bot that identifies edits that invalidate answers.
• mickmackusa: fyi: File is to large should be File is **too** large @user13477176

Answer 1

This is my personal opinion, but I'd say the following:

1. The code should be formatted, I'd personally look at PSR-12 as this standard should be followed when possible.
2. move_uploaded_file doesn't protect against directory traversal. You should use basename on $_FILES['fileToUpload']['tmp_name'] and some other forms of validation
3. Checking the file extension with if(in_array($file_ext,$extensions)=== false) doesn't prevent a user from uploading a malicious file they could for instance use a magic byte to trick the server into thinking it's a certain type of file. You should take a look at finfo and the first example on file upload
4. You're create an array of errors, currently that's being checked in an if statement and is then thrown away. If you aren't planning on using it you might be better just returning out of the function early rather than continuing execution.
5. Depending on how unique the filename should be you might want to use something like uniqid(mt_rand(), true)
6. move_uploaded_file will replace a file if it already exists, you might want to check that this exists before you overwrite an existing file. Depending on your naming solution it's very unlikely to occur but under high load for long periods of time this could happen more often than you think.
7. You're using UPDATE users SET profile_pic = ? WHERE username = ? I'd assume that this value exists in the database as the user needs to be logged in. However, if you aren't sure if the field exists or not (I haven't seen the database) I'd personally use: INSERT INTO users (profile_pic, username) VALUES (?,?) ON DUPLICATE KEY UPDATE profile_pic=?, username=? this will insert into the table if the row doesn't exist but will update it if it does.
8. You've set a local variable called width and height and are comparing them to the same value. I assume this was meant to check the actual file dimensions?

I hope this helps in some way :)

• user13477176: Is the second part of my code secure ?
• Sᴀᴍ Onᴇᴌᴀ: Because the original post was rolled back the "second part of the code" isn't visible anymore. Per the help center page What should I do when someone answers my question? an option is to post a follow-up question with the second part of the code.