Projet

Général

Profil

Evolution #3926

Remplacement de safehtml par le plug htmlpurifier ou autre

Ajouté par Franck D il y a presque 2 ans. Mis à jour il y a 4 jours.

Statut:
Nouveau
Priorité:
Normal
Assigné à:
-
Catégorie:
-
Version cible:
Début:
26/03/2017
Echéance:
% réalisé:

0%

Resolution:

Description

Bonjour :-)
Je fais un ticket, sinon, il y aura oubli, la lib safehtml n'a plus de mise à jour dispo et n'est même plus trouvable, si je suis le lien qu'il y a https://zone.spip.org/trac/spip-zone/browser/_core_/plugins/safehtml/lib/safehtml/classes/safehtml.php#L14
Sur la zone, il y a un plug qui semble pouvoir résoudre le problème https://zone.spip.org/trac/spip-zone/browser/_plugins_/htmlpurifier après, je ne sais pas si il y a une autre lib qui serait mieux, mais en tout cas, il y a déjà une solution.
Je suppose que cela représente trop de boulot pour la 3.2, surtout que maintenant la version alpha est dispo, mais c'est peut-être possible pour la version 3.3 ?
Franck

spip-3.2.diff Voir - Patch pour SPIP 3.2 (1,44 ko) Guillaume Fahrner, 26/08/2018 08:41

textwheel.diff Voir - Patch pour textwheel (2,73 ko) Guillaume Fahrner, 26/08/2018 08:41

Historique

#1 Mis à jour par Guillaume Fahrner il y a 6 mois

Je me permet de rebondir sur ce ticket pour lancer la discussion, expliquer le travail déjà réalisé de mon coté et les problèmes rencontrés :

un constat : on rencontre souvent des contenus HTML devant passer dans safehtml() qui ne respecte pas à la lettre la spécification, quelques exemples :

Ces morceaux de code HTML seront systématiquement normalisé par htmlPurifier ; ce qui est une bonne chose.

Cela pose un problème d'incompatibilité avec le fonctionnement de la fonction echapper_html_suspect() https://core.spip.net/projects/spip/repository/entry/spip/ecrire/inc/texte_mini.php#L471 :

if (strlen(safehtml($texte)) !== strlen($texte)) {

Tous les contenus modifiés car normalisés se retrouveront donc encodés puis affichés :(

Plusieurs solutions :
  • changer la logique de la fonction echapper_html_suspect() pour faire confiance à safehtml()@htmlPurifier
  • normaliser tout ce qui est susceptible de passer à travers cette fonction pour éviter ces modifications et conserver cette logique

#2 Mis à jour par Guillaume Fahrner il y a 6 mois

Tant que j'y pense, un autre "problème" :

safehtml()@htmlPurifier transforme les entités HTML dans leur correspondance UTF8 (ce que safehtml() actuel ne fait pas), on se retrouve encore dans le cas avec des longeurs de chaines différentes et donc du texte HTML encodé puis affiché.

#3 Mis à jour par RealET 🔸 il y a 6 mois

Et on a donc une longueur différente dès qu'on a une apostrophe ' transformée en ’
Et c'est ici que ça se passe : https://core.spip.net/projects/spip/repository/entry/spip/ecrire/typographie/fr.php#L27

#4 Mis à jour par Guillaume Fahrner il y a 6 mois

Après avoir passé pas mal de temps à chercher une solution sécurisée ET fonctionnelle, voilà ce à quoi je suis arrivé sans tout casser ou devoir normaliser "100ans d'historique". La version de SPIP utilisé est SPIP 3.2.1 [23954]. En court : j'inverse la logique actuelle et je fais confiance à la sortie de safehtml du plugin Purifier :

j'ai modifié la fonction echapper_html_suspect() de inc/texte_mini.php, la fonction echappe_anti_xss() du plugin textwheel et 2 règles YAML et... c'est tout :

inc/texte_mini.php :

function echapper_html_suspect($texte, $strict=true) {
    if (!$texte
        or strpos($texte, '<') === false or strpos($texte, '=') === false) {
        return $texte;
    }
    // quand c'est du texte qui passe par propre on est plus coulant tant qu'il y a pas d'attribut du type onxxx=
    // car sinon on declenche sur les modeles ou ressources
    if (!$strict and
      (strpos($texte,'on') === false or !preg_match(",<\w+.*\bon\w+\s*=,UimsS", $texte))
      ){
        return $texte;
    }

    $safed_texte = safehtml($texte);
    if (strlen($safed_texte) !== strlen($texte)) {
        $texte = $safed_texte;
    }
    return $texte;
}

plugins-dist/textwheel/wheels/spip/echappe-js.php :

function echappe_anti_xss($match) {
    static $safehtml;
    if (!is_array($match) or !strlen($match[0])) {
        return "";
    }
    $texte = &$match[0];

    if ( 
    (strpos($texte, ":") !== false and preg_match(",(data|script)\s*:,iS", $texte) ) or 
        (stripos($texte, "on") !== false and preg_match(",\bon\w+\s*=,i", $texte) )
    ) {
        if (!isset($safehtml)) {
            $safehtml = charger_fonction('safehtml', 'inc', true);
        }
        $texte = $safehtml($texte);        
    }
    return $texte;
}

plugins-dist/textwheel/wheels/spip/echappe-js.yaml :

-
  if_str: "<script" 
  match: "{<script.*?($|</script.)}isS" 
  is_wheel: y
  replace:
    -
      type: all
      replace: htmlspecialchars
      is_callback: Y
    -
      type: all
      replace: nl2br
      is_callback: Y
    -
      type: all
      replace: "&lt;code class=\"echappe-js\"&gt;$0&lt;/code&gt;" 

-
  if_str: "&lt;" 
  match: "{&lt;[a-z]+.*?($|&gt;)}UisS" 
  is_callback: Y
  replace: echappe_anti_xss

plugins-dist/textwheel/wheels/spip/interdire-scripts.yaml :

securite-js:
  if_str: "<" 
  if_match: "/<[a-z]+/iS" 
  type: all
  replace: "echappe_js" 
  is_callback: Y

Pour le moment, du coté des effets de bord/choses cassées (nos fonctions sont remplis de spip_log() et on affiche le code impacté par safehtml() pour identifier rapidement les régressions) :
  • le changement de statut via le survol des puces (onmouseover inline) n'existe plus car l'attribut onmouseover est supprimé par htmlPurifier (rien de compliqué a fixer à mon sens, mieux cela oblige à développer (très) proprement, jme propose pour le patch si besoin)

Sinon pas de problème, on publie/modifie/supprime nos objets éditoriaux comme d'habitude, leur mise en forme n'a pas bougé, on install/update/supprime nos plugins, stats OK, config OK, etc, etc (cela fait déjà plusieurs jours que nous utilisons ces modifications).

Coté performance, la machine virtuelle est suivi via SNMP/librenms et on ne voit aucune différence d'utilisation CPU (merci le cache SPIP et OPcache) entre avant/après. D'autre part, htmlPurifier n'est pas appelé tout le temps, loin de là, on ne passe que "rarement" les conditions dans echappe_anti_xss().

Coté sécurité :

Coté publication :
Cela me semble risqué d'attendre une version majeure de SPIP avant de corriger ces problèmes de sécurité ; surtout que la transition peut se faire doucement en 3.2 avec une nouvelle version de textwheel et le remplacement (enfin) de plugin-dist/safehtml par htmlpurifier.

En espérant que ça aide et vous serve de base pour la suite.

g0uZ

#5 Mis à jour par Guillaume Fahrner il y a 6 mois

Content de voir que ce patch règle également des problèmes de mise en forme (je n'observe pas ce pb @home), je m'explique :

Dans mon post précédent, dans le contenu du fichier plugins-dist/textwheel/wheels/spip/echappe-js.yaml donné entre les balises

< pre>< code class="yaml">< / code>< / pre>

certains caractères "<" et ">" ont été encodé (mais pas d'autres (!) ) alors qu'à l'origine cette partie de mon post n'en contient aucun.

NB : essayez de mettre en forme avec <pre><code class="html"> la simple ligne de HTML donné dans ce post est ici impossible (les < > sont encodé alors que dans un < code >), une nouvelle fois je n'ai pas ce pb @home

#6 Mis à jour par b b il y a 6 mois

Merci g0uz, poste plutôt des patch au format diff, cela sera bien plus pratique à lire et à intégrer ;)

#7 Mis à jour par cam.lafit - il y a 6 mois

Salut

Tu peux aussi tester un PR sur git.spip.net/spip ou github

#8 Mis à jour par Guillaume Fahrner il y a 6 mois

Bonjour à tous,

ci-joint les patchs permettant de faire la bascule et d'avoir un mode parano fonctionnel, (il faut bien entendu activer le plugin HTMLPurifier) en attendant de l'avoir par défaut dans plugins-dist/safehtml/.

D'avance merci pour vos retours !

#9 Mis à jour par cam.lafit - il y a 6 mois

Bonjour

Merci pour le patch ::)

#10 Mis à jour par cedric - il y a 5 mois

  • Version cible changé de 3.3 à 99 plus tard

On a mise à jour SafeHTML c'est reparti pour 10 ans :)

#11 Mis à jour par Guillaume Fahrner il y a 2 mois

https://zone.spip.net/trac/spip-zone/browser/spip-zone/_plugins_/htmlpurifier

La version du trunk est à mon sens bonne pour être testée à partir de SPIP 3.2 (on l'utilise désormais depuis un bon moment sans problème hormis le onmouveover sur les puces de statuts qui est supprimé).

#12 Mis à jour par cedric - il y a 2 mois

Depuis https://core.spip.net/projects/spip/repository/revisions/24131 le plugin HTMLPurifier est fonctionel sans nécessiter de patch dans le core ?
on est bon et on peut release tel quel en indiquant que le plugin htmlpurifier est disponible pour tests et se laisser le temps ?

Je pense que si on doit intégrer le plugin ça sera sur la version dev 3.3, pas sur la version stable, et ça va demander de prendre le temps de voir toutes les modifs et les impacts éventuels que tu n'aurais pas vu ou auxquelles tu n'aurais pas pensé.

Sur la fonction de survol des logos le sujet reste ouvert ? On est en effet d'accord que cette écriture est obsolète et devrait être revue de toute façon, mais c'est donc un point à traiter dans le core le cas échéant

#13 Mis à jour par Guillaume Fahrner il y a environ un mois

Sorry je n'avais pas vu ce message :

cedric - a écrit :

Depuis https://core.spip.net/projects/spip/repository/revisions/24131 le plugin HTMLPurifier est fonctionel sans nécessiter de patch dans le core ?

Malheureusement si : il reste une surcharge de la fonction propre() via inc/texte.php. Elle est utilisée pour réaliser la protection HTML via echappe_html() AVANT le 1er filtrage de sécurité via echapper_html_suspect(). Un deuxième est réalisé via echappe_retour_modeles($t, $interdire_script) ou $interdire_script est vrai en espacé privé ou si le mode parano actif et faux sinon. (Je n'ai pas su faire autrement, mais il y a peut être une astuce a trouver.)

Je ne sais pas si tu considères que c'est le core mais une surchage des règles YAML de textwheel est également réalisé afin que l'on envoi bien tout qui doit l'être dans les filtres de sécurité.

Donc non pas de modification du core nécessaire pour que le plugin fonctionne. A voir si ces surcharges mériterais une "intégration directe" future.

on est bon et on peut release tel quel en indiquant que le plugin htmlpurifier est disponible pour tests et se laisser le temps ?

A mon avis oui. Pour rappel : le mode parano est actuellement inutilisable avec l'actuel safehtml(). Du coup ça peut être une approche pour l'intégrer/le tester au fil de l'eau : commencé par les gens qui utilisent le mode parano en leur recommandant chaudement/forcant ce plugin.

Ci dessous la liste des plugins que j'utilise sans problème hors textwheel/todo (cf https://core.spip.net/issues/4254) avec htmlPurifier :

Abonnements 3.3.6 - stable
Agenda 3.26.0 - stable
API de vérification 1.8.1 - stable
API Prix 0.1.16 - dev
Article PDF 1.0.10 - stable
Autorité 0.10.20 - stable
Banque&paiement 3.6.7 - stable
Cache Cool 0.5.4 - stable
Challenge Hacking 1.0.0 - stable
Champs Extras 3.11.7 - stable
Chatbox 1.0.5 - stable
Coloration Code 99 - stable
Commandes 1.15.5 - stable
Compositions 3.7.3 - stable
Crayons 1.26.18 - stable
CTF all the day 1.0.0 - stable
Facteur 3.6.2 - stable
Formulaire de contact avancé 0.16.5 - stable
Frimousses 1.5.1 - stable
GIS 4.44.24 - stable
HTML Purifier 5.0.0.1 - test
iCalendar 0.4.5 - test
Import ics 4.4.3 - stable
Imprimer document 0.2.2 - stable
LangOnet 1.4.6 - stable
MailShot 1.27.3 - stable
MailSubscribers 2.11.3 - stable
Markdown 1.0.2 - test
Messagerie 3.1.8 - test
Mini Calendrier 2.4.1 - stable
Mot de passe dès l’inscription 1.0.19 - stable
Newsletters 1.6.0 - stable
Nombres de visiteurs connectés 0.2.3 - stable
NoSPAM 1.5.18 - stable
Notation 2.4.2 - test
Notifications 3.6.8 - stable
Notifications avancées 0.4.2 - test
Pages 1.3.7 - stable
Pre & Code 99 - test
Saisies pour formulaires 3.11.1 - stable
Social tags 1.1.1 - stable
SPIP Bonux 3.4.6 - stable
Tip A Friend 1.6.9 - stable
Todo 2.2.1 - stable
Traduction entre rubriques 3.1.3 - stable
YAML 1.5.4 - stable

Je pense que si on doit intégrer le plugin ça sera sur la version dev 3.3, pas sur la version stable, et ça va demander de prendre le temps de voir toutes les modifs et les impacts éventuels que tu n'aurais pas vu ou auxquelles tu n'aurais pas pensé.

Clairement je pense me faire insulter :-) Je suis prêt ^^ Plus sérieusement il faut absolument tester/restester/reretester. J'ai tenté de faire au mieux mais je ne peux pas revoir tout le code HTML généré par les plugins... Au moindre écart de conformité HTML ca ne passera pas. Le bug https://core.spip.net/issues/4254 en est un bon exemple : avec htmlpurifier l'élément <table> est supprimé ; avec "safehtml() standard" le code invalide est conservé et c'est le navigateur qui va corriger.

Il faut voir les bons cotés: cela force les bonnes pratiques, met SPIP au plus haut niveau du standard (Drupal fait moins bien), aide a identifier des bugs et ce qui passe dans propre() sera obligatoirement valide. C'est aussi a terme moins de problèmes d'injection XSS a traiter pour la team.

Sur la fonction de survol des logos le sujet reste ouvert ? On est en effet d'accord que cette écriture est obsolète et devrait être revue de toute façon, mais c'est donc un point à traiter dans le core le cas échéant

Euh je comprend pas, faut-il intégrer le patch des puces de statut dans ce plugin ? Je pensais plus coder le truc """proprement""" avec un JS dans le <head> directement dans /prive/. A décolérer complètement à mon sens.

Encore merci pour le temps que tu y passes :)

#14 Mis à jour par b b il y a 4 jours

C'est aussi a terme moins de problèmes d'injection XSS a traiter pour la team

\o/ ho oui ça ferait du bien :)

Formats disponibles : Atom PDF