2
\$\begingroup\$

Could you guys tell me if my code is safe? And i want to know how to improve it. You will see a detailed explanation about the code on the comment blocks inside the code.

profileIMGform.php

require_once 'db.php';

if(!empty($_SESSION['user_id'])){
  $user_id = $_SESSION['user_id'];
}else{
  $user_id = NULL;
}

// here i check if the user is logged in
if(\array_key_exists('user_id', $_SESSION) && $user_id = \filter_var($_SESSION['user_id'], \FILTER_VALIDATE_INT)){

    if($_SERVER['REQUEST_METHOD'] == 'POST'){              
        //i use this condition because i have another code about file upload inside this file
        if(!empty($_FILES['uploadedIMG']['size'])){

            $finfo = finfo_open(FILEINFO_MIME_TYPE);
            $mimeType = finfo_file($finfo, $_FILES["uploadedIMG"]["tmp_name"]);

            $allowedMimeTypes = [
                'image/jpeg',
                'image/jpg',
                'image/png'
            ];

            list($width, $height) = getimagesize($_FILES["uploadedIMG"]["tmp_name"]);
            $sizeFILE = filesize($_FILES["uploadedIMG"]["tmp_name"]);
            //here i check if the uploaded file is a "valid" image, the function "getimagesize()"
            // is not 100% trustable, but i added this function here as a "wall" 
            if(in_array($mimeType, $allowedMimeTypes) && $width != null && $height != null && $sizeFILE <= 768000){   
                //if the uploaded file is a "valid" image, then it will execute this code on the 
                //try/catch
                try{
                    $linkN = $_SESSION['linkN']; //user local folder name
                    $imgPF = $_SESSION['img']; //user defaut avatar img that is set when they 
                    //resgister the account
                    $capaPF = $_SESSION['capa']; //user defaut wallpaper that is set when they 
                    //resgister the account
                    $linkNPFFetch = $linkN;
                    $folderPFFetch = 'user/'.$linkN.'/'; //this variable is used on the glob() 
                    //function bellow

                    // here i made a SELECT query to get the slug from the username, just in case 
                    //if they changed the username while logged in, this slug is used on the URL
                    $perfilUpdated = $conn->prepare("SELECT `slug_UN` FROM `users` WHERE `user_id` = :user_id");
                    $perfilUpdated->bindParam(':user_id', $user_id, PDO::PARAM_INT);
                    $perfilUpdated->execute();
                    $rowPFUpdated = $perfilUpdated->fetch(PDO::FETCH_ASSOC);

                    $SlgUSPFFetch = $rowPFUpdated['slug_UN'];

                    $ext = explode('.',$_FILES['uploadedIMG']['name']);
                    $extension = end($ext); //here i get the extension of the uploaded file, 
                    $newname = mt_rand(10000, 10000000); //generate a random name
                    $folderPFFetchFILE = $folderPFFetch.'avatar/'.$newname.'_'.time().'.'.$extension;

                    //here i check if exist a file on the user's folder
                    $imgUserPasta = glob($folderPFFetch.'avatar/*')[0] ?? NULL;

                    // i don't remember why i made this variable, anyways it's not relevant, 
                    //you can just forget it
                    $checkPathEdelet = $folderPFFetch.'avatar/'.($_FILES['uploadedIMG']['name']);

                    $imagickWrite = 'C:/xampp/htdocs/site/user/'.$linkN.'/avatar/'.$newname.'_'.time().'.'.$extension;

                    if($imgUserPasta != NULL && $imgUserPasta != $checkPathEdelet){
                        unlink($imgUserPasta);
                    }

                    // here is how i'm 99% sure that i will get only valid images, if the uploaded image 
                    //is not valid it won't execute the Imagick code and will print out a error, but 
                    //i'm using a try/catch and this error won't appers
                    $imgk = new Imagick($_FILES["uploadedIMG"]["tmp_name"]);
                    $imgk->thumbnailImage(180, 180, true);
                    $imgk->writeImage($imagickWrite);

                    $imgUserPastaIMCK = glob($folderPFFetch.'avatar/*')[0] ?? NULL;
                    if(file_exists($imgUserPastaIMCK)){
                        $avatar = $siteURL.'/'.$imgUserPastaIMCK;
                    }else{
                        $avatar = $siteURL.'/img/avatar/'.$imgPF.'.jpg';
                    }                  
                    echo json_encode(['avatar' => $avatar]);     

                }catch(Exception $e){

                    // even if Imagick will only upload valid images, it's recommended to use other 
                    //ways to also validade images 
                    //so i have 3 "walls": 
                    // 1° - i check the mimetype with "finfo_open()", 
                    // 2° - i check if is a "valid" image with "getimagesize()" 
                    // 3° - Imagick will only upload valid images

                    // if a user falls here on the "catch" block, i'm 100% sure that it's a "malicious"
                    //user, so i made this query to delete his profile from my database, altrough it
                    //don't give any protection against attackers, it will make their life a 
                    //bit harder, because they will need to create another account to try uploaded
                    //malicious files to my server.

                    // i know that there's ways to forge a user ID, i don't know if it can be possible
                    //in my project because i don't generate ID when the user loge in, i store the user
                    //ID on my database when they register the account, anyways i think that this DELETE
                    //query will be better than nothing.
                    $deleteProfile = $conn->prepare("DELETE FROM `users` WHERE `user_id` = :user_id");
                    $deleteProfile->bindParam(':user_id', $user_id, PDO::PARAM_INT);
                    $deleteProfile->execute();
                    session_unset(); 
                    session_destroy();
                    echo json_encode(['delete' => 'true']);

                }

            }         
        }

    }

}
$conn = null;
?>

