search

IFAEDI / S.I

Mise en place d'un S.I avancé pour l'AEDI
3 stars 2 forks source link

Fix old style #42

Closed blackheaven closed 11 years ago

blackheaven commented 11 years ago

J'ai reprotégé les sorties au lieu des entrées, j'ai fait des tests basiques mais de vrais tests seraient préférable. Fix #13 #19 #24 voire #10

benjaminplanche commented 11 years ago

Hey,

Bonne journee, bye

blackheaven commented 11 years ago

En fait dans le web tu as deux grands type d'attaques :

Pour le serveur : nous n'avons que peut d'upload donc pour les droits ça à l'air bon (je n'ai pas vérifié) et les injections SQL et pour ça toutes les requêtes sont préparés.

Pour le client : en terme de CSRF on est pas couvert et pour le XSS il suffit de balancer un htmlspecialchars sur chaque sortie.

Pourquoi ne pas appliquer la protection du XSS sur l'entrée ?

d'une part ça altère les données et d'autre part un traitement effectué entre la récupération des données et l'affichage des données peut potentiellement annuler cette protection.

Pourquoi une nouvelle branche ?

D'importantes modifications, ainsi on pourra tester plus facilement (c'est le workflow que j'ai compris).

benjaminplanche commented 11 years ago

Flemme de rajouter les accents aujourd'hui, sorry de piquer les yeux.

bnjbvr commented 11 years ago
blackheaven commented 11 years ago

Quel serait l'intérêt ?

bnjbvr commented 11 years ago

Cela ne me semble pas très judicieux de ne pas appliquer les protections en entrée mais en sortie:

J'aimerais d'autres avis, notamment @PandiPanda69 ?

La fonction proposée permettait de simplifier la sécurisation des entrées de formulaire en invoquant simplement cette fonction directement sur _POST et _GET depuis le début de traitement des données reçues du formulaire, de manière unifiée (pas de recopie de code, etc.).

blackheaven commented 11 years ago

Je m'y attendais un peu.

