How can I make my image upload more secure?

Question

I gathered a lot of information in order to try to make my code as secure as possible, and I would like to know what I can do to improve more its security. (Sorry for my english if it's bad)

What did I do:

• Only logged-in users will be able to upload images.
• If a user has uploaded more than 10 images in the last hour, they will have to answer a captcha for new uploads.
• I check for errors during the upload.
• I verify the file extension and mime type (using pathinfo and finfo).
• I check if the tmp_name exists.
• I limit the maximum file size.
• Using getimagesize, I get the width and height to set maximum and minimum values and verify the mime type again.
• Images will be stored in a folder outside the public_html directory.
• All images will be converted to webp format.
• Images will be displayed on a subdomain.
• The folder and image files will have permissions set to 644.
• Images will have two generated names: one for the .webp file and another name that will be used to identify the image in the database.

My implementation:

<?
//Allows only logged in users to upload images
if($ta_logado == 0){
    die('{"error":"Você precisa estar logado"}');
}

//If the user has sent more than 10 images in the last hour, request captcha
if(lastHourUploadImage() > 10){
    
    if (isset($_POST['g-recaptcha-response'])) {
        $captcha_data = empty($_POST['g-recaptcha-response']) ? NULL : filter_input(INPUT_POST, 'g-recaptcha-response', FILTER_SANITIZE_STRING);
    }else{
        die('{"error":"desafio-captcha-pendente"}');
    }

    $ip_user_valid = filter_input(INPUT_SERVER, 'HTTP_CF_CONNECTING_IP', FILTER_VALIDATE_IP);

    if(!filter_var($ip_user_valid, FILTER_VALIDATE_IP)){
        $ip_user_valid = '0.0.0.0';
    }
    
    $resposta = file_get_contents("https://www.google.com/recaptcha/api/siteverify?secret=SECRET_KEY&response=".$captcha_data."&remoteip=".$ip_user_valid);
    $resposta = json_decode($resposta, true)['success'];

    if($resposta === false){
        die('{"error":"Captcha invalida"}');
    }

}

// Check if upload is not empty
if(empty($_FILES['upload'])){
    die('{"error":"Faça upload da imagem"}');
}

// Check if upload is not empty
if (empty($_FILES['upload']['tmp_name'])) {
    die('{"error":"Faça upload da imagem"}');
}

// Check for upload errors
if ($_FILES['upload']['error'] > 0) {
    die('{"error":"Houve algum erro durante o upload da imagem"}');
}

$extensoesPermitidas = array('jpg', 'jpeg', 'png');
$mimePermitidos = array('image/jpeg', 'image/png');
$tamanhoMaximo = 2 * 1024 * 1024; // 2MB

// Check file extension
$extensao = pathinfo($_FILES['upload']['name'], PATHINFO_EXTENSION);
if (!in_array(strtolower($extensao), $extensoesPermitidas)) {
    die('{"error":"Extensão de arquivo não permitida."}');
}

// Check file mime
$tipoArquivo = getMimeType($_FILES['upload']['tmp_name']);
if (!in_array($tipoArquivo, $mimePermitidos)) {
    die('{"error":"Tipo de arquivo não permitido."}');
}

// Check if tmp file exists
if (!file_exists($_FILES['upload']['tmp_name'])){
    die('{"error":"Imagem não encontrada no servidor."}');
}

// Check file size
if ($_FILES['upload']['size'] > $tamanhoMaximo) {
    die('{"error":"O tamanho do arquivo excedeu o limite permitido."}');
}

//Check the image once more, but with GD now
$verifyimg2 = getimagesize($_FILES['upload']['tmp_name']);

if (!$verifyimg2){
    die('{"error":"Imagem invalida"}');
}

//Width and height
$largura = $verifyimg2[0];
$altura = $verifyimg2[1];

// Check minimum height and width
if ($largura < 100 || $altura < 100){
    die('{"status": "error", "text": "Esse arquivo e muito pequeno tente enviar em dimensões superiores a 100px"}');
}

// Check maximum height and width
if ($largura > 2000 || $altura > 2000){
    die('{"status": "error", "text": "Esse arquivo e muito grande tente enviar em dimensões inferiores a 2000px"}');
}

// Verify File Mime with GD
if (!in_array($verifyimg2['mime'], $mimePermitidos)) {
    die('{"error":"Tipo de arquivo não permitido."}');
}

// Working the image with GD
if ($verifyimg2['mime'] == "image/jpeg") {
    $image_create = imagecreatefromjpeg($_FILES['upload']['tmp_name']);
}

if ($verifyimg2['mime'] == "image/png") {
    $image_create = imagecreatefrompng($_FILES['upload']['tmp_name']);
    imagepalettetotruecolor($image_create);
    imagealphablending($image_create, true);
    imagesavealpha($image_create, true);
}

if(!isset($image_create)){
    die('{"error":"Erro desconhecido."}');
}

//If tipo_upload is avatar, save in different folder
if(isset($_POST['tipo_upload']) && $_POST['tipo_upload'] == 'avatar'){
    $path = createDirectoryIfNotExist('../upload/avatar_uploaded');
    $is_avatar = 1;
}else{
    $path = createDirectoryIfNotExist('../upload/images_uploaded');
    $is_avatar = 0;
}

// If the image is an avatar, we will leave it with a size of 140px and if it is not, we will do nothing
if($is_avatar == 1){

    // Cria uma nova imagem com o novo tamanho
    $resizedImage = imagecreatetruecolor(140, 140);

    // Redimensiona a imagem original para a nova imagem
    imagecopyresampled(
        $resizedImage, // Imagem de destino
        $image_create, // Imagem original
        0, 0, // Coordenadas da imagem de destino
        0, 0, // Coordenadas da imagem original
        140, 140, // Novo tamanho
        $largura, $altura // Tamanho original
    );

    $image_create = $resizedImage;

    imagedestroy($resizedImage);
}

//Create file names
$name_save = uniqid('IMG-'.$id_user_logado.'-', true) . '_' . str_shuffle(implode(range('e', 'q'))).'.webp';
$slug_name = uniqid('IMG-'.$id_user_logado.'-', true) . '_' . str_shuffle(implode(range('e', 'q')));

// Convert image to WebP
$success = imagewebp($image_create, $path.'/'.$name_save, 80);

// Free memory
imagedestroy($image_create);

if ($success) {

    // Set file permissions
    chmod($path.'/'.$name_save, 0644);

    //Send to the database
    sql($db_user, "INSERT INTO images_upload(user_id, slug_name, name_save, data_send, is_avatar) VALUES (?, ?, ?, NOW(), ?)", array($id_user_logado, $slug_name, $name_save, $is_avatar), "fake");
   
    die('{"success":"http://images.localhost/media/'.$slug_name.'"}');

} else {
    die('{"error":"Erro ao concluir processamento do arquivo."}');
}

function createDirectoryIfNotExist($baseDir){

    // get day and month
    list($year, $month) = explode('-', date('y-m-d'));

    // Create the path
    $targetDir = $baseDir . '/' . $year . '/' . $month;

    // Check the path
    if (!is_dir($targetDir)) {
        if (!mkdir($targetDir, 0644, true)) {
            die('{"error":"Não foi possivel seguir com sua solicitação contate um administrador"}');
        }
    }

    return $targetDir;
}

function lastHourUploadImage(){
    global $db_user, $id_user_logado;
    return sql($db_user, "SELECT * FROM images_upload WHERE user_id = ? AND data_send >= DATE_SUB(NOW(), INTERVAL 1 HOUR)", array($id_user_logado), "count");
}

function getMimeType($filename) {
    $finfo = finfo_open(FILEINFO_MIME_TYPE);
    $mimeType = finfo_file($finfo, $filename);
    finfo_close($finfo);
    return $mimeType;
}

How do I show the image

<?
//Dados banco de dados usuarios
define("HOST", 'localhost');
define("USER2", 'root');
define("PASS2", '');
define("NAME2", 'admin_account');

//Conectamos ao banco de dados dos usuarios
try {
    $db_user = new PDO("mysql:host=".HOST.";dbname=".NAME2.";charset=utf8mb4", "".USER2."", "".PASS2."");
    $db_user->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $db_user->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
    $db_user->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
}catch(PDOException $e2) {
    die("Erro ao conectar no banco de dados");
}

// Função basica para gerenciamento PDO
function sql($db_atual, $q, $params, $return) {
    
    try {

        //prepara a querry
        $stmt = $db_atual->prepare($q);

        //executa a query
        $stmt->execute($params);
        
        // retorna no formato solicitado pela variavel
        if ($return == "rows") {
            return $stmt->fetch();
        }elseif ($return == "rowsall") {
            return $stmt->fetchAll();
        }elseif ($return == "count") {
            return $stmt->rowCount();
        }elseif( $return == "lastid"){
            return $db_atual->lastInsertId();
        }
        
    }catch(PDOException $e) {
        
        echo "DataBase Erro: ".$e->getMessage();
        error_log($e->getMessage());
        die();
        
    }catch(Exception $e) {
        
        echo "Erro Geral: ".$e->getMessage();
        error_log($e->getMessage());
        die();
        
    }
    
}

if(!isset($_GET['image'])){
    die('Informe a imagem');
}

$buscar_image = sql($db_user, "SELECT * FROM images_upload WHERE slug_name = ?", array($_GET['image']), "rows");

if(!isset($buscar_image['id'])){
    die('ERROR - IMAGEM NÃO ENCONTRADA');
}

if($buscar_image['is_avatar'] == 1){
    $info_path = 'avatar_uploaded';
}else{
    $info_path = 'images_uploaded';
}

list($ano, $mes) = explode('-', date('y-m-d', strtotime($buscar_image['data_send'])));

$path = '../upload/'.$info_path.'/'.$ano.'/'.$mes.'/'.$buscar_image['name_save'];

header("Content-Type: image/webp");
header('X-Content-Type-Options: nosniff');
readfile($path);
exit;

Answer 1

Security

If you are worried about security (and you should), then you should not connect to the database with the root user. The Mysql root user has extended rights like reading other databases, reading arbitrary files on the file system, and even writing to some designated directories (or worse if you have explicitly tweaked configuration).

In case of vulnerability in your code (more specifically a SQL injection) this could escalate to takeover of your server, in addition to exfiltration of your database.

At first glance, I think this is the biggest oversight. A hacker will be going for the low hanging fruit, and an attack will not necessarily take place where you expect.

Create a user that has access to that database only, and do not grant more rights than necessary for your purpose. Usually you just need to read from and write to tables, you don't need to change schema or FILE privilege.

Speaking of SQL injection, you've done what has to be done.

Unchecked checks

Some stuff is redundant eg:

// Check if upload is not empty
if(empty($_FILES['upload'])){
    die('{"error":"Faça upload da imagem"}');
}

// Check if upload is not empty
if (empty($_FILES['upload']['tmp_name'])) {
    die('{"error":"Faça upload da imagem"}');
}

It should be sufficient to use the is_uploaded_file function and then there is no conceivable reason why the tmp_name attribute would be missing. Even if that happened, an exception would be raised and the code would simply stop.

And you also do this further down:

// Check if tmp file exists
if (!file_exists($_FILES['upload']['tmp_name'])){
    die('{"error":"Imagem não encontrada no servidor."}');
}

Which is another unneeded check. The file must exist if the previous checks have gone through.

Make sure you have read the PHP docs on file uploads, which would be a good starting point to understand the possible pitfalls.

This at line 93:

// Verify File Mime with GD
if (!in_array($verifyimg2['mime'], $mimePermitidos)) {
    die('{"error":"Tipo de arquivo não permitido."}');
}

is a repetition of lines 58-60:

if (!in_array($tipoArquivo, $mimePermitidos)) {
    die('{"error":"Tipo de arquivo não permitido."}');
}

As far as I can tell, you are still processing the same image. It seems to me that you've added so many checks that you are losing track of the process.

This code could be simplified a bit.

It would surely be useful to split your code in small functions. For example, write a dedicated function to check images. Then the redundant code would stand out better.

File uploads are dangerous if not done right. Probably the most important is to restrict the file extension: if an attacker can upload a .php file (a webshell) to your server then you have a problem. Some webserver configuration can still mitigate this problem, for example I believe Nginx won't run files it does not own. Other pitfalls to avoid include directory traversal attacks, which can happen if rely on file name without sufficient sanitization.

I believe the code is safe enough at first glance. I haven't run it though, and this is nothing more than visual analysis.

File size and disposal

You're restricting the file size to 2 Mb which is low by today's standards. Pictures from digital cameras or smartphones typically weight a couple Mbs. Many of your users will have to tweak with settings or edit the picture on their own before they can even upload it to your site. Consider raising the limit a bit. Since you are not using the move_uploaded_file function but building a new image, I suggest you erase the temporary file when you're done with it. Although the temp directory should get flushed at the next reboot, some servers do not reboot frequently. It's conceivable that the temp could fill up under heavy usage.

Misc

To determine that the user is logged in you rely on this:

$ta_logado == 0

It's not known how and when this variable is set, and whether it will always be in sync. I would rather expect a reference to a session object here.

Answer 2

This is going to be a reasonably short review, with just a few ideas I had when I looked at your code. Other people might do a thorough review.

die, die, die

You used this a lot: die('{"error":"The error message."}');, you could create a function for that:

function returnError($message)
{
   die(json_encode(["error" => $message]));
}

Notice that I actually use json_encode() to create the JSON string. This is better because certain characters should be escaped inside a JSON string value. If you also want to log these errors you can do that in this function.

Image Processing and Generation

You are using GD for your image functions, and you're restricting the image formats to 'jpg', 'jpeg', 'png' and the size to between 100 and 2000 for width and height.

Although I can understand why you made these choices, I think your users will be happier if you were just simply able to accept the images they upload. Perhaps they have a TIFF image, or something from a mobile phone which can easily be bigger than 2000 pixels.

It's probably not because your server cannot handle these images, it's just that GD cannot. PHP however supports other image processing extensions like Gmagick and ImageMagick.

These are very similar to each other and far more powerful than GD. They support over a 100 image formats. Rewriting your code for one of these extensions means the images your users upload will be more readily accepted and that can only be a good thing.

All you do is try to load the image in one of these extension and see if it worked. If it did you resize and save it, and you're done. If it didn't work you return a detailed error message.