Issues #5059 #4927 #3824 et #2109 : on reforme la logique du login #5064

plus de hashage+sel cote client, car les algos js sont penibles a gerer, voire buggués, et https est maintenant un standard de securite
cote serveur on utilise les fonction modernes de PHP pour la gestion des mots de passe (sel, poivre, hashage et verification)

Directement inspiré du plugin chiffrer de g0uz https://git.spip.net/spip-contrib-extensions/chiffrer avec en plus:

utilisation de sodium qui est natif avec PHP depuis 7.2 https://php.watch/articles/modern-php-encryption-decryption-sodium
la gestion de plusieurs cles, dont aussi le secret_du_site qui n'est plus en meta
une passe de modernisation sur les cles d'action egalement

Excellent, merci marcimat pour le dernier commit, j'ai relu et je pense que ça couvre bien les besoins en notifs !

Pour la suite (dans des versions ultérieures) :

Notons que ces histoires de jeton mériteraient quelques améliorations

permettre l’expiration des jetons

permettre plusieurs jetons pour un même auteur directement

Il me semble que pour ça, il faudrait une "table des clés", ou "table des jetons" (je sais pas si cette table ne serait que pour les auteurs, mais au moins pour ça), qui contiendrait à minima : id_auteur, cle, date_creation
Ce qui permettrait bien :

plusieurs clés par compte (et donc PLUS besoin de chercher l'ancienne clé ! on peut alors en générer une nouvelle différente à chaque notification, ce qui est encore mieux)
avec la date, de ne PAS dire qu'un jeton est valide, si on a dépassé X temps (défini dans un define par ex ou autre), cf le ticket : #4869 (ça pourrait être fait déjà même avec un seul jeton en ajoutant un champ dans spip_auteurs, mais peut-être que tant qu'à faire autant attendre une table multi ?)

J’ai tenté de suivre les recommandations de g0oZ qui disait que c’était dommage de mettre la clé secret_du_site sur le disque car si qqn·e arrive à l’afficher il peut alors générer certaines clés d’authentifications.

De la même manière si qqn·e peut lire la Bdd il peut aussi finalement…

Du coup, on s’est dit avec @cerdic que découper la clé en 2 était une solution convenable : il faut les 2 parties pour reconstituer la clé valide.

C’est ce que j’ai fait dans 3ba4787877

Cependant…
Il y a un hic.
La table spip_meta se sauvegarde (en clair) dans tmp/meta_cache.php

Et donc qqn·e avec un accès disque peut obtenir les 2 parties quand même pour le momment.

Que faire alors ?

Pour avoir rapidement regardé, la solution la plus propre (pour avancer avec l'existant sans tout revoir) me semble être de ne pas écrire cette valeure dans meta/cache.php.

Ce cas a déjà été prévu mais commenté dans la fonction touch_meta() :

https://github.com/spip/SPIP/blob/master/ecrire/inc/meta.php#L137


			// le secret du site est utilise pour encoder les contextes ajax que l'on considere fiables
			// mais le sortir deu cache meta implique une requete sql des qu'on a un form dynamique
			// meme si son squelette est en cache
			//unset($r['secret_du_site']);

J'ai également cherché au sein du core et des plugins les éventuelles utilisations direct de $GLOBALS['meta']['secret_du_site']. Seul le plugin mutualisation devrait être adapté : https://git.spip.net/spip-contrib-extensions/mutualisation/search?q=secret_du_site

Nous avons du mal a comprendre le commentaire et l'impact de la sortie du cache de cette valeur, Cédric tu saurais nous aider ?

Bon.

J’ai vaguement l’impression que ça nécessite des réflexions complémentaires et des tests plus poussés et va être sujet à des corrections encore, et que ça repousserait d’autant la sortie de SPIP 4.1, ce qui me parait là assez dommage compte tenu qu’elle était prête.

Alors proposition : Est-ce que ça peut attendre une 4.2, qui sortirait disons fin avril après avoir validé et testé cette PR et éventuellement celle sur l’introduction de composer #5056 qui a déjà une base intéressante ?

Une version intermédiaire donc avant une éventuelle 4.3 ensuite début de l’été ?

Des avis / envies ou pas ?
Est-il préférable de reporter plutôt la 4.1 pour intégrer (et tester cette partie) ?

Alors proposition : Est-ce que ça peut attendre une 4.2, qui sortirait disons fin avril après avoir validé et testé cette PR et éventuellement celle sur l’introduction de composer #5056 qui a déjà une base intéressante ?

+1 ça serait dommage de continuer à retarder la 4.1 à cause de cette "grosse évolution" qui arrive un peu sur le tard (sans remettre en cause son intérêt).

Je ne vois pas d'inconvénient à ce que cette évolution intègre la 4.2 en juillet prochain. Ce n'est peut-être pas utile de rajouter une 4.3 pour ça

Dommage, a 2 chars du push ^^

Plus sérieusement je vois pas de soucis à repousser.

A coté de ca c'est dommage de différer a cause de l'existence d'un chemin (avec un prérequis important) permettant de retrouver la clé complète. Il y en a certainement d'autres a trouver/fixer ; et amha ce n'est pas l'objectif initial.

Ca pourrait/devrait faire l'objet d'un ticket différent de la refonte du login non (indépendamment du quand) ?

Trouver tous les chemins permettant d'arriver a éxecuter du code avec un pré-requis type "je peux lire les fichiers sur le serveur" risque d'être long :)