Filtrer les entées de cette façon ne sert strictement à rien (d'autant plus que dans l'histoire de PHP des gens ont eu la même idée, on a appelé ça les magic_quotes, résultat c'est tout simplement déprécié depuis 10 ans), le but est de protéger les usages. En quoi protéger les caractères accentués peuvent protéger de quoi que ce soit ?

Pour ce qui est du temps CPU, ça ne pèse absolument pas sur le temps CPU, en fait tu propose un système de cache, seulement, dans un système de cache, on conserve les données sources, là tu les altère purement et simplement. De plus il n'y a pas de protection universelle, ça dépend de la sortie (cf. Zend\Escaper).

En tant qu'attaquant je ne m'embêterais à jouer sur ce point (sachant qu'il ne peux pas jouer là dessus au final) avec les grosses failles CSRF.

La sécurité consiste a mettre les protections nécessaires aux bons endroits.

bnjbvr commented 11 years ago

Je nourris juste la discussion, j'aime bien comprendre l'intérêt ou la raison qui motive un choix, voilà tout.Tant que tu peux justifier tous les choix techniques que tu proposes, tout va bien :+1:

Par contre, là où je ne te suis pas, c'est pourquoi parles-tu de caractères accentués? Cela n'a rien à voir avec les problèmes de sécurité évoqués précédemment, il n'a jamais été question de les échapper, etc. Par contre, ça soulève une question intéressante: est-ce que l'encodage des chaînes en BDD est bien en utf-8? (je pense que c'est le cas, sinon nos très nombreux utilisateurs se seraient probablement plaint de ne pas voir d'accents :-) )

Du coup, quelques remarques:

Voili voilou, je pense que l'on a tous vu la PR et commenté ce qu'on avait à dire ou pas, je merge et ferme. A tester en production au plus vite, pour voir si ça règle bien toutes les issues ouvertes!

Edit: si c'était possible de faire des noms de PR plus explicites aussi, ce serait cool ;-)

blackheaven commented 11 years ago

c'est en prod', ça à l'air de fonctionner, le je suis pour un merge

benjaminplanche commented 11 years ago

Annuaire, en voulant consulter n'importe quelle entreprise :

Warning: utf8_decode() expects parameter 1 to be string, array given in /data/www/ifaedi/html/commun/php/utils.inc.php on line 139 {"code":"ok","entreprise":""}

Et c'est le seul module testé pour l'instant. Des vérifs ont-elles vraiment été faites pour chacun avant la PR ?

... Bref, je ne vote pas pour le merge tout de suite.

(Et quand tout sera réglé, pourras-tu, @blackheaven, corriger en BDD les données corrompues ?)

blackheaven commented 11 years ago

J'ai oublié de répondre, désolé.

Pour le fait que tu ne me suive pas : c'est normal. Pour la BDD : ça ne change pas grand chose l'UTF-8 ou pas, au final il n'y a que très peu de cas où ça peut être pénible (si on utilisait InnoDB par exemple). Les MQ sont désactivés de base depuis PHP 5 je crois et on disparues en PHP 5.3. Pour le dépôt privé, ça peut être une bonne idée mais j'aimerais relancé le débat sur le FW avant/en même temps.

PandiPanda69 commented 11 years ago

Je prends le temps de répondre par rapport à la remarque de @BenjBouv.

J'avoue que c'est dommage de filtrer systématiquement en sortie et que mathématiquement, il y a un réel impact (on a 2 boucles imbriquées...). Pour une personne comme moi qui considère que les perfs sur un site comme l'AEDI ne pose pas de problème, c'est moyennement raisonnable.

Pour la sécurité, tu parles de XSS, ..., ce sont des failles de kikoolol. MySQL est réputé pour ses failles de buffer overflow (je vomis MySQL). Cadeau: https://isc.sans.edu/diary/Zero+Day+MySQL+Buffer+Overflow/14611 Le contrôle des données en entrée est tout de même préconisé à mon sens pour éviter des attaques critiques. (La sécurité, c'est aussi prévenir des attaques ZeroDay et faire des plans de continuité d'activité en cas d'attaque... Mettre les protections aux bons endroits, c'est une légende populaire, pour avoir participé au CTF, tu apprends à faire du blitzkrieg.)

Le deuxième mail de Boule n'est pas répondu en revanche, c'est dommage.

2013/2/14 blackheaven notifications@github.com

c'est un prod', ça à l'air de fonctionner, le je suis pour un merge

— Reply to this email directly or view it on GitHubhttps://github.com/IFAEDI/S.I/pull/42#issuecomment-13540164.

PandiPanda69 commented 11 years ago

Gautier, 14/02/2013 :

Pour le fait que tu ne me suive pas : c'est normal.

Je ne comprends pas ce que tu insinues. C'était une preuve de modestie?

Le 14 février 2013 11:10, blackheaven notifications@github.com a écrit :

J'ai oublié de répondre, désolé.

Pour le fait que tu ne me suive pas : c'est normal. Pour la BDD : ça ne change pas grand chose l'UTF-8 ou pas, au final il n'y a que très peu de cas où ça peut être pénible (si on utilisait InnoDB par exemple). Les MQ sont désactivés de base depuis PHP 5 je crois et on disparues en PHP 5.3. Pour le dépôt privé, ça peut être une bonne idée mais j'aimerais relancé le débat sur le FW avant/en même temps.

— Reply to this email directly or view it on GitHubhttps://github.com/IFAEDI/S.I/pull/42#issuecomment-13540785.

blackheaven commented 11 years ago

Je ne comprends pas ce que tu insinues. C'était une preuve de modestie?

Non, j'ai fais mon message en deux fois et j'ai oublié une partie de mon argumentaire.

blackheaven commented 11 years ago

@PandiPanda69 tu connais mon avis sur MySQL...

Le soucis du filtrage inutile à l'entrée c'est que ça altère les données et que comme on peut le voire dans Zend\Escaper la protection des données json et html (par exemple) n'est pas la même. Pour les failles 0-Day de MySQL... on peut toujours mettre en Base64 toutes les données insérées, mais ça n'apporterais pas grand chose.

@Aldream : oui, je m'occuperais des données :)

@BenjBouv à quel question j'ai oublié de répondre ? (désolé)

PS : oui pour le nom des PR : mais je n'ai pas énormément d'imagination :/