Labels Milestones

[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

pull from: refactor_texte_safety

merge into: spip:master

spip:1.8

spip:1.9.1

spip:1.9.2

spip:2.0

spip:2.1

spip:3.0

spip:3.1

spip:3.2

spip:4.0

spip:4.1

spip:boutons-danger

spip:debug_ecrire_fichier

spip:dev-sortable

spip:dev/autoloader

spip:dev/instituer_ergo

spip:dev_infos_image

spip:fix/valider_url_distante

spip:fix_modifier_login

spip:issue_4101

spip:issue_4678

spip:issue_4705

spip:issue_4717

spip:issue_4946

spip:issue_5016

spip:issue_5056_composer_road

spip:issue_5272_php_82

spip:issue_5273

spip:master

spip:neversayoupsagain

Conversation 5 Commits 12 Files Changed 5

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

8f5be2c5fa chores: synchronisation du code avec celui de textwheel car toute la partie traiter_modele() est forkee sans raison

3a5d10ee31 chores: deplacer le code specifique au traitement des modeles dans inc/modeles pour eviter le fork par le plugin textwheel

4a5ccaddc8 chores: refactorer la fonction traiter_modele en separant la detection/collecte d'une part dans une fonction modeles_collecter() du traitement d'autre part

cb8f350f01 feat: prise en charge de l'option collecter_liens=false sur la fonction modeles_collecter() et optimisation

1f5846e2e8 feat: un jeu de fonction modeles_echapper_raccourcis() et modele_retablir_raccourcis_echappes() pour echapper les modeles sous forme de texte neutre qui ne sera pas considere comme dangereux par un purifier html puis les retablir

b4252f1f69 feat: la fonction echappe_html() s'enrichit d'un argument callback_options qui sera transmis aux callback d'echappement eventuelle ...

+ si callback_options contient une entree secure_prefix alors on cherche preferentiellement une callback d'echappement prefixee par ce prefixe, et sinon on se rabat sur la callback sans ce prefixe (Le secure_prefix peut se cumuler avec l'argument deja existant callback_prefix)

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() ...

- on assure une compatibilité avec l'ancienne signature d'appel pour ne pas casser du code appelant ou une personalisation de la fonction
- le tableau d'option permet de plus de passer
  - wrap_suspect pour baliser le code suspect avec un markup a l'aide de la fonction wrap, ce qui permet d'adapter le rendu au cas par cas,
  - texte_source_affiche qui sera le texte affiche apres mise en securite si le texte qu'on examinait parait suspect
- dans l'espace prive, la fonction continue a echapper le html suspect, mais avec plus de discernement : elle ne trig plus sur les modeles qu'on masque via modeles_echapper_raccourcis() et elle commence par echapper tout les tricks js/xss connus via echappe_js(). Si malgre cela safehtml() semble y trouver a redire, on echappe tous les tags html dans l'affichage pour avoir quelque chose de plus lisible et mieux distinguer html du texte
- dans l'espace public, la fonction applique un safehtml() sur le texte sans ses modeles puisqu'on est la uniquement dans le cas parano de la globale filtrer_javascript=-1

e3711fd925 fix: passer connect et env dans les appels de echapper_html_suspect() + un petit peu de refactoring

a0022487d1 fix: ne pas envoyer un connect null a traiter_modeles()

cerdic referenced this pull request from spip/textwheel 2 weeks ago

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

cerdic commented 2 weeks ago

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 ...

par defaut on gere une comparaison un peu plus robuste pour passer le test via HTMLPurifier, mais la fonction est personalisable si on veut utiliser une autre lib

cerdic referenced this pull request from spip/safehtml 2 weeks ago

[WIP] fix: Bye bye safehtml, bienvenue HtmlPurifier #4781

cerdic commented 2 weeks ago

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 :

• 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.

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.

👍 3

cerdic added 2 commits 6 days ago

072da13e50 fix: ne pas perdre les raccourcis liens dans du texte qui passe dans safehtml, soit pour le test is_html_safe() soit pour l'affichage. On expanse donc les liens avant si on a été appelé par propre()

3a65d5ced4 fix: eviter is_html_safe() de déclencher de faux positifs du fait de la présence d'un   qui peut être réduit par safehtml()

cerdic commented 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

👍 1

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.

Reviewers

Request review

No reviewers

Labels

Apply labels

Clear labels

accessibilité amélioration
Amélioration, nouvelle fonctionnalité APIs authentification base de données bug
Ca ne fonctionne pas code généré compilo css divers documentation doublon
Ce ticket est un doublon ergonomie espace privé filtres et balises formulaires Inscription installation invalide
Ticket invalide javascript langues LDAP plugin PostgreSQL refusé
Ignoré, c'est comme Ca... sécurité traduction

No Label accessibilité amélioration APIs authentification base de données bug code généré compilo css divers documentation doublon ergonomie espace privé filtres et balises formulaires Inscription installation invalide javascript langues LDAP plugin PostgreSQL refusé sécurité traduction

Milestone

Set milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Assign users

Clear assignees

No Assignees

2 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Write Preview

Loading…

Cancel

Save

There is no content yet.