Je rappelle que ce n'est pas juste une "évolution" mais un fix de #5059 : si vous voulez repousser cette PR, ça veut dire qu'il faut proposer un autre fix car on va pas décemment releaser une version avec un hash 256 en JS qu'on sait buggué.

Cette course à la release à tout prix me semble un peu ridicule...

Concernant cette PR, après relecture des derniers commentaires, il me semble qu'on est bon jusqu'à f9424719aa

Si l'on exclue ce commit on a : un fix de #5059, et une remise au propre de l'authentification pour être conforme à l'état de l'art, ce qui traite #4927 #3824 et #2109.

Sur le point particulier du secret_du_site() on ne dégrade rien par rapport à l'existant puisqu'il était dejà stocké sur disque dans tmp/meta_cache.php et donc un accès au disque suffisait à obtenir le secret pour forger des contextes ajax et de formulaires.

dans le cas des contextes ajax cela permettrait potentiellement de faire des inclusions arbitraires, mais si tu as accès au disque tu peux déjà probablement faire ça
dans le cas des formulaires, la gravité est fortement réduite depuis que les formulaires sont signés pour chaque session auteur. Tu ne peux donc forger que des formulaires anonymes ou pour ta propre session

@g0uZ la raison pour laquelle on a remis le secret_du_site dans le cache des meta, est qu'il est utilisé pour calculer les env des formulaires, lesquels sont la plupart du temps dynamique. Ça veut dire que si tu as un formulaire dans la page (par exemple de notation, de forum), le simple affichage du formulaire va requerir une requete en base, et donc on casse la feature de pouvoir afficher une page en cache sans requete en base, ce qui a un gros impact performance également.

Une autre proposition pour protéger globalement tmp/meta_cache.php (ce fichier contient beaucoup d'information qui mériterait d'être protégé) d'un attaquant qui peut lire des fichiers serait de le stocker avec un nom non prédictible (et qui n'implique pas une requête en DB pour le reconstituer).

Est ce qu'on peut trouver une valeure sécrète et constante (qui change rarement) uniquement récupérable via PHP, très rapidement, qu'on pourrait utiliser pour stocker tmp/meta_cache_[md5(mavaleure)].php ?

gouz@T530 /home/gouz $ time php -r 'for ($i=0; $i< 1000;$i++) { $r=md5(serialize(ini_get_all())); }'

real    0m0,119s

C'est peut être une très mauvaise idée, je ne sais pas bien :)

Si bonne idée, l'implémentation peut être assez rapide si on conserve tmp/meta_cache.php qui contiendrait uniquement le code suivant :


<?php

$token=md5(serialize(ini_get_all()));
include("meta_cache_".$token.".php");

@g0uZ j'ai répondu dans #5066 pour ne pas mélanger les sujets

Je rappelle que ce n'est pas juste une "évolution" mais un fix de #5059

