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
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'refactor_texte_safety'
Deleting a branch is permanent. It CANNOT be undone. Continue?
suite à spip-contrib-extensions/htmlpurifier#2 et à la relecture de #3926 j'ai donc entrepris un refactoring des appels à
echapper_html_suspect()
pourfiltrer_javascript=-1
qui était un peu cassé vu qu'on affichait du contenu échappé dans le public, ce qui est pas très user friendlyAu 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 uninc/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
Cette PR va avec spip/textwheel#4844 (il faut tester les 2 ensemble)
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)
Sur la forme des logs :
b4252f1f69
preferentiellement => de préférenceTesté 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.
J'ai complété la PR
->
transformé en->
ou ne soient perdus lors de la sanitization dans le public
qui semble aussi être parfois transformé en espace normal par safehtmlJe viens de tester de nouveau avec les derniers commits et j'ai un nouveau warning cf :
Sinon, tout est bon, aucune XSS de la liste ne passe.
[WIP] Refactoring de la mise en sécurité des textes dans ecrire et publicto Refactoring de la mise en sécurité des textes dans ecrire et public 7 months agoa3cb859602
into master 7 months ago\o/
Glop, j'ai un soucis avec ces commits :
echapper_html_suspect
appellemodeles_echapper_raccourcis
inc/modeles
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'utilisableJe 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.
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.
Reviewers
a3cb859602
.