Bonjour,

je suis actuellement en train de lire des recommandations concernant l'upload afin de sécuriser un formulair et je suis tombé sur un cours du site "Protégez-vous efficacement contre les failles web" et en lisant la partie "LA FAILLE UPLOAD" contenant un "petit script pour bien gérer vos fichiers" avec plein d'erreurs.
Le script en question (avec la vrai indentation... je déconne pas):

<?php

// On crée un nom de fichier aléatoire
$nom = sha1(session_id().microtime());

// On choisit les extensions autorisées
$extensions = array('png', 'jpg', 'jpeg', 'txt'); 

// On vérifie l'extension du fichier
if(!in_array(substr(strrchr($_FILES['fichier']['name'], '.'), 1), $extensions))
    {     
        echo "Impossible d'uploader ce type de fichier";
    }

// On récupère l'extension du fichier
$extension_fichier = pathinfo($_FILES['fichier']['name'], PATHINFO_EXTENSION);

// deuxième vérification d'extension
if (!in_array($extension_fichier, $extensions))
    {
        echo "Impossible d'uploader ce type de fichier";
    }
    
// On remet l'extension à notre fichier
$nom = $nom + $extension_fichier;

// On renomme le fichier avec son extension et on l'enregistre
$var = move_uploaded_file($_FILES['icone']['tmp_name'], "destination/$nom");

// Variable qui enregistre les erreurs
$erreur = 0;

// On ouvre le fichier pour voir s'il contient des caractères louches
$handle = fopen($nom, 'r');
if ($handle)
{
    while (!feof($handle) AND $erreur == 0)
    {
        $buffer = fgets($handle);
        
        switch (true) {
        
        case strstr($buffer,'<'):
                $erreur += 1;
        break;
        
        case strstr($buffer,'>'):
                $erreur += 1;
        break;
        
        case strstr($buffer,';'):
                $erreur += 1;
        break;
        
        case strstr($buffer,'&'):
                $erreur += 1;
        break;
        
        case strstr($buffer,'?'):
                $erreur += 1;
        break;
        }
    }
    
    fclose($handle);
    
// Si on a trouvé des caractères suspescts, on supprime le fichier par sécurité
if ($erreur > 0) {
    
        if( file_exists ($nom))
        
        @unlink( $nom );
    
    }
    
    else 
        {
            echo "Le fichier a bien été sauvegardé";
        }
    
}

?>

 Je vais donc lister les points qui ne vont pas:

• L'indentation. Alors je sais bien que ce n'est pas ce qu'il y a de plus grave mais déjà quand un débutant fait un poste pour avoir de l'aide c'est la première chose qu'on attend afin d'avoir quelque chose de lisible et un cours est là pour donner l'exemple en montrant les bonnes pratiques..
   
• Le code en lui même. Ce script est censé être fait pour sécuriser un upload de fichier se trouvant dans le tableau $_FILES et si il y a bien un problème récurant chez les novices c'est de travailler sur des indices de tableau sans vérifier si ils sont définis, ainsi ce script plante dans le cas ou aucun fichier n'est envoyé... Pas très sérieux en terme d'exemple et de bonne pratique
   
• La connaissance du PHP. Ce script est écrit en php qui à des normes, et ainsi dans le cas ou un fichier est submit par un formulaire, le tableau $_FILES contient alors un indice file qui est lui même un tableau, or dans ce script l'indice file est remplacer par l'indice "fichier" ou même "icone" qui n'existent tout simplement pas.
   
• L'algorithme. Un script extrêmement mal pensé, si l'extension de fichier ne correspond pas, on affiche un message d'erreur mais on continue l'upload...
  
  
• le fonctionnement du script. Ce script ne fonctionne tout simplement pas et n'a donc même pas été testé....

Je n'ai pas vérifier le reste du cours "Protégez-vous efficacement contre les failles web" mais là déjà tombe des nues 

-
Edité par coolswing 29 juillet 2016 à 17:58:00

Bonjour coolswig,

Je suis l'auteur de ce cours et je serai ravi d'apporter des précisions à ce sujet.

Tout d'abord merci d'avoir pris le temps d'analyser ce qui ne va pas selon toi, et d'en avoir fait un rapport aussi détaillé ! J'avoue que après relecture il y a un bon nombre d'erreurs "impardonnables" qui se sont glissé dans le script et j'en suis sincèrement désolé.

