Security of my upload script #2

Question

I recently asked this question and got a response. I made improvements, it now checks if the file is an image and/or used UUID to store the images, and now I want to see if it's good or not.

How secure is my upload script?

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.';
        }

        if(!$errors) {

            $picToUpload =  uniqid() . '.' . $imageFileType;
            if( !move_uploaded_file($file_tmp, "assets/images/profile_pics/" . $picToUpload)) {
                
                echo "Error uploading files";
                die();
            }

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

            $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 this implementation is still lacking improvement vs the previous version.

Image size

First of all, the maximum allowed size for pictures is too large. But the biggest problem is that the pictures are stored in raw form without any optimization. So if a user uploads a picture of 10 Mb straight from their smartphone, then the picture will consume 10 Mb on the server. Since it is a profile picture the ultimate size should be closer to 50 Kb than 50 Mb. Because it should be compressed further and resized. There are massive gains to be made. Standard practice actually. Bonus: resizing the image is a good way to ensure that the uploaded file is actually an image in a proper format.

Speed

The other problem is that pages will be slow to load. Constraining pictures within a frame of 200px by 300px for example will not change anything - the browser will load the picture in full size. As long as you are developing in local mode you won't notice. It's when you go live that the slowness will become apparent.

Even in this day and age, visitors expect pages that load fast, lightning fast. They get impatient with sites that are slow and irresponsive. Bottom line, you are wasting storage space, bandwidth and people's time.

Think about your carbon footprint too. The Internet is largely running on coal. Definitely not green at all.

So, I strongly recommend that you not only resize and optimize but convert all pictures to PNG format on your end to streamline things.

Unique identifier and privacy

My advice is to ditch uniqid because as already explained it goes not guarantee uniqueness and you are not even requesting additional entropy. The odds of a collision are slim but still. The more data you have, the more there is a chance the "unthinkable" will happen. If you are using Mysql you can use the UUID() function instead. I don't think PHP has an equivalent built-in function.

I find myself in disagreement with @KIKO Software over one point. Using an incremented ID for the profile picture is more convenient, but using an incremented or predictable identifier also makes scraping and enumeration of users easier. Probably not something you really want due to privacy considerations - remember how content was scraped massively from Facebook, Linkedin etc. I don't like it either, but this is defensive coding vs convenient coding.

OK, maybe this is not the next Facebook but you have to think ahead and be aware of the future consequences of design choices you are making today.

Constants

Regarding the picture directory, it's hard-coded twice in your piece of code. It is a parameter that should go in a configuration file. So that when you need to migrate files, you won't have to rewrite the whole table in your database and change your code on top of that.

You should use a full path, not a relative path. If you restructure your code, move files to directories etc, the path could become invalid as a result and you will have to debug and rewrite your code. It can be more flexible.

Security

Security-wise the function looks sound. Since the file name is not under the control of the user (potential attacker) it is difficult to interfere with it.

Question: how is $userLoggedIn set ? You are checking that the variable is not empty, is your assumption well-tested ?

SQL performance

I might change this statement a bit:

$stmt = $con->prepare("UPDATE users SET profile_pic = ? WHERE username = ?");

and use the ID instead of the username as criterion. The ID can be loaded as a session variable along with other data. The assumption is that the username field may not be covered by an index unlike the ID, thus updates will be slower especially on a large table. Check your table structure and run an execution plan to find out more. It goes without saying that there should be a unique constraint on the username.

Type confusion

At the top of your code $errors is defined as an array:

$errors = [];

The intention being to be able to report multiple errors. But in practice you are only handling one error at a time eg:

$errors = "Only jpeg, jpg, png and gif files are allowed.";

and $errors becomes a string. To remain consistent you could do:

$errors[] = "Only jpeg, jpg, png and gif files are allowed.";

And then you can report more than one error at a time, for example "File too large" and "Wrong image format". Currently the previous error is overwritten, if any. As long as the array length is zero it means there is no error. if(!$errors) { will work as expected, it could also be expressed with the empty function.

Answer 2

Your upload script allows people to upload something to your server. It has to be a file with a .jpeg, .jpg, .png or .gif extension, but its contents can be anything. In other words, it doesn't have to be a valid image, it could be anything.

You could check that it really is an image.

Not thoroughly checking the images allows any content to be stored on your server, and you probably actively do things with this content, for instance you serve it to your visitors.

I could put Javascript code in an file with the .gif extension, and if you have another security problem elsewhere in your code I could possibly execute it in the browser of your visitors. This illustrates the point that assessing the security of a piece of code cannot be done in isolation. It also depends on all the other code, which is not present in your question.

You use ($file_size > 55097152), but that's over 50 Mega Bytes. That is just way to big for a simple profile picture. I would not go over 10 MB. It would also save you a lot of resources if you make big images small before you store them. See: imagescale().

Your code assumes unique user names, for instance in your database query. This is used in a lot of code you see, but it is better to use an unique user ID, which is usually a simple number. In your database it could be an AUTO_INCREMENT column. This also gives users more freedom to choose an user name. For instance, if the user name is also used as an email address, and someone want to start a new account with the same email address, you could allow that if you use IDs instead of names. There could also be other reasons to use IDs rather than names, but let's just say it an overall better, and more standard, approach. It is always easier to prevent problems than to correct them later.

I don't really like the way you store the image. If you look at a stored image file, you have no idea to which user it belongs. This is because you use a random UUID to generate the file name, so you have to look the image name up in the database to connect it to an user. Why not use the user's unique ID, I talked about in the previous paragraph?

You also store the path to the profile images in the database. This path it always the same, so you waste resources, but what if you ever want to move the images? You would have to update all the stored paths in your database. Since you know the directory, why not only store the file name? If you use the user's unique ID, for the file name, you don't have to store anything.

It is a good idea to define the directory where the profile images are stored only once and then use this definition in the rest of your code. If it ever needs to change you would only have to change that single definition and your code would work again.