Forum d'entraide PHPFrance

Bonjour à tous, je viens juste d'arriver sur ce site où l'ambiance à l'air sympathique et où j'espere trouver ma tite place.

Depuis quelques temps je me suis remis à coder en php et j'aimerai avoir votre avis sur quelques bouts de code.

Je travail sur un projet perso où j'utilise pas mal de include() avec des variables recuperé par $_GET (aïe!), je sais que cela peut très facilement ammener de grosse failles de sécurité (voilà le pourquoi de mon post).

En gros cela donne :

#fichier index.php
<?php
//$page est de la forme "fichier" sans extension
$page = $_GET['page'];

if(!file_exists("rep_pages/$page.php")) {
//Si le fichier n'existe pas on redirige vers error.php
   $page = "error";
}

//appel de la page demandée
include("rep_pages/$page.php");
?>

Voilà, je voudrais savoir si la verification est suffisante ou s'il y a un moyen de contourner pour executer un shell via mon script.
Merci pour les réponses ou les commentaires.

Je ne suis pas un crack en php, loin s'en faut, ceci dit je pense qu'il vaudrait mieux que tu utilises la méthode POST, même si ça ne garantie pas une sécurité parfaite...

... Mais sans doute est-il préférable d'attendre l'avis d'experts en la matière plutôt que prendre le mien en considération car, je le répète, j'en suis aussi à mes balbutiements dans ce domaine.

Cordialement.

Dans la mesure où le test vérifie que c'est bien un fichier existant, ça ne pose à priori pas de problème. Je suggère un tout petit ajustement en mettant :

if(!file_exists("./rep_pages/". $page .".php)) {

au lieu de

if(!file_exists("rep_pages/$page.php")) {

Ça ne modifiera pratiquement rien si ce n'est qu'on est sûr que la recherche se fait bien sur un chemin relatif à partir de la page courante et la concaténation propre améliore les performances de l'interpréteur si on l'emploie systématiquement.

À mon avis, il manque une vérification cruciale sur le contenu de $page. Si le nom des pages n'est normalement composé que de lettres et chiffres (et le symbole _ ) alors il faut le vérifier avec une expression régulière. Par exemple:

<?php

if (!preg_match('#^[a-z_0-9]+$#', $page))
{
   die('Page invalide');
}
?>

Et ce afin d'empêcher que quelqu'un aille fouiller dans des répertoires où il ne devrait pas avoir accès en ajoutant ../ dans le chemin de $page. De plus, je pense qu'il doit être possible de provoquer une boucle infinie si $page='../index' parce que ./rep_pages/../index.php est équivalent à ./index.php et donc l'index se re-inclurait à l'inifini.

oki merci pour les reponses, je me doutais bien qu'il pouvait y avoir moyen de detourner le script et j'en ai maintenant la certitudes grace à vos reponses.

@Kaoteknik, malheureusement c'est une contrainte technique que d'utiliser $_GET, cela permet de savoir où l'on se trouve sur le site.

@Cyrano, merci ça me plait

@Hubert, Je vais aussi rajouter cette verification mais avec quelques modifs. Le script est un peu plus compliqué en réalité,

je vais chercher les fichiers dans d'autres sous-repertoires, "/rep/file1/file1.php" ou même "/rep/file1/file1.autre.php"

Il y a donc des "." (je trouvais ça joli), ceci dit je pourrais très bien modifier et les remplacer par "_".
Je pensais aussi à remplacer l'url genre "http://mydomain.com/mapage/auteur/date" , quoique je verrais ça plus tard et quand j'aurais compris le principe.

Pour éviter toute boucle il suffirait de mettre include_once(), non?
Mais peut être un poil plus lourd pour php...

Et bien voilà mon script au final :

#index.php
<?php
function inc_page($page){

  //le nom de repertoire se trouve dans la variable
  list($dir) = explode("_",$page);

  //on verifie que le fichier existe ou qu'il n'y ait pas de tentative de boucle
  if(!file_exists("./rep/".$dir."/".$page.".php") && !preg_match('#^[a-z_0-9]+$#', $page)){

    //sinon on lance ./rep/error/error.php
    $page = $dir = "error";
   }

  include("./rep/".$dir."/".$page.".php");
}

inc_page($_GET['page']);
?>

Pour appeller le script qui se trouve dans rep/article/article_edit.php : on appel index.php?page=article_edit

Pour rep/article/article.php :: index.php?page=article

Merci pour toutes les infos .

Pour éviter toute boucle il suffirait de mettre include_once(), non?
Mais peut être un poil plus lourd pour php...

Oui, ça marcherait bien sûr. Les "_once" (include_once, require_once) demandent un tout petit peu plus d'effort à PHP, mais c'est très très marginal.

Peut-être que la solution la plus simple et la plus fiable serait de créer un tableau des pages accessibles ?

$pages = array(
  'lire' => './rep/article/article.php',
  'editer' => './rep/article/article_edit.php'
);

if (!isset($pages[$_GET['page']]))
{
   die('Page inexistante');
}

include($pages[$_GET['page']]);

Cela donne la possibilité d'executer n'importe quel script php y compris ceux qui serait hors de ta racine web. Si la personne connait la liste des fichiers ou trouve un moyen de l'obtenir ou trouve en devinant , il lui suffit de donner un chemin relatif vers ce fichier

http://tonsite/index.php?page=../../etc/path vers fichier php


ce qui fera un include de ./rep_page/../../etc/path vers fichier php


Exemple. Ton site dispose d'un forom X open source ou autre, il suffit donc de faire quelques essai pour trouver un fichier sensible disponible dans la distrib et qui permet de reset les passwd admin, d'accéder a l'admin, de clear la BDD etc...

ex. expose_php est a on, on peut donc voir des infos sur ta distrib, et fonction de ca, il existe peut etre des fichier sensible dispo dont on connait le nom et l'emplacement.

Bref, c'est plutot a éviter AMHA.

En effet c'est pas mal d'ailleurs ça irait très bien dans l'optique de pouvoir "désactiver" des scripts sans pour autant supprimer les fichiers.

ty.

.........t la concaténation propre améliore les performances de l'interpréteur si on l'emploie systématiquement.

C'est une idée que tu te fais ou tu as des arguments pour justifier cette affirmation ?