À ce que je sache la 4.0 et la 3.2 auront toujours le bug (cette PR ne sera pas reportée). Et des bugs / évolutions à traiter c’est pas ce qui manque à côté non plus.

Je ne cherche pas à minimiser le problème que ça corrige ; juste que les corrections mêmes apportées par ce ticket nécessitent manifestement quelques tests (en passant d’ailleurs on n’a a pas encore fait de TU dessus, ce qui serait pourtant une bonne idée).

gOuZ signale au fur et à mesure des améliorations à apporter, à juste titre, et en soit par conséquent, on voit que ça ne semble pas parfaitement mature. Je ne pense pas avoir manqué de mauvaise volonté sur ce ticket.

Je vois juste que ça va reporter encore, parce que "profitons de cette sortie qui arrive pour faire xxx" (c’est vite fait) :)

Cette course à la release à tout prix me semble un peu ridicule...

Soit.

Je vais essayer de réexprimer mes pensées :

on avait essayé de se mettre d’accord sur un calendrier. J’ai essayé de le tenir, (et d’adapter constamment mes scripts de release entre autres), mais peut être que ça peut pas marcher finalement ? Ou peut être que ça convient pas ?
définir un calendrier évite (normallement) justement ce genre de situation : ne pas intégrer au dernier moment une évolution qui débarque sans prévenir et repousse la sortie : parce que ça peut durer longtemps et être sans fin (c’est pas comme si on savait pas… )
on n’est pas forcé d’avoir toujours des grosses releases avec plein de features dedans. Des petites releases X.Y plus fréquentes avec moins de delta, ça peut aussi fonctionner a priori aussi. C’est pour ça que j’ai tenté de proposer de décaler ce ticket.

Maintenant osef, j’ai pas envie de batailler sur ça, on peut attendre pour de l’intégrer et de tester pour la 4.1

Et sortir la 4.1 «quand elle sera prête». C’est bien cette date là, au moins ça ennuira personne.

Bah vous n’aurez qu’à me dire quand ce sera le cas, quand faut releaser et quoi…
Sinon vous savez aussi où sont mes scripts de release.

L’idée de hacher le chemin tmp/cache/meta/meta_{hash}.php me semble intéressant. Reste à trouver avec quoi calculer le hash par exemple

(mais ça veut dire aussi supprimer les vieux fichiers à un moment dans ce cas non ?)

> Je rappelle que ce n'est pas juste une "évolution" mais un fix de #5059 À ce que je sache la 4.0 et la 3.2 auront toujours le bug (cette PR ne sera pas reportée). Et des bugs / évolutions à traiter c’est pas ce qui manque à côté non plus. Je ne cherche pas à minimiser le problème que ça corrige ; juste que les corrections mêmes apportées par ce ticket nécessitent manifestement quelques tests (en passant d’ailleurs on n’a a pas encore fait de TU dessus, ce qui serait pourtant une bonne idée). gOuZ signale au fur et à mesure des améliorations à apporter, à juste titre, et en soit par conséquent, on voit que ça ne semble pas parfaitement mature. Je ne pense pas avoir manqué de mauvaise volonté sur ce ticket. Je vois juste que ça va reporter encore, parce que "profitons de cette sortie qui arrive pour faire xxx" (c’est vite fait) :) > Cette course à la release à tout prix me semble un peu ridicule... Soit. Je vais essayer de réexprimer mes pensées : 1) on avait essayé de se mettre d’accord sur un calendrier. J’ai essayé de le tenir, (et d’adapter constamment mes scripts de release entre autres), mais peut être que ça peut pas marcher finalement ? Ou peut être que ça convient pas ? 2) définir un calendrier évite (normallement) justement ce genre de situation : ne pas intégrer au dernier moment une évolution qui débarque sans prévenir et repousse la sortie : parce que ça peut durer longtemps et être sans fin (c’est pas comme si on savait pas… ) 3) on n’est pas forcé d’avoir toujours des grosses releases avec plein de features dedans. Des petites releases X.Y plus fréquentes avec moins de delta, ça peut aussi fonctionner a priori aussi. C’est pour ça que j’ai tenté de proposer de décaler ce ticket. Maintenant osef, j’ai pas envie de batailler sur ça, on peut attendre pour de l’intégrer et de tester pour la 4.1 Et sortir la 4.1 «quand elle sera prête». C’est bien cette date là, au moins ça ennuira personne. Bah vous n’aurez qu’à me dire quand ce sera le cas, quand faut releaser et quoi… Sinon vous savez aussi où sont mes scripts de release. ---- L’idée de hacher le chemin tmp/cache/meta/meta_{hash}.php me semble intéressant. Reste à trouver avec quoi calculer le hash par exemple (mais ça veut dire aussi supprimer les vieux fichiers à un moment dans ce cas non ?)

