Merci à toi pour cette investigation et ces solutions
Peut-être qu’on pourrait mettre en place une rémunération provenant du compte des développeurs pour rémunérer ce travail d’investigation et de proposition de solution.
Qui va s’occuper de faire le changement de protocole (v12) et l’intégration des deux versions de TweetNaCl dans Duniter ? Pareil, une rémunération provenant du compte des développeurs pour ces travaux serait bienvenue.
Pour ma part, je l’ai jamais fait.
Côté Silkaj ≥ 0.7.0, ça utilise PyNaCl (libsodium). Il est prévu de dégager le code qui l’utilise.
Côté DuniterPy, ça utilise pyaes, pylibscrypt et libnacl. En dessous, libnacl utilise libsodium installé par de gestionnaire de paquets du système.
Pour conclure, je pense que les bibliothèques de cryptographie qui nous intéressent ici sont PyNaCl et libnacl qui sont toutes les deux basées sur libsodium.
Oui, on pourra essayer de vérifier la signature du bloc #15144.
Je suis en capacité de le faire moi-même si nécessaire, une rémunération spécifique pour cette tache serait bienvenue
Bien entendu si @cgeek souhaite le faire lui même je lui laisse la main (Je souhaiterai toutefois que ce soit fait rapidement).
Si vous pouvez faire le test rapidement (avant de changer de wrapper), vous devriez normalement constater que la signature est invalide, essayez ensuite avec la signature générée par Dunitrust et vous devriez la trouver valide.
Avant de faire tout changement, il faut également s’assurer que Cesium dépend d’une lib de crypto a jours, @kimamila on a donc besoin que tu vérifie également la signature du bloc #15144
This error should be relatively frequent, it happens around or a bit less than one time for every 2^16 computations
2^16 c’est bien du même ordre de grandeur que la fréquence des signatures invalides dans la blockchain Ğ1 (les blocs #0 à #15143 sont valides, le #15144 est le 1er invalide, ce qui est cohérent avec une fréquence théorique de 1/2^16).
Ok grâce aux tests de @Moul on a confirmation que Duniterpy utilise une lib de crypto iso avec la norme Ed22519 tout comme Dunitrust :
Ce qui veut dire qu’environ 1 document sur 2^64 signé par silkaj/sakia est rejeté par Duniter, c’est sans doute déjà arrivé sans qu’on s’en rende compte (une transaction qui ne passe pas pour raison inconnue, on la refais quelques blocs plus tard et ça passe car le hash du document n’est plus le même, le blockstamp ayant changé).
Je pense que nous avons suffisamment d’éléments pour prendre la décision de corriger Duniter.
Je propose de remplacer naclb par tweetnacl-js pour 3 raisons :
tweetnacl-js est une réécriture en asm.js du code C de la lib tweetnacl. Ce qui permettra de garder de bonnes performances (perf d’asmjs quasiment natives) tout en simplifiant et accélérant le build et le packaging de Duniter (une lib C en moins a compiler).
Ça reste aussi léger que TweetNaCl, ce qui permet de garder une lib de crypto minimaliste et légère.
Voici comment je compte procéder :
Ajout de tweetnacl-js dans Duniter 1.7.20 (en conservant naclb) et utilisation de tweetnacl-js pour les blocs v12, déclenchement automatisé via nonce.
Test du passage en V12 sur la g1-test uniquement.
Si tout s’est bien passé sur la g1-test, lancement de l’appel a installer la 1.7.20 sur la G1 puis attente du passage automatique en V12 sur la G1.
Quand le passage en V12 s’est bien passé sur la G1, livraison d’une nouvelle release de Duniter qui supprime naclb et qui ne vérifie plus les signatures des bloc v10 et v11.
@cgeek ce plan te conviens t’il ? Si oui je compte prioriser ce chantier au plus tôt, c’est à dire arrêter Dunitrust et consacrer mes prochains WE au développement de cette proposition
Si tout cela se confirme, je dirais au contraire que c’est une bonne nouvelle : une faille cryptographique a été détectée, ce qui est une des failles les moins faciles à mettre en évidence. La preuve, il a fallu 2,5 ans pour débusquer celle-ci. Et pourtant rien n’était caché, c’est du logiciel libre et en plus @Junidev avait plus ou moins repéré cette anomalie. Mais c’en était resté là.
Je vais regarder davantage l’origine du bug, ça m’intéresse, merci
Oui, c’était déjà le cas avec Junidev qui a révélé quelques anomalies dans le protocole. En fait sauf si ton passe-temps est de lire des protocoles, il y a peu d’occasions de vérifier la cohérence de l’ensemble (protocole + code). Là c’est une occasion en or, assurément.
À quoi sert cette phrase ? Bien évidemment que personne n’a envie d’utiliser une librairie de crypto buguée, quant à l’audit celui-ci n’est pas une démonstration de l’absence de bug. Il rassure, tout au plus.
Pour la rapidité, honnêtement ce n’est pas si pressé d’un point de vue sécurité : il y a bien d’autres façon d’attaquer techniquement la Ğ1. Il n’y a pas lieu de s’alarmer, par contre il n’y a aucune raison de traîner non plus.
Si tu veux le faire, OK. Ce serait un bon exercice pour te familiariser avec le déclenchement via nonce.
Je te demande juste de faire une MR pour que je vérifie.
Ça fait une probabilité de très faible si tu multiplies ça par l’utilisation de Silkaj/Sakia/DuniterPy.
Ça s’est peut-être déjà produit avec Césium.
Est-ce qu’il est possible d’identifier/savoir le numéro des blocs invalides ?
Est-ce que ça se reproduit selon une suite arithmétique ?
Si oui, on pourrait demander à Duniter d’ignorer la vérification de la signature de ces blocs en particulier.
C’est peut-être trop violent de ne plus vérifier la signature d’environ 280 000 blocs.
Si ça se reproduit tous les 15 000 blocs, ça fait environ 18 blocs à ignorer par la future version du protocole de vérification des blocs v12.
Oui tu as raison cette phrase ne sert à rien dsl, des fois j’enfonce des portes ouvertes par peur de ne pas être compris
100% d’accord
En effet, le bug en question ne semble pas présenter de risque de sécurité, si je suis tant pressé c’est parce que ça me bloque pour avancer sur Dunitrust, j’ai découvert ce bug en mergeant la validation locale, et depuis Dunitrust ne peut pas rester synchroniser, sauf à commenter toutes les parties qui vérifient les signatures, ce que j’ai fait mais c’est moche et comme je dois construire par-dessus ces parties de code j’ai besoin que ce problème soit résolu rapidement.
Oui biens sûr je te ferai une MR et je te taggerai quand elle sera prête pour une review
Oui Cesium est basé sur libsodium comme duniterpy (et donc pas sur TweetNaCl) donc ça c’est forcément déjç produit car il y a plusieurs dizaines de milliers de documents utilisateurs dans la blockain G1.
Oui, je peux faire ça avec Dunitrust en modifiant légèrement le code pour qui la sync check les signatures et log les signatures invalides.
En fait non car c’est déjà ce qu’on fait. Duniter ne vérifie la signature des blocs que en mode start, pas en mode sync, sauf pour une sync cautious qui est un mode particulier utilisé seulement par les développeurs pour tester.
On peut toutefois lister les blocs V10 et V11 invalides après passage en V12 puis n’exclure que ceux-là du contrôle lors de la sync cautious mais ça fait beaucoup de boulot en plus pour une fonctionnalité qui n’est pas utilisée par l’utilisateur final de Duniter (sync cautious), donc personnellement je pense que le jeu n’en vaut pas la chandelle.
Trop de solutions “temporaires” deviennent définitives par manque de moyens humain ou dé-priorisation, non vraiment ça ne me conviens pas du tout de faire comme ça, je sent que ça traînerai tel quel un moment. En plus, l’api de tweetnacl-usable impose d’avoir accès a la secret key pour signer, or ring m’interdit d’y accéder et j’ai refactoré tout Dunitrust en ce sens (paradigme safety by design).
Refactorer Dunitrust pour utiliser une lib de crypto buggée serait du temps purement perdu, je préfère consacrer ce temps à améliorer Duniter.
De plus, comme Dunitrust n’est pas tenu par une deadline particulière, je n’ai aucune raison de préférer une solution temporaire pas satisfaisante, je peut me permettre de me concentrer directement sur une solution long terme
Pour moi c’est obligatoire pour garantir la cohérence du protocole. Sinon ça signifie qu’on ne pourras plus utiliser le sync cautious, même si un jour on souhaite que les utilisateurs s’appuient aussi dessus (le jour ou on aura résolu un problème de perfs ?)
Le langage Rust inclus l’encapsulation*, ainsi le développeur d’une librairie peut choisir d’interdire l’accès a certains champs d’une struct (=attributs d’un objet en POO).
C’est le choix réalisé par ring, cette librairie est conçue de façon a ce qu’il est impossible d’accéder a la clé privée, a aucun moment, pas même lors de la génération du trousseau (on donne un seed et on reçoit une struct “keypair”).
L’avantage d’utiliser une librairie ainsi faite, c’est que l’on est certain qu’aucun contributeur de Dunitrust ne pourra exposer la clé privée par inadvertance (moi le premier ), cela apporte donc plus de sécurité.
Quand j’ai migré sous ring, j’ai du refactorér une bonne partie de la codebase pour m’adapter au fait que je n’ai pas accès a la clé privée. Ce serait donc couteux en refactoring de reswitcher sur une lib qui me demande la clé privée pour signer.
*NB: Rust inclus un mécanisme d’introspection, mais contrairement a d’autres langages, ce dernier ne permet pas d’outrepasser la visibilité des entités (champs, fonctions, types, etc) définies dans une autre crate.
De mon point de vue ce n’est pas un problème de perf, la sync cautious sera toujours trop lente pour être utilisée par l’utilisateur final car par construction la validation globale est couteuse, même en l’optimisant a mort, et ça n’apporte pas de plus-value particulière par rapport a une sync rapide.
La sync rapide garantie déjà (via la vérification de tous les hashs) d’avoir la même blockchain que le nœud de confiance renseigné à la virgule près.
Toutefois, si vous y tenez, vous pouvez lister tous les blocs invalides, et indiquer la liste dans la RFC du protocole V12, afin de permettre a ceux qui voudrais vérifier la blockchain de pouvoir quand même vérifier la signature de tous les blocs v10 et v11 ne se trouvant pas dans votre liste. Duniterpy peut le faire, c’est un bon exercice
Mais tu n’as pas abstrait les fonctions de signature / vérification ? Tout cela est encapsulé dans keyring.ts dans Duniter, l’objet se construit en fournissant la clé privée (et publique) puis on ne la redemande jamais.
Avec cela le coût de refactoring est pratiquement nul.
Tu parles là d’un utilisateur qui veut faire une synchro, Inso parle de celui qui veut valider la blockchain relativement au protocole. Celui-là veut justement vérifier chaque bloc un à un, selon les règles établies pour la version dudit bloc.
Si bien sûr on à une grosse couche d’abstraction dans ed25519.rs, et même une autre couche d’abstraction par dessus dans le module keys pour être agnostique de l’algo de crypto.
Le problème que j’ai rencontré quand jai migré sous ring c’est que la couche d’abstraction partait du postulat que le Keypair était Clonable, ce que ring ne permet pas. J’ai donc été obligé de repenser entièrement la couche d’abstraction pour permettre que le KeyPair ne soit pas clonable, ce qui est bien plus compliqué qu’il n’y parait si l’on veut rester safe et multithread.
Il est vrai que passer d’un lib de crypto très contraignante a une lib moins contraignante devrait pouvoir ce faire en gardant la même API pour la couche d’abstraction, donc sans avoir a refactorer toute la codebase, et je pense que c’est le cas. Mais ça reste tout de même du dev a faire, je n’ai pas modifié la couche d’abstraction. Pour tester TweetNaCl j’ai fait un binding manuel très salement dans un TU temporaire, ce n’est pas utilisable “tel quel” dans tout le projet.
Migrer ma couche d’abstraction de crypto sur la version buggée de tweetnacl me demanderais certes moins de boulot a court terme que de dev Duniter 1.7.20 selon ma proposition plus haut.
Mais ce serait me binder sur du C (ce que j’essaye d’éviter au maximum), moins performant et moins secure que ring, perdre en perf, perdre en sécurité et perdre en simplicité (a cause du binding C) c’est un coût que j’estime bien plus cher que de maj la lib de crypto de Duniter
De plus, a long terme ça représenterai plus de boulot au global, car se serait censé être temporaire et tôt ou tard il m’aurait fallu remigrer sur ring une fois la lib de crypto de Duniter mise a jours. Cela équivaudrait a creuser un trou pour le reboucher.
Oui vous avez raison, donc il faut cette liste, toutefois je n’en ai pas besoin pour dev Duniter 1.7.20 car il comportera encore les 2 lib de crypto et sera donc toujours en mesure de vérifier tous les blocs.
J’indiquerai dans la doc du protocole que les blocs v10 et v11 doivent être vérifiées avec une version précise de TweetNaCl.
J’aimerais bien @Moul et @Inso (ou d 'autres) s’occupent de faire cette liste, afin que je n’ai pas tout a faire sur ce dossier
Désolé si je te fais répéter, mais pour être bien clair : Dunitrust ne vérifiera pas la signature de ces blocs ? Et quid de la vérification du reste des règles du protocole v10 et v11 ?
