que pensez vous de cette extrait de code ?

que pensez vous de cette extrait de code ? - PHP - Programmation

Marsh Posté le 22-05-2014 à 11:15:42    

Bonjour,
 
   Je suis en train d'auditer le code d'un de mes clients, et je voudrais votre sur celui là :
 

Code :
  1. $path_sauvgarde="/var/www/files/mgc/images_mgc/";
  2. $tab_form = $this->getRequest()->getParams();
  3. $commande_supp="rm -rf ".$path_sauvgarde.$tab_form['code_visuel'];
  4. exec($commande_supp);


Merci :-)


---------------
Du tofu en Alsace : www.tofuhong.com
Reply

Marsh Posté le 22-05-2014 à 11:15:42   

Reply

Marsh Posté le 22-05-2014 à 12:42:33    

Pff, encore un code de stagiaire débutant ! [:haha petain]  
 
- Bien sûr qu'il faut mettre un espace avant et après le signe égal. C'est une règle élémentaire de typographie. Mais le débutant, lui, il s'en moque. Il écrit n'importe quoi, mettant des espaces sur une ligne et n'en mettant pas sur deux autres, rendant la présentation du code incohérente, donc peu lisible, et donc difficilement maintenable.
 
Après, tout suit dans la même veine.
 
- Le code n'a pas de ligne de commentaires.
 
- Le chemin est écrit en absolu au lieu d'être relatif. Cela rend difficile l'implémentation du code dans différents environnements (de test, de recette, de secours, etc.)
 
- La suppression se fait sans avertissement, sans trace de ce qui est supprimé.
 
- Il n'y a pas de contrôle du paramètre $tab_form['code_visuel']. Il pourrait contenir des jokers et d'autres caractères entraînant des suppressions non désirables.

Reply

Marsh Posté le 22-05-2014 à 14:21:59    

Utiliser exec est toujours un peu risqué d'autant que je doute que ça fonctionne sur tous les OS où PHP est disponible :/
J'aurais plutôt utilisé la fonction php unlink(), avec, avant, la récupération des fichiers qu'ont veut supprimer. Le code sera peut-être 0.001s plus long à s'exécuter, mais ça sera plus portable. :D
 
Et même remarque sur le contenu de $tab_form['code_visuel']. Je voudrais bien savoir comment il prend sa valeur. Parce que si c'est pas bien fait, ça introduit une faille de sécurité :/
 
Pour $path_sauvgarde, ce qui me gêne, c'est pas que ça soit un chemin absolu, mais que :
1) son contenu ne provienne pas d'une variable de conf
2) que son contenu ne soit pas généré dynamiquement (ie construit à partir de l'emplacement de stockage du script php).
 
Pour le commentaire, le bout de code étant très petit, le contexte était peut-être indiqué un peu plus haut. Qq'un qui connait la commande rm -rf comprend ce qui va se passer...


---------------
Astres, outil de help-desk GPL : http://sourceforge.net/projects/astres, ICARE, gestion de conf : http://sourceforge.net/projects/icare, Outil Planeta Calandreta : https://framalibre.org/content/planeta-calandreta
Reply

Marsh Posté le 22-05-2014 à 18:48:13    

rufo a écrit :

Qq'un qui connait la commande rm -rf comprend ce qui va se passer...


Ouai voila quoi, meme sans connaitre PHP c'est impensable de laisser un truc pareil. Tu soumets un / et boum c'est fini, aucune barriere.
 
Edit: j'avais pas lu le reste du code, donc je rectifie: tu soumets un ../../../../../ (et p'etre un flag pour le faire sans confirmation) et boum c'est fini.


Message édité par lasnoufle le 22-05-2014 à 18:58:08

---------------
C'était vraiment très intéressant.
Reply

Marsh Posté le 22-05-2014 à 18:58:35    

Si c'était du perl, je mettrais $tab_form['code_visuel'] dans une variable $imfile et je vérifierais que ce n'est pas une chaine vide et que c'est un nom sans risque (pas de sous chaine "../" dedans par exemple), et j'écrirais plutôt $commande_supp = 'rm -rf ' . $path_sauvgarde . '"' . $imfile . '"'; au cas ou le nom de fichier $imfile contiendrait des espaces.
 
A+,


Message édité par gilou le 22-05-2014 à 19:00:25

---------------
There's more than what can be linked! --    Iyashikei Anime Forever!    --  AngularJS c'est un framework d'engulé!  --
Reply

Marsh Posté le 23-05-2014 à 09:52:18    

En PHP, tu peux tout à fait faire les mêmes vérifs ;)


---------------
Astres, outil de help-desk GPL : http://sourceforge.net/projects/astres, ICARE, gestion de conf : http://sourceforge.net/projects/icare, Outil Planeta Calandreta : https://framalibre.org/content/planeta-calandreta
Reply

Sujets relatifs:

Leave a Replay

Make sure you enter the(*)required information where indicate.HTML code is not allowed