Je pense qu'il y a une marge entre dire "OK ce bug ne sera pas fixé sur les versions anciennes." et "OK on release une nouvelle version qui contient toujours ce bug"...

Il s'agit pas de reporter sans cesse. Encore une fois, hormis le dernier commit qui traite un problème qu'on peut traiter plus tard, tout semble bon.

Je propose donc d'intégrer sans ce dernier commit et pour ce faire, je peux refaire une passe de tests au préalable et passer en live sur contrib pour vérifier qu'on est bon avant de merger et release...

(et je suis tout à fait d'accord qu'il faudrait aussi faire des tests unitaires, mais considérons que ce n'est pas bloquant et qu'on peut finir ça ensuite)

(pour les anciennes versions 3.2, on peut proposer le palliatif d'utiliser la constante _AUTORISER_AUTH_FAIBLE qui permet de ne plus hasher les mots de passe côté client, pour les sites qui seraient embêtés par le bug et qui sont en https)

on peut proposer le palliatif d'utiliser la constante _AUTORISER_AUTH_FAIBLE

Comme tu le notes judicieusement, ça aurait aussi été une solution en 4.1 sans ce correctif :p

(don’t feed the troll)

Après relecture, on peut même garder le dernier commit qui concerne le secret_du_site: même si il ne corrige pas complètement la remarque initiale de @g0uZ il améliore la sécurité globale puisqu'il faut accèder à la fois à config/cle.php et tmp/meta_cache.php pour avoir le secret.

Je viens de faire une série de test :

connexion/deconnexion OK
connexion webmestre apres suppression du fichier cle.php OK
tentative de connexion redacteur apres suppression du fichier cle.php OK : pas de connexion possible, pas de generation des cles auth, log qui indique qu'un webmestre avec un backup doit de connecter pour restaurer les cles
re-installation OK
re-installation par un webemstre existant apres suppression du fichier cle.php OK
re-installation en essayant de creer un nouveau webmestre apres suppression du fichier cle.php => OK message erreur indiquant que un webmestre existant avec un backup doit faire la procedure d'install (chaines de langues ajoutées)

Je test un upgrade sur dev-contrib et si ok je place la branche en prod sur contrib (je fais un backup de spip_auteurs avant pour pouvoir revert les alea/pass si besoin)

Lors de l'installation, on créé le fichier cles.php ET on fait un backup des cles immediatement sur le compte webmestre créé, donc pas de risque de perdre les cles.
Mais lors de la mise à jour on avait un risque car on créait le fichier de cles au premier login, et on commençait à hasher les password avec même si aucun webmestre ne s'était connecté.

J'ai envoyé 7be4a254e pour résoudre ça : tant qu'un webmestre ne se loge pas après upgrade, on ne génère pas la nouvelle clé.

J'ai testé une mise à jour sur dev-contrib sans soucis, et j'ai mis la branche https://git.spip.net/spip/spip/src/branch/refactor_auth_spip41 (rebase de cette PR en 4.1) en prod, donc on est en test live

Ce soir j'ai voulu me connecter sur contrib->il ne reconnaissait pas mon mot de passe. Et pourtant il n'a pas changé depuis hier, et je l'avais en plus en memoire dans firefox.

Seule solution : reinitialiser mon mot de passe.

je tente de voir en local.

Refus de connexion aussi mais Firefox n'a pas perdu la mémoire.

Bon, en local je ne reproduis pas...

Ah attend c’est peut être ma faute…
Je me demande s’il ne s’est pas remis sur un commit hors de la branche de test

Et donc on était bien sortis de la branche de test sur Contrib tout à l’heure… désolé