Script di caricamento sicuro

Question

Sto creando un social network che consente agli utenti di caricare un'immagine del profilo. Voglio solo sapere se questo è un modo sicuro per farlo. Grazie.

<?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";
}

?>

Answer 1

Questa è la mia opinione personale, ma direi quanto segue:

1. Il codice dovrebbe essere formattato, guarderei personalmente alla PSR-12 poiché questo standard dovrebbe essere seguito quando possibile.
2. move_uploaded_file non protegge dall'attraversamento di directory. Dovresti usare basename on $_FILES['fileToUpload']['tmp_name']e altre forme di validazione
3. Il controllo dell'estensione del file con if(in_array($file_ext,$extensions)=== false)non impedisce a un utente di caricare un file dannoso che potrebbe, ad esempio, utilizzare un magic byte per indurre il server a pensare che si tratti di un certo tipo di file. Dovresti dare un'occhiata a finfo e al primo esempio di caricamento di file
4. Stai creando una serie di errori, che al momento vengono controllati in un'istruzione if e poi gettati via. Se non hai intenzione di usarlo, potresti essere meglio uscire dalla funzione in anticipo piuttosto che continuare l'esecuzione.
5. A seconda di quanto univoco dovrebbe essere il nome del file, potresti voler usare qualcosa di simile uniqid(mt_rand(), true)
6. move_uploaded_file sostituirà un file se esiste già, potresti voler controllare che esista prima di sovrascrivere un file esistente. A seconda della soluzione di denominazione, è molto improbabile che si verifichi, ma sotto carico elevato per lunghi periodi di tempo ciò potrebbe accadere più spesso di quanto pensi.
7. Stai usando UPDATE users SET profile_pic = ? WHERE username = ?presumo che questo valore esista nel database poiché l'utente deve essere loggato. Tuttavia, se non sei sicuro che il campo esista o meno (non ho visto il database) uso personalmente: INSERT INTO users (profile_pic, username) VALUES (?,?) ON DUPLICATE KEY UPDATE profile_pic=?, username=?questo si inserirà nella tabella se la riga non esiste ma la aggiornerà se esiste.
8. Hai impostato una variabile locale chiamata larghezza e altezza e le stai confrontando con lo stesso valore. Presumo che questo avesse lo scopo di controllare le dimensioni effettive del file?

Spero che questo aiuti in qualche modo :)