This is the code that is inside success on Ajax call, it's not related to security, because important things should be made on back-end, but i will put this code here just to you guys see how exactally my code works:

$('#imgUpload').change(function (event) {
...
...
...
 success: function (data){
  var jsonPF = JSON.parse(data);           
  if($.trim(jsonPF.delete)){ // if the data returned from 
  //the profileIMGform.php contains the array 'delete', then
  //te page will be reloaded and the user will seen a error page,
  //with the mensage: "You need to be logged in to view this page" 
    $("img.imgPform").css('opacity', '0.4');
    $("#loadingIMG").show();
    window.location = siteURL;
  }else{
    $("#loadingIMG").hide();
    $("img.imgPform").css('opacity', '1');
    $(imgEdit).attr('src', jsonPF.avatar+'?'+ new Date().getTime());
    if(window.matchMedia("screen and (max-width: 1172px)").matches) {
      $(imgEditMd).attr('src', jsonPF.avatar+'?'+ new Date().getTime());
    }else if(window.matchMedia("screen and (min-width: 1173px)").matches) {
      $(imgEditMm).attr('src', jsonPF.avatar+'?'+ new Date().getTime());
    }
  }
 }
\$\endgroup\$
4
\$\begingroup\$

Indentation

First of all, format your code wisely. Do not make your main program body shifted by five levels of indentation. It makes it awfully hard to read (and review, mind you).

Fail early. If a condition makes the further code execution impossible - just make it throw an error and then keep writing the following code on the same level, like

require_once 'db.php';

if(!empty($user_id)) {
    die(json_encode(['error' => 'Unauthorized']));
}
$user_id = $_SESSION['user_id'];
if(empty($_FILES['uploadedIMG']['size'])) {
    die(json_encode(['error' => 'No file uploaded']));
}

As you can see, here we made your code die early (but informative, so in case the code won't process the image you would have an idea why), and then continue from the same indentation level.

Code pollution

As you can see, I removed a lot of code from a mere few lines. It means there was a lot of code which is just useless. There are two reasons for this:

  • What you wrote in the user id checking code could be called "defensive programming" but in reality it is not:
    • First of all, a defensive programming code should always raise an error if a condition is not met. While your code just silently bypasses it. So if ever a user id won't be int, you will just have no idea why your code suddenly stopped working.
    • even a defensive programming should be justified. You can wrap every operator in a dozen conditions, but it will just make no sense. Use strict verifications only if there is a chance that a variable could be the wrong type. Assuming user_id coming from a database, I don't see how a non-empty one could be any other type than int.
  • You don't take the surrounding code into consideration. For example,
    • you are checking the $_SESSION['user_id'] variable twice. Why? One condition is enough, then just assign it to a variable.
    • this one is rather tricky but $_FILES['uploadedIMG']['size'] will be empty if $_SERVER['REQUEST_METHOD'] is not 'POST'. So it makes the latter superfluous.

So just remember - the more code you write, the harder it is to read and the more errors you introduce.

Security

Like it often happens, while trying to do "the most secure way" you are overreacting. Most of your verification code is just duplicates itself. For example, getimagesize() would use the same mechanism as fileinfo. So just one call is enough.

At the same time, there is an open vulnerability. Your web-server doesn't judge your files by whatever "mime type" but by the extension. In means that if an image file will have a .php extension, a web-server will try to run it as a PHP file, not serve it as an image file. So, security-wise all your numerous mime-type checks are rather useless while the only real protection is missing.

You must verify the file extension against a while list

$allowedExts = array('png', 'jpeg', 'jpg', 'gif');
$ext = pathinfo($name, PATHINFO_EXTENSION);
if( !in_array($ext, $allowedExts) )
{
    die(json_encode(['error' => 'invalid image format'));
}

this one, given you are renaming the files, will be the real protection.

Innocent users' experience

It is very important not to spoil the innocent users' experience in pursue for the imaginary security. And you are outright deleting a user account if their image upload failed, which is gross. Imagine your frustration as are an active member for several years who just accidentally uploaded a broken image, like sometimes our cameras produce. Then out of the blue you have your account deleted!

At the same time, as you admit yourself, it "don't give any protection against attackers"! The consequences are just incomparable! So this approach won't offer any real protection but could ruin the whole site.

There was an old short story from the great science fiction writer Robert Sheckley, "Ghost V". Arnold, the main character, was trapped in the space ship with nightmares. On one occasion, he had to keep the lights on, but but wast tricked into believe that some creatures are attacking the lights. So he shot on them and as a result shot all the lights off!

Don't be like Arnold, do not shoot your users off only to "make an attacker's life a bit harder".

Better user experience

It is considered a standard to let a user know what is their fault. So don't just return an empty response but tell a user what's wrong.

if (filesize($_FILES["uploadedIMG"]["tmp_name"]) > 768000){
    die(json_encode(['error' => 'File is too big']));
}

Error reporting

i'm using a try/catch and this error won't appear

Here you are outright wrong. PHP has a designated setting that prevents errors from appearing. And it should be set to OFF on a live site. While in the program code there should be no operators responsible for this task. Please refer to my article on PHP error reporting for the further explanation.

However, as image processing error is a deliberately recoverable error, you can just inform the user and let it go.

The refactored code.

To be honest, I was unable to understand the whole affair with the image name, but as far as I can tell there was an awful lot of code that is either duplicated or not used at all. So, assuming there could be just one avatar, I decided to make its name static.

<?php
require_once 'db.php';

if(!empty($user_id)) {
    die(json_encode(['error' => 'Unauthorized']));
}
$user_id = $_SESSION['user_id'];
if(empty($_FILES['uploadedIMG']['size'])) {
    die(json_encode(['error' => 'No file uploaded']));
}
//Image verifications
$size = getimagesize($_FILES["uploadedIMG"]["tmp_name"]);
if (!$size) {
    die(json_encode(['error' => 'Unrecognized file format']));
}
if (filesize($_FILES["uploadedIMG"]["tmp_name"]) > 768000){
    die(json_encode(['error' => 'File is too big']));
}
$allowedExts = array('png', 'jpeg', 'jpg', 'gif');
$ext = pathinfo($name, PATHINFO_EXTENSION);
if( !in_array($ext, $allowedExts) )
{
    die(json_encode(['error' => 'invalid image format']));
}
// saving the image
$imagePath = "/user/{$_SESSION['linkN']}/avatar/avatar.$ext";
$imagickWrite = $_SERVER['DOCUMENT_ROOT'].$imagePath;
try {
    $imgk = new Imagick($_FILES["uploadedIMG"]["tmp_name"]);
    $imgk->thumbnailImage(180, 180, true);
    $imgk->writeImage($imagickWrite);
} catch (ImagickException $e){
    die(json_encode(['error' => 'Error processing the image']));
}
echo json_encode(['avatar' => $imagePath]);

Note that you should amend your client side code in order to make it process errors returned.

\$\endgroup\$
  • \$\begingroup\$ accidentally uploaded a broken image I think i got what you said about the DELETE query. But this broken image will pass the getimagesize() condition? \$\endgroup\$ – Natalie Jan 13 at 19:32
  • \$\begingroup\$ What about the ImageMagick? the guy bellow said that if someone modify the headers of the image (inserting some code inside the image) it will be uploaded successfully to my server. Should i use GD to read the file and create a new one? \$\endgroup\$ – Natalie Jan 13 at 19:44
  • \$\begingroup\$ This is what i should do stackoverflow.com/a/18948152/10443076 ? There's some ways to disable execution of scripts on a folder, as you can see on the other answears in this link. \$\endgroup\$ – Natalie Jan 13 at 20:05

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.