• Pour ma défense, l'indentation n'est pas la mienne. Quand j'ai rédigé cette partie, l'éditeur de cours OC était... pas très au point. Au final, les scripts en arrières plan rajoutaient des espace/tabulation aléatoirement et c'était assez chaud de sortir un truc propre. J'ai fait du mieux que j'ai pu avec ce que j'avais. Mais si cela te choque à ce point, je tâcherai de corriger ça.
• Je n'ai rien à dire pour ma défense, je suis le premier à lutter pour systématiquement vérifier indices et variables.
• Pour cette remarque là, j'avoue que je n'ai pas bien compris ce qui m'étais reproché. (Voir indices)
• En effet, je ne sais même pas comment je me suis débrouillé pour faire un truc aussi mal foutu. Je vais corriger ça dès que possible, parce-que là ça pique réellement les yeux, je l'admet et j'en suis profondément désolé.
• Aux dernières nouvelles, le script fonctionnait. Mais quelques erreurs semblent s’être glissées dedans donc je vais en écrire un nouveau à partir de zéro. Comme ça, on pars sur de bonnes bases et ça évitera de proposer quelque chose de brouillon aux lecteurs.

Voilà voilà, je vais travailler là dessus dès que possible. Ce chapitre est le premier que j'ai écrit, et j'avoue qu'avec du recul j'en suis très déçu. Je vais me remettre un peu au travail pour proposer quelque chose de cohérent et - surtout - de correct ! Ces erreurs ne sont pas issues de l'incompétence, mais surtout de l'inattention.

Merci une fois de plus pour le retour, je vais faire ce que je peux pour corriger tout cela au plus vite. N'hésite pas à jeter un coup d’œil au reste et à me faire un retour sur tes impressions.

Merci de ta réponse,

Pour info c'est $_PATH['nameDeTonChamp'] donc a moins que tu n'ai 2 champs files différents l'indice ne change pas (je me suis mal exprimé) donc c'est soit $_FILES['fichier'] soit $_FILES['icone'].

Hello,

Petit message pour indiquer que les modifications on été acceptées par la modération et sont maintenant en ligne

Bonjour,

J'ai essaye d'appliquer le code donne du cours et j'ai eu des erreurs (variable $nom, $erreur inexistante, fopen qui marchait pas,la verification si le fichier existe etait toujours vrai) . Je l'ai donc remodele pour qu'il n'y ai plus d'erreurs.

Je suis encore debutant et je ne suis pas sure a 100% si ma version contre les upload malicieux est juste. Ce code ci-dessous ne fait apparaitre aucune erreur lorsqu'il est lance . Quelqu'un peut-il me confirmer si ce code est correct?

// Varibale d'erreur par soucis de lisibilité
// Evite d'imbriquer trop de if/else, on pourrait aisément s'en passer
$error = false;

// On définis nos constantes
$newName = bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM));
$path = "../upload";
$legalExtensions = array("jpg", "png", "gif", "jpeg");
$legalSize = "10000000" ;// 10000000 Octets = 10 MO

// On récupères les infos
$file = $_FILES['photoProfile'];
$fileName = $file['name'];
$actualName = $file['tmp_name'];
$actualSize = $file['size'];
$extension = strtolower(pathinfo($fileName, PATHINFO_EXTENSION));
echo $fileName.'-'.$actualName.'-'.$actualSize.'-'.$extension.'-';

//ouverture du fichier pour verifier caracteres dangereux
$handle = fopen($_FILES['photoProfile']['tmp_name'], 'r');

if ($handle) {


    while (!feof($handle) AND $error = false) {

        $buffer = fgets($handle);

        switch (true) {
            case strstr($buffer,'<'):
                $error = true;
                break;

            case strstr($buffer,'>'):
                $error = true;
                break;

            case strstr($buffer,';'):
                $error = true;
                break;

            case strstr($buffer,'&'):
                $error = true;
                break;

            case strstr($buffer,'?'):
                $error = true;
                break;
        }
    }
}
fclose($handle);

// On s'assure que le fichier n'est pas vide

if (strlen($newName) == 0 || $actualSize == 0) {
    $error = true;
    echo ' presnce du fichier vide - ';
}

// On vérifie qu'un fichier portant le même nom n'est pas présent sur le serveur
if (file_exists($path.'/'.$newName.'.'.$extension)) {
    $error = true;
    echo ' doouble nom - ';
}

// On effectue nos vérifications réglementaires
if (!$error) { echo 'pas d\'erreur - ';
if ($actualSize < $legalSize) {
    echo 'taille ok - ';
    if (in_array($extension, $legalExtensions)) {
        echo ' fichier bouger dans le dossier';
        move_uploaded_file($actualName, $path.'/'.$newName.'.'.$extension);
        }
    }
}

else {

    // On supprime le fichier du serveur
    @unlink($path.'/'.$newName.'.'.$extension);

    echo "Une erreur s'est produite";

};




?>

-
Edité par Silkflo 25 octobre 2017 à 16:09:21