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

Open
cerdic wants to merge 12 commits from refactor_texte_safety into master
cerdic commented 2 weeks 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 2 weeks 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 2 weeks 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 2 weeks 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
b_b commented 2 weeks ago
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 6 days 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
b_b commented 5 days ago
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.
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch
Sign in to join this conversation.
Loading…
There is no content yet.