Refactoring de la mise en sécurité des textes dans ecrire et public #5271

Merged
marcimat merged 15 commits from refactor_texte_safety into master 7 months ago
cerdic commented 8 months ago
Owner

suite à spip-contrib-extensions/htmlpurifier#2 et à la relecture de #3926 j'ai donc entrepris un refactoring des appels à echapper_html_suspect() pour

  • être plus robuste sur la detection
  • ne pas se fourvoyer à cause d'un modèle
  • faciliter la transition vers une autre librairie de purification
  • mieux sécuriser les contenus
  • fixer le cas parano avec la globale filtrer_javascript=-1 qui était un peu cassé vu qu'on affichait du contenu échappé dans le public, ce qui est pas très user friendly

Au passage j'ai remarqué que traiter_modeles() était totalement inutilement forkée dans textwheel ce qui simplifie pas la maintenance. Je l'ai donc déplacée dans un inc/modeles que j'ai refactoré ensuite pour séparer la detection des modeles de leur traitement, et que j'ai complété par 2 fonctions rapides d'echappement/reinjection pour permettre de passer safehtml sur du contenu sans casser les modeles.

Bref tout ça est un peu plus propre et ça à l'air pas plus mal selon mes tests

suite à https://git.spip.net/spip-contrib-extensions/htmlpurifier/issues/2 et à la relecture de #3926 j'ai donc entrepris un refactoring des appels à `echapper_html_suspect()` pour - être plus robuste sur la detection - ne pas se fourvoyer à cause d'un modèle - faciliter la transition vers une autre librairie de purification - mieux sécuriser les contenus - fixer le cas parano avec la globale `filtrer_javascript=-1` qui était un peu cassé vu qu'on affichait du contenu échappé dans le public, ce qui est pas très user friendly Au passage j'ai remarqué que `traiter_modeles()` était totalement inutilement forkée dans textwheel ce qui simplifie pas la maintenance. Je l'ai donc déplacée dans un `inc/modeles` que j'ai refactoré ensuite pour séparer la detection des modeles de leur traitement, et que j'ai complété par 2 fonctions rapides d'echappement/reinjection pour permettre de passer safehtml sur du contenu sans casser les modeles. Bref tout ça est un peu plus propre et ça à l'air pas plus mal selon mes tests
cerdic added 9 commits 8 months ago
b4252f1f69 feat: la fonction echappe_html() s'enrichit d'un argument callback_options qui sera transmis aux callback d'echappement eventuelle
35b22fbf37 feat: la fonction echapper_html_suspect() change de signature pour prendre un tableau d'option en second argument ainsi que connect et env tels que reçus par propre() ou typo()
Poster
Owner

Cette PR va avec spip/textwheel#4844 (il faut tester les 2 ensemble)

Cette PR va avec https://git.spip.net/spip/textwheel/pulls/4844 (il faut tester les 2 ensemble)
cerdic added 1 commit 8 months ago
c512ab8315 fix: une fonction is_html_safe() personalisable pour tester si une chaine est safe plutot qu'une simple comparaison de longeur avant/apres
Poster
Owner

et donc la 3ème PR sur SafeHtlm pour passer à Purifier complète le tout
spip/safehtml#4781

(mais cette dernière n'est pas indispensable, elle est simplement rendue possible par les 2 PR sur le core et textwheel)

Les 3 PR sont notées en WIP car je pense qu'il faut une bonne relecture et tester un peu intensivement avant d'intégrer, et il manque la maj du changelog.

Mais pour le reste c'est OK pour moi: chez moi ça marche (tm)

et donc la 3ème PR sur SafeHtlm pour passer à Purifier complète le tout https://git.spip.net/spip/safehtml/pulls/4781 (mais cette dernière n'est pas indispensable, elle est simplement rendue possible par les 2 PR sur le core et textwheel) Les 3 PR sont notées en WIP car je pense qu'il faut une bonne relecture et tester un peu intensivement avant d'intégrer, et il manque la maj du changelog. Mais pour le reste c'est OK pour moi: chez moi ça marche (tm)
b_b reviewed 8 months ago
b_b left a comment
Owner

Sur la forme des logs :

  • toujours quelques accents qui manquent
  • b4252f1f69 preferentiellement => de préférence
Sur la forme des logs : - toujours quelques accents qui manquent - https://git.spip.net/spip/spip/commit/b4252f1f69eae47f81743b9b6bb0acd3289ef815 preferentiellement => de préférence
Owner

Testé avec les 3 PRs "actives" sous PHP 7.4 (cf spip/safehtml#4781) avec les listes de XSS suivantes :

Je n'arrive plus à mettre la main sur une liste compilée de chez OSWAP, qu'on a souvent mentionné ici, pour la tester, je ne retrouve que ça https://cheatsheetseries.owasp.org/cheatsheets/XSS_Filter_Evasion_Cheat_Sheet.html

En tout cas, ça semble bien efficace :)

Est-ce que ça vaudrait le coup qu'on teste ça sur un site de la communauté ? Je pourrais tenter de passer spip.net en 4.2 et le brancher sur les PRs.

Testé avec les 3 PRs "actives" sous PHP 7.4 (cf https://git.spip.net/spip/safehtml/pulls/4781#issuecomment-39644) avec les listes de XSS suivantes : - https://gist.github.com/sseffa/11031135 => liste dépassée, mais aucune ne passe dans le privé - https://github.com/payloadbox/xss-payload-list => liste bien complète, aucune ne passe dans le privé 👍 Je n'arrive plus à mettre la main sur une liste compilée de chez OSWAP, qu'on a souvent mentionné ici, pour la tester, je ne retrouve que ça https://cheatsheetseries.owasp.org/cheatsheets/XSS_Filter_Evasion_Cheat_Sheet.html En tout cas, ça semble bien efficace :) Est-ce que ça vaudrait le coup qu'on teste ça sur un site de la communauté ? Je pourrais tenter de passer spip.net en 4.2 et le brancher sur les PRs.
cerdic added 2 commits 8 months ago
Poster
Owner

J'ai complété la PR

  • pour améliorer le comportement et éviter que les raccourcis liens ne déclenchent des faux positifs du fait du -> transformé en -> ou ne soient perdus lors de la sanitization dans le public
  • pour éviter des faux positifs du fait de   qui semble aussi être parfois transformé en espace normal par safehtml
J'ai complété la PR * pour améliorer le comportement et éviter que les raccourcis liens ne déclenchent des faux positifs du fait du `->` transformé en `->` ou ne soient perdus lors de la sanitization dans le public * pour éviter des faux positifs du fait de ` ` qui semble aussi être parfois transformé en espace normal par safehtml
Owner

Je viens de tester de nouveau avec les derniers commits et j'ai un nouveau warning cf :

Warning: Undefined array key "connect" in ecrire/inc/texte_mini.php on line 554

Warning: Undefined array key "env" in ecrire/inc/texte_mini.php on line 554

Sinon, tout est bon, aucune XSS de la liste ne passe.

Je viens de tester de nouveau avec les derniers commits et j'ai un nouveau warning cf : ``` Warning: Undefined array key "connect" in ecrire/inc/texte_mini.php on line 554 Warning: Undefined array key "env" in ecrire/inc/texte_mini.php on line 554 ``` Sinon, tout est bon, aucune XSS de la liste ne passe.
marcimat added 12 commits 7 months ago
b3ef8a4dfa feat: la fonction echappe_html() s'enrichit d'un argument callback_options qui sera transmis aux callback d'echappement eventuelle
47b4fad2ed feat: la fonction echapper_html_suspect() change de signature pour prendre un tableau d'option en second argument ainsi que connect et env tels que reçus par propre() ou typo()
76f6378a23 fix: une fonction is_html_safe() personalisable pour tester si une chaine est safe plutot qu'une simple comparaison de longeur avant/apres
marcimat added 1 commit 7 months ago
marcimat added 1 commit 7 months ago
marcimat changed title from [WIP] Refactoring de la mise en sécurité des textes dans ecrire et public to Refactoring de la mise en sécurité des textes dans ecrire et public 7 months ago
b_b approved these changes 7 months ago
marcimat added 15 commits 7 months ago
12319f2da8 feat: la fonction echappe_html() s'enrichit d'un argument callback_options qui sera transmis aux callback d'echappement eventuelle
5ceac00b33 feat: la fonction echapper_html_suspect() change de signature pour prendre un tableau d'option en second argument ainsi que connect et env tels que reçus par propre() ou typo()
0def276295 fix: une fonction is_html_safe() personalisable pour tester si une chaine est safe plutot qu'une simple comparaison de longeur avant/apres
marcimat merged commit a3cb859602 into master 7 months ago
marcimat deleted branch refactor_texte_safety 7 months ago
Owner

\o/

\o/

Glop, j'ai un soucis avec ces commits :

  • echapper_html_suspect appelle modeles_echapper_raccourcis
  • mais cette fonction n'est pas dans le même fichier mais dans inc/modeles
  • qui n'est jamais chargé explicitement à cet endroit avant d'utiliser une fonction qu'il contient

Tant qu'on n'a pas de loader auto, et qu'une fonction est dans un autre fichier (à part utils…), quand c'est un fichier thématique pas forcément toujours là, on devrait toujours s'assurer de charger ce qu'il faut non ? Par exemple en haut de text_mini il y a inc/filtres et inc/lang… mais donc il faudrait inc/modeles aussi (ou dans la fonction modeles_echapper_raccourcis au moins)

Là moi j'ai parfois des erreurs partout dans l'admin (y compris avec recalcul ou vidage total de tmp/) : Line 546 : Call to undefined function modeles_echapper_raccourcis() avec plus rien d'utilisable

Glop, j'ai un soucis avec ces commits : - `echapper_html_suspect` appelle `modeles_echapper_raccourcis` - mais cette fonction n'est pas dans le même fichier mais dans `inc/modeles` - qui n'est jamais chargé explicitement à cet endroit avant d'utiliser une fonction qu'il contient Tant qu'on n'a pas de loader auto, et qu'une fonction est dans un autre fichier (à part utils…), quand c'est un fichier thématique pas forcément toujours là, on devrait toujours s'assurer de charger ce qu'il faut non ? Par exemple en haut de text_mini il y a inc/filtres et inc/lang… mais donc il faudrait inc/modeles aussi (ou dans la fonction modeles_echapper_raccourcis au moins) Là moi j'ai parfois des erreurs partout dans l'admin (y compris avec recalcul ou vidage total de tmp/) : `Line 546 : Call to undefined function modeles_echapper_raccourcis()` avec plus rien d'utilisable
Owner

Je n'ai pas rencontré ce problème lors de mes tests et encore à l'instant avec SPIP 4.2.0-dev GIT [master: 8694e97f]. Tu es certain que ton installation est à bien jour sur tous les plugins-dist aussi ?

Je n'ai pas rencontré ce problème lors de mes tests et encore à l'instant avec SPIP 4.2.0-dev GIT [master: 8694e97f]. Tu es certain que ton installation est à bien jour sur tous les plugins-dist aussi ?

Ouais bizarre le core était à jour mais pas tous les plugins dist. Ça remarche.

Cependant ça reste du coup un "hasard" si ça marche parce que "quelqu'un quelque part a chargé tel fichier en amont"…

Du coup ma remarque sur la solidité des include tant qu'on a pas de loader vaut un peu quand même : hormis utils.php, tout ce qui est thématique (texte, filtres, modeles, lang, etc) devrait être inclus à l'endroit où on l'utilise.

Ouais bizarre le core était à jour mais pas tous les plugins dist. Ça remarche. Cependant ça reste du coup un "hasard" si ça marche parce que "quelqu'un quelque part a chargé tel fichier en amont"… Du coup ma remarque sur la solidité des include tant qu'on a pas de loader vaut un peu quand même : hormis utils.php, tout ce qui est thématique (texte, filtres, modeles, lang, etc) devrait être inclus *à l'endroit où on l'utilise*.

Bonjour à tous,

les problèmes remontés ne sont pas pris en compte dans ce refactoring, donc on abouti logiquement aux mêmes problèmes (XSS) que ceux déjà fixé avant, cad :

<form id="test"></form><button form="test" formaction="javascript:alert(1)">X</button>

Ce motif abouti à une injection de code coté privé et coté public.

Bonjour à tous, les problèmes remontés ne sont pas pris en compte dans ce refactoring, donc on abouti logiquement aux mêmes problèmes (XSS) que ceux déjà fixé avant, cad : - recherche google sur : xss html5 - 1er lien : https://html5sec.org/ - 1er motif fourni dans la page : ```<form id="test"></form><button form="test" formaction="javascript:alert(1)">X</button>``` Ce motif abouti à une injection de code coté privé et coté public.

Bonjour à tous,

les problèmes remontés ne sont pas pris en compte dans ce refactoring, donc on abouti logiquement aux mêmes problèmes (XSS) que ceux déjà fixé avant, cad :

<form id="test"></form><button form="test" formaction="javascript:alert(1)">X</button>

Ce motif abouti à une injection de code coté privé et coté public.

Pour faire suite à #5271

Peut on profiter de ce refactoring pour élargir la liste des balises HTML filtrées :

A voir si le motif d'injection donné ne "profite pas" également de https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/texte_mini.php#L565

Je ne comprend pas pourquoi c'est mieux de faire 2 fois la protection/deprotection des éléments (1 fois dans echapper_html_suspect() et 1 autre fois dans typo/propre) plutot que de s'insérer au bon endroit dans typo() et propre() et le faire 1 seule fois (a la fin comme dans typo() et pas au début comme dans propre()).

Ca devient trèèèèès compliqué de comprendre echapper_html_suspect() dans typo/propre non ?

En espérant aider.

> Bonjour à tous, > > > les problèmes remontés ne sont pas pris en compte dans ce refactoring, donc on abouti logiquement aux mêmes problèmes (XSS) que ceux déjà fixé avant, cad : > > - recherche google sur : xss html5 > - 1er lien : https://html5sec.org/ > - 1er motif fourni dans la page : > > ```<form id="test"></form><button form="test" formaction="javascript:alert(1)">X</button>``` > > Ce motif abouti à une injection de code coté privé et coté public. > Pour faire suite à https://git.spip.net/spip/spip/pulls/5271#issuecomment-46651 Peut on profiter de ce refactoring pour élargir la liste des balises HTML filtrées : - https://git.spip.net/spip/textwheel/src/branch/master/wheels/spip/interdire-scripts.json#L20 : amha vouloir faire une liste est une mauvaise idée dans la mesure ou les navigateurs sont ce qu'ils sont et interprêtent n'importe quoi, proposition : avoir une liste "complète" HTML5 par défaut et en mode parano filtrer <.*) A voir si le motif d'injection donné ne "profite pas" également de https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/texte_mini.php#L565 Je ne comprend pas pourquoi c'est mieux de faire 2 fois la protection/deprotection des éléments (1 fois dans echapper_html_suspect() et 1 autre fois dans typo/propre) plutot que de s'insérer au bon endroit dans typo() et propre() et le faire 1 seule fois (a la fin comme dans typo() et pas au début comme dans propre()). Ca devient trèèèèès compliqué de comprendre echapper_html_suspect() dans typo/propre non ? En espérant aider.

Reviewers

b_b approved these changes 7 months ago
The pull request has been merged as a3cb859602.
Sign in to join this conversation.
Loading…
There is no content yet.