PHP Upload script security

Question

I am creating a social network and part of the features allows people to upload a profile pic. I want to know how secure my code is. From start to finish.

<form action="upload_profile_pic.php" method="POST" enctype="multipart/form-data">

</form>

---

include 'includes/header.php';

if ($_SERVER['REQUEST_METHOD'] != 'POST' || !isset($userLoggedIn)) {
    # code...

    header('Location: profile.php');
    exit();
}

$date_time = date('Y-m-d_H-i-s');
$errors = [];

if(!empty($userLoggedIn)) {

    if (isset($_FILES['fileToUpload'])) {

        $errors = array();
        $file_name = $_FILES['fileToUpload']['name'];
        $file_size = $_FILES['fileToUpload']['size'];
        $file_tmp = $_FILES['fileToUpload']['tmp_name'];
        $file_type = $_FILES['fileToUpload']['type'];
        $tmp = explode('.',$_FILES['fileToUpload']['name']);
        $file_ext = strtolower(end ($tmp));
        $imageFileType = strtolower(pathinfo($file_name, PATHINFO_EXTENSION));

        $extensions = array("jpeg", "jpg", "png", "gif");

        if(in_array($file_ext, $extensions) === false){

            $errors = "Only jpeg, jpg, png and gif files are allowed.";
        }
 
        if ($file_size > 55097152) {

            $errors = 'File is too large.';
        }

        //$file_name = imagescale($file_name , 100, 100); // width=500 and height = 400

        if(!$errors) {

            $userPic = $userLoggedIn . " " . $date_time . " " . $file_name . " " . uniqid() 
                . '.' . $imageFileType;

            $profilePic = move_uploaded_file($file_tmp,"assets/images/profile_pics/" . $userPic);

            $file_path = "assets/images/profile_pics/" . basename($userPic);

            $stmt = $con->prepare("UPDATE users SET profile_pic = ? WHERE username = ?");
            $stmt->bind_param('ss', $file_path, $username);
            $stmt->execute();
            $stmt->close();

            header('Location: profile.php');
            exit();
        }
    }
}

$_SESSION['error'] = '<b><p style="color: #000; font-size: 30px; top: 34%;right: 60%;position: absolute;">
' . $errors . '</p></b>';
header('Location: profile.php'); 
exit();

Answer 1

I think the way you rename the image is too convoluted. Since you have a database you could simply build a pattern with the user ID, followed by the original file extension. As the documentation says, uniqid does not guarantee uniqueness of value. This is probably a moot point, because you are using additional elements to build the file name, thus the likelihood of a collision is slim anyway, but it's important to keep in mind the limitations of the functions being used. The user ID should be unique, so it is a sound basis.

On the other hand you might have good reasons not to base the file name on the user ID, for instance if you want to avoid enumeration of user names/IDs, and don't want bots scraping data (and profiles) too easily from your website. We don't need another Facebook scandal right. It's just that I can't see the benefit in your current pattern, what's the point of storing datetime and other stuff in the file name.

Since you are using PATHINFO_EXTENSION, you can ditch $file_ext. PATHINFO_EXTENSION is sufficient.

One issue is that you are building the target file name using $file_name, which is: $_FILES['fileToUpload']['name']; and represents the original name of the file that was uploaded. You are not sanitizing it but it is untrusted third-party input by definition. It could contain invalid characters, or stuff like ../.. to carry out path traversal attacks, meaning it could be uploaded outside of the intended target directory. By the way you should define a constant for the directory assets/images/profile_pics/ for more flexibility and to avoid repetition. Add it to a configuration file and use a fully qualified (not relative) path instead so there is no ambiguity and possible issues in case you adapt your virtual host configuration. I suggest you also set the maximum allowed file size to a configuration file as well. It's better to centralize those parameters.

To sum up: don't reuse any part of the original file name while building the target file name on the server, certainly not without sanitization. Not even the extension, I'll explain why later on.

You should verify that the call to move_uploaded_file returns true - it can always fail for some reason. Note that the uploaded file could have a size of zero bytes. Check that too. And you can ditch $profilePic since it does not seem to be used at all.

I don't think your function is complete at this point, because you are normally not going to store the uploaded picture as-is. You will want to apply post-processing, resize it - especially if it is an avatar it is supposed to fit within a frame of specific dimensions - and maybe add watermarks (that could be invisible) to discourage reuse by scrapers or dubious entities like Clearview.

Also, it makes sense to harmonize pictures and convert them all to a single format - often PNG but could be JPEG.

You can do all that with GD or Imagemagick. By doing that you will also ensure that the uploaded file is indeed a valid picture file, regardless of what the file extension suggests. Merely checking the file extension is not enough. If I can upload a webshell to your site and bypass the extension check and you're in trouble. The file extension check seems safe enough at first glance but there are people more experienced than I am at spotting vulnerabilities.

I don't know if you're using Apache, Nginx or something else but you can harden your configuration by stipulating that .php files and other executables cannot be run in the uploads directory. This is an additional precaution that could mitigate the unintended consequences of poor code. If I can upload a webshell to your site, but I can't execute it then the attack fails, unless I am creative and can find a workaround. But the job is made more difficult for an attacker as a result of a restrictive (but thoughtful) server configuration.

The file should also contain some EXIF metadata. In fact you should also strip the EXIF metadata from the picture, because it may contain a lot of personal information like brand of camera/phone but also GPS coordinates, that is detrimental to privacy. Most of your users are very probably unaware of EXIF but will be grateful to you one day for taking care of privacy.

To repeat myself, there are mature libraries on the market and development frameworks designed to alleviate the work of the developer. File uploads are notoriously dangerous, this is kinda reinventing the wheel and could expose you to danger if not done right.

One last note: I have not run your code but I have the impression that if $_FILES['fileToUpload'] is not set (no valid upload) then no error message is returned. Your form should provide meaningful feedback to the user if data that is expected is missing from the form submit. Likewise, if the file exceeds the magic size of 55097152 bytes, your form only tells the user: 'File is too large.', but the user is left to guess what the acceptable size would be.

Don't you hate those forms that tell you there is a problem with your input, but don't tell you what the error is ?

I am even wondering where that 55097152 value comes from. 52.5447 Mb ? Why ? Does your PHP configuration even allow it ? It seems excessive for a picture or avatar anyway. Something like 1024 * 1024 * 5 should be reasonable. Or ten if you will. A high-res picture weighs a few megabytes. Remember, you are going to resize it.