UPLOADER DES FICHIER DANSUN DOSSIER, SÉCURISÉ AVEC MOT DE PASSE
cod57
Messages postés
1653
Date d'inscription
dimanche 7 septembre 2008
Statut
Membre
Dernière intervention
11 septembre 2013
-
21 oct. 2011 à 15:30
cs_emilia123 Messages postés 122 Date d'inscription mercredi 19 décembre 2001 Statut Membre Dernière intervention 5 janvier 2009 - 24 oct. 2011 à 16:38
cs_emilia123 Messages postés 122 Date d'inscription mercredi 19 décembre 2001 Statut Membre Dernière intervention 5 janvier 2009 - 24 oct. 2011 à 16:38
Cette discussion concerne un article du site. Pour la consulter dans son contexte d'origine, cliquez sur le lien ci-dessous.
https://codes-sources.commentcamarche.net/source/53694-uploader-des-fichier-dansun-dossier-securise-avec-mot-de-passe
https://codes-sources.commentcamarche.net/source/53694-uploader-des-fichier-dansun-dossier-securise-avec-mot-de-passe
24 oct. 2011 à 16:38
J'avais compris que c'était uniquement le "isset" qui était inutile et donc à supprimer (vu que tout le reste du code utilise $login et $pass).
Si on ne supprimait que le isset en laissant l'assignation qui est en place, on aurait un warning pour css lignes si il n'y avait pas de login/pass à traiter.
Mais en effet si on supprime complètement l'assignation et qu'on remplace partout dans le code les variables d'origine, le "empty" suffit.
EM.
24 oct. 2011 à 15:24
"Inutile? si ca n'est pas fait et que le serveur est configuré pour afficher les warning, ca affichera un joli petit message d'avertissement... c'est pas top pour l'internaute."
Tu as tort. Juste après ceci, il test à nouveau ces variables à l'aide de empty(). Comme tu peux le constater dans la documentation, empty() considère une variable non déclarée comme vide et du coup n'affiche aucun avertissement, pas même une notice. Tu peux tester si tu doutes de ce comportement. Bref, vu que empty() remplis le rôle de isset() en plus de tester autre chose, il est parfaitement inutile de faire ce isset().
Un autre argument que j'ai contre cette affectation (même après test) est plus d'ordre sémantique. Le fait que la valeur se trouve dans la variable $_POST (ou $_GET) est porteur de sens : on sais que cette donnée est fournie par l'utilisateur. En utilisant une variable perso on perd cette signification et il faut se souvenir soi même de cette provenance. Vu qu'il ne faut *jamais* se fier à une donnée fournie par l'utilisateur, je trouve dangereux ce genre de choses qui peut faire oublier au développeur la potentielle dangerosité de que contient la variable.
24 oct. 2011 à 15:08
Je vais modifier le code pour qu'il soit plus correct.
24 oct. 2011 à 08:31
@TychoBrahe : pourquoi indiques tu :
---------------------------------------
$login = (isset($_POST["login"])) ? $_POST["login"] : "";
Useless au possible, à supprimer, et pareil pour son collègue.
---------------------------------------
Inutile? si ca n'est pas fait et que le serveur est configuré pour afficher les warning, ca affichera un joli petit message d'avertissement... c'est pas top pour l'internaute.
Comme le propose COD57, il serait intéressant de découper le script en plusieurs parties :
1) identification avec mise en session et redirection vers upload si OK,
2) upload si l'identification est OK
Concernant le script actuel, il y a un petit pb avec les "echo" d'erreur (ligne 18, 26, 38 et 42).
Dans l'état actuel, ils sont affichés avant le début du code HTML, ce qui n'est pas correct.
Il faudrait enregistrer le message dans une variable et l'afficher ensuite dans le formulaire.
Pense aussi à ne pas utiliser de javascript et d'"alert" pour ce genre de message. Un joli petit cadre au dessus du formulaire fait beaucoup plus propre (et pro) qu'une vilaine alerte javascript.
Bon courage pour la suite.
EM.
22 oct. 2011 à 10:39
@En lui même le code est pas trop mal pour un débutant (ya bien pire)...
@TychoBrahe d'accord avec toi en espérant qu'il poste un code plus sécurisé
tu devrais séparé le code, quelques idéees :
1/Un formulaireadmin.html saisir login + password <form action='traitement.php' method="post"> ...
2/tu vérifies dans traitement.php les identifiants
(depuis une base ou un fichier caché dans un répertoire protégé par .htaccess)
si ok tu crées une variable $_SESSION['client']=true
si true===$_SESSION['client'] tu affiches le formulaire d'upload
echo $form;
sinon retour à formulaireadmin.html par la fonction header()
header('Location: formulaireadmin.html');exit;
il faut aussi faire une fonction de déconnection est un bouton se déconnecter dans traitement.php (et ou) par un système de cookies une déconnection automatique au bout de x secondes
4/$form pointe vers upload.php
qui contient la function upload avec tout les trucs habituels de securité
en vrac + ceux que j'oublie
protection acces de $form sur upload par token ou captcha
nettoyage du 'name' par un un filtre (preg_replace)
verification du mine
verification de l'extension du fichier envoyé + tableau des extensions autorisées
verification du repertoire et du déplacement is_uploaded()
verification de l'existence du fichier (file_exists)dans le dit repertoire (contre les doublons)
deplacement vers une arborescence inferieur ...
exemple :
./titi/toto/traitement.php
./titi/toto/upload.php
tu deplaces les fichiers vers ./images/
...
stockage eventuelle dans mysql infos
date heure : membre : depuis ip : déplace fichier vers : x
si l'upload
$_POST=$_GET=$_FILES=array();
header('Location: merci.html');exit;
tu affiches les fichiers avec de l'url rewriting
et un bouton retour vers traitement.php
sinon
header('Location: byebye.php');exit;
byebye.php
<?php
session_start();
$_POST=$_GET=$_FILES=array();
unset_session();
destroy_session();
header('Location: http://monsite.fr/formulaireadmin.html');exit;
?>
++
22 oct. 2011 à 02:21
if ($login != "098f6bcd4621d373cade4e832627b4f6")
[...]
if ($pass != "098f6bcd4621d373cade4e832627b4f6")
Alors ça, jamais, jamais et jamais ! Il ne faut surtout pas utiliser les opérateurs == et != pour comparer deux chaînes de caractères, surtout dans un cas comme celui là. La raison ? Un cast qui peut être dévastateur et induit ici une faille de sécurité. Plus d'info sur la dernière partie de ce billet :
http://blog.uraniborg.net/post/Quelques-%C3%A9tranget%C3%A9s-de-PHP
22 oct. 2011 à 02:18
Quelques points qui me choquent un peu :
"Attention le mot de passe et le login doivent être chiffré en MD5."
Hachés, pas chiffrés. Mais au moins tu n'as pas dit crypté, c'est un bon début et mérite donc de l'indulgence. Quelques infos à ce sujet : Chiffrement, hachage et compagnie : disambiguation.
"FTP Service"
Désolé, mais tu fais du HTTP et non du FTP.
$login = (isset($_POST["login"])) ? $_POST["login"] : "";
Useless au possible, à supprimer, et pareil pour son collègue.
get_magic_quotes_gpc(), addslashes()
Idem, strictement aucun intérêt ici.
Sinon, la comparaison login/password en dur c'est vraiment pas cool m'enfin bon...
$dossier = 'chemin_du_dossier';
$fichier = basename($_FILES['fichier']['name']);
move_uploaded_file($_FILES['fichier']['tmp_name'], $dossier . $fichier)
/!\ ATTENTION /!\
Faille de sécurité monstrueuse ! Ya pas a dire, quand on ne connais pas bien tous les risques, il ne faut surtout jamais faire de script d'upload soi même, c'est quelque chose d'extrèmement sensible. Là avec ton truc j'upload n'importe quoi n'importe où, y compris des fichiers malicieux et dans des répertoires différentes du répertoire cible. Juste 3 conseils vite fait :
1. Ne jamais laisser l'utilisateur choisir le nom du fichier, tu dois le générer toi même (merci uniqid() par exemple). Ainsi on évite les fichiers dans les mauvais répertoires (../toto.lol), la collision de noms et autres trucs.
2. Ne jamais donner accès aux fichiers uploadés. Et oui, même si le nom est auto-généré, certains serveurs ont de jolies failles (certaines versions de nginx par exemple) qui permettent de quand même interpréter n'importe quoi comme du php ou autre. Si on veux publier les fichiers uploadés, il est donc obligatoire de compléter ce script avec un script permettant d'accéder aux fichiers (prudence également sur ce nouveau script, c'est également très sensible niveau sécurité).
3. Ce n'est pas le cas ici, mais toute tentative de vérification du type de fichier, que ce soit par extension ou type mime, est vouée à l'échec. Il est donc totalement inutile d'essayer, il est préférable d'admettre son impuissance et traiter tous les fichiers comme potentiellement malicieux.
Bref, conclusion finale :
En lui même le code est pas trop mal pour un débutant (ya bien pire), mais le sujet est bien trop complexe et demande une véritable attention. Du coup ton code est une faille de sécurité avec un potentiel destructeur monumental, ilne faut donc surtout pas l'utiliser. Amis débutants, ne faites jamais de scripts d'upload et ne prenez jamais le premier trouvé sur internet ! (ni le second d’ailleurs... ni aucun autre qui ne soit pas écrit par un pro et que vous ne savez pas parfaitement utiliser et adapter en fonction des cas).
21 oct. 2011 à 16:35
21 oct. 2011 à 15:30
le fait de devoir se connecter à chaque envoi ... tu devrais songer à faire évoluer ça. Tu affiches l'ip mais elle n'est pas 'enregistré'.
++