THELIA Forum

1) Ne jamais faire confiance aux couches supérieures;
2) Ne jamais laisser une entrée utilisateur passer dans la base de donnée sans validation ou échappemment;
3) Ne jamais faire de requête SQL sans échappement.

La classe Requete, celle qui enregistre tous les objects en base (oui oui, celle qui fait toute la magie de votre application, hors moteur), n'applique strictement aucun échappement des données incluses en base!!

Apparament, vous faites confiance à magic_quote_gpc(), c'est une grave erreur.
1) ça fait plusieurs années que tout le monde dit de ne pas faire confiance à cette fonction;
2) elle est pas forcément présente par défaut sur le serveur web qui héberge l'application;
3) il est même possible que le serveur web en question vous interdise les .htaccess.

Donc voilà, le patch de classes/Requete.class.php

--- Requete.class.php.orig	2008-12-22 12:22:50.000000000 +0100
+++ Requete.class.php	2008-12-23 16:30:55.000000000 +0100
@@ -72,7 +72,52 @@
 			
 			for($i=0; $i<count($this->bddvars); $i++){
 				$varn = $this->bddvars[$i];
-				$listv.= $this->bddvars[$i] . "=\"" . $this->$varn . "\", ";
+                                $non_escaped = $this->$varn;
+                                /*
+                                 * ESCAPE YOUR STRINGS !!!! ALWAYS !!!!
+                                 * (And try to indent correctly your code BTW).
+                                 *
+                                 * Old code was:
+                                 *   $listv.= $this->bddvars[$i] . "=\"" . $this->$varn . "\", ";
+                                 *
+                                 * Lesson of the day, escape your vars. Since we
+                                 * don't know which type to use, so let's try
+                                 * to guess it.
+                                 */
+                                if (is_bool($non_escaped)) {
+                                    $escaped_var = ($non_escaped ? 'TRUE' : FALSE);
+                                }
+                                if (($escaped_var = intval($non_escaped)) > 0) {
+                                    // Ok, keep the escaped var
+                                }
+                                else if (is_numeric($non_escaped)) {
+                                    // May be a float, just cast it
+                                    $escaped_var = floatval($non_escaped);
+                                }
+                                else {
+                                    // Treat everything else as a string
+                                    // For PHP >= 4.3
+                                    if (function_exists("mysql_real_escape_string")) {
+                                        $escaped_var = "'".mysql_real_escape_string($non_escaped)."'";
+                                    }
+                                    // For PHP < 4.3
+                                    // I think this is weak.. but none of moderns sites
+                                    // should have an older PHP version.
+                                    if (! get_magic_quotes_gpc()) {
+                                        $escaped_var = "'".addslashes($non_escaped)."'";
+                                    }
+                                    else {
+                                        // Your PHP is outdated, and you web server
+                                        // misconfigured! just prey.
+                                        $escaped_var = "'".$non_escaped."'";
+                                    }
+                                }
+				$listv.= $this->bddvars[$i] . "=" . $escaped_var . ", ";
+                                /*
+                                 * End of lesson.
+                                 */
 			}
 			
 			$query = "update $this->table set " . substr($listv, 0, strlen($listv)-2) . " where $varid=\"" . $this->$varid . "\"";

Je conseille à tout le monde, sans exception, qui utilise Thelia de passer ce patch, sinon vous risquez d'avoir vraiment de très mauvaises suprises.

Pour l'utiliser, copiez-collez le dans un fichier genre Requete.class.php-escape_sql_vars.patch
Copiez le fichier dans le répertoire /class/ du docroot.
puis tapez (sous unix)
% cd <path/to/docroot>/class
% patch -P0 < Requete.class.php-escape_sql_vars.patch

@yoan

Simple test, utilise TinyMCE pour inclure une image, et regarde si quand la page se recharge ton produit à bien été mis à jour.

Si tu as de la chance, ça va passer (sûrement grâce à magic_quote_jpc()). Si comme moi tu as une configuration apache d'administrateur système parano dans les mains, tu auras la surprise de voir que ton produit n'a pas été mis à jour, parce que les " dans <img src="/bla/bla" /> on fait péter la requête SQL.

De plus, ça ne change rien aux quelques concepts de base énoncés, il ne faut jamais faire confiance aux couches supérieurs, pas même aux plugins, ils sont souvent mis à contribution par des développeurs tiers et le code pas forcément reviewé.

EDIT: De plus, c'est quand même extrêment présompteux de penser que construire une requête SQL de la sorte, sans même aucun échappement est 100% sans risques. C'est du web, c'est ouvert à tout le monde.

Puis, un jour, c'est sûrement pas le cas aujourd'hui parce que preuve est que l'application tourne, vous aurez sans doute des problèmes avec les variables globales, ou l'émulation du register_globals que vous faites là:

admin/pre.php

	foreach ($_POST as $key => $value) $$key = $value;
	foreach ($_GET as $key => $value) $$key = $value;

Last edited by pounard (23-12-2008 17:10:33)

Je t'ai donné le jeu de test. Le coup de l'image avec TinyMCE à bel et bien fait planter mes requêtes.

Essaye le, sur différentes configuration, avec/sans magic_quotes_gpc().

Je participe, je poste tous les patches que je fais, suites à des erreurs que j'ai sur mon environnement particulier.

C'est une bonne façon de participer d'envoyer des patchs, non ?

J'ai été un peu aggressif sur ce coup là, je m'en excuse, mais perdre une demi journée de boulot parce qu'il n'y a pas d'échappement sur une requête SQL, ça fait quand même mal.

Je travaille avec des framework depuis maintenant pas mal de temps (ZendFramework et Drupal), et je peux affirmer qu'aucun d'entre eux, dans leur gestion du SGBD, ne laissent jamais l'utilisateur faire un $query = "update toto set truc = '$mavar'"; simple règle de base.

EDIT: au niveau technologie, cette application à quelques années de retard. Ça n'impacte pas le fait que ce soit bien, ou mal codé, ça impacte juste le fait qu'aujourd'hui un sacré paquet de framework (et même le coeur de PHP) propose une série de méthodes et fonctions pour ne pas avoir à faire ce genre de choses à la main, laissant le développeur à l'abris d'un oubli d'échappement.

J'ai en plus remarqué que le code est parfois dupliqué (copié/coller), notamment l'upload d'image dans photo_produit.php, photo_rubrique.php, ou photo_dossier.php par exemple.
Du coup, j'ai fait un patch un peu bancal dans le rush pour les produits, à cause de problèmes d'encodages de noms de fichiers, mais je vais être obligé d'en faire un copier coller dans plusieurs fichiers si je veux le repercuter ailleurs, c'est un peu dommage. L'object (et même le procédural par ailleurs) donnent de nombreux moyens pour éviter de faire ce genre de choses.

Cette application part d'un très bon principe, je pense juste que son coeur doit subir un certain dépoussiérage, ça ne sera que bénéfique.

Last edited by pounard (23-12-2008 17:18:59)

Avis aux utilisateurs de THELIA. Je ne tiens pas à supprimer ce post car il fait parti de la vie du projet. Certes un peu agressif au départ, il met l'accent sur un problème à corriger.

Je tiens à détailler le problème pour les non techniciens afin d'éviter une paranoia excessive. Vous avez un risque si votre php.ini contient un magic_quotes_gpc à Off

Toutes les installations debian classique par ex ont un magic_quotes_gpc à On comme la plupart des distributions. Vous avez donc peu de risques d'être soumis à cette faille.

Cependant je vous invite simplement à vérifier cela et à changer la configuration de votre serveur si ce n'est pas le cas.

Nous n'allons bien sûr pas laisser ça en plan et le moteur va être très rapidement adapté pour corriger le soucis quelque soit la configuration.

Merci Pounard d'avoir mis l'accent sur le soucis.

Tu n'as pas tord là dessus non plus.

Lorsque nous avons démarré le projet PHP5 était encore assez rare. Il est clair qu'aujourd'hui nous aurions démarré différemment ... nous allons tendre vers du 5 voir 6 bientôt au fur et à mesure.

Une dernière note: vous avez la chance d'utiliser un système de templates, qui peut rendre le code facile à refondre de bout en bout sans pour autant péter l'API proposé au développeur, donc faut pas hésiter, si vous cassez tout les seules choses à porter seront les modules, et y'en a pas tant que ça de ce que j'ai pu voir.

Non là c'est totalement autre chose.