Duniter utilise une ancienne version buggée de TweetNaCl: que faire?

Nodes Duniter-v1
, ,
1

Certains ont peut-être suivi mes échanges rapprochés avec @cgeek lors des RML14 concernant un écart entre Dunitrust et Duniter sur la génération des signatures et leur vérification.
Afin de ne pas créer d’inquiétudes inutiles, j’attendais de découvrir le fin mots de l’histoire avant de communiquer sur le sujet.
Après de nombreuses fausses pistes, des dizaines d’heure de recherche et de tests, un gros plongeon dans le C et son binding avec Rust (j’avais oublié à quel point c’est moche et chiant a gérer le C :yum: ), j’ai enfin trouvé le fin mots de l’histoire, et j’ai une mauvaise nouvelle comme le titre vous le fait deviner.

Dans Duniter, cgeek utilise un binding custom de tweetnacl-usable. Le problème avec les binding custom c’est que l’on ne bénéficie pas des correctif.

Or, le 16 février 2015, le dépôt tweetnacl-usable a été mis a jours pour intégrer un correctif d’un bug dans TweetNaCl : updated to latest tweetnacl · ultramancool/tweetnacl-usable@a8dcaa7 · GitHub

Cet update est cassant et change le comportement. J’ai bindé en Rust les 2 versions du dépôt tweetnacl-usable (a8dcaa7 avec correctif et 9a160106 sans correctif).

La version a8dcaa7 avec correctif a exactement le même comportement que les 4 autres lib de crypto que j’ai testé, a savoir: ring, cryptoxide, rust-crypto et sodiumoxide (oui j’en ai pris 4 pour être vraiment sûr). Le comportement étant : environ une signature sur 15000 est invalide dans la blockchain g1 (toujours les mêmes pour chaque lib).

A savoir que certaines signatures invalides correspondent a des blocs que j’ai forgé moi-même, notamment le #15144, j’en ai donc profité pour re-signer ce bloc avec Dunitrust :

Duniter signature : fJusVDRJA8akPse/sv4uK8ekUuvTGj1OoKYVdMQQAACs7OawDfpsV6cEMPcXxrQTCTRMrTN/rRrl20hN5zC9DQ==

Dunitrust signature (avec tweetnacl-usable a8dcaa7, + 4 autres lib de crypto) : aZusVDRJA8akPse/sv4uK8ekUuvTGj1OoKYVdMQQ/3+VMaDJ02I795GBBaLgjypZFEKYlPMssJMn/X+F/pxgAw==

Dunitrust signature (avec tweetnacl-usable 9a160106) :
fJusVDRJA8akPse/sv4uK8ekUuvTGj1OoKYVdMQQAACs7OawDfpsV6cEMPcXxrQTCTRMrTN/rRrl20hN5zC9DQ==

J’ai réussi a binder la version 9a160106 sans correctif (intégrée dans Duniter), et Dunitrust a alors la même anomalie, ce qui lui permet d’être parfaitement ISO avec Duniter.

Mais utiliser une ancienne version buggée de TweetNaCl n’est pas envisageable sur le long-terme, personne* ne s’en ai rendu compte en 2 ans et demi car cela ne concerne qu’une signature sur 15000 et que seuls les serveurs blockchain vérifient les signatures, les clients se contentent de les émettre, et générer un document sur 15000 qui ne passe pas ça peut passer inaperçu longtemps.

*En fait si, junidev s’en est forcément rendu compte en développant Juniter puisqu’il vérifiait les signatures, d’ailleurs @cgeek tu m’a bien dit aux rml14 que “Junidev avait rencontré le même problème”. Il semble s’être contenté de se baser sur le même code C que Duniter sans creuser le pourquoi du comment, car il n’aurait pas du avoir de problème (et moi non plus).

Je me rend compte que le développement de Dunitrust vas être l’occasion de faire un audit au peigne fin de Duniter, pour chaque écart constaté je ne vais pas me contenter de copier Duniter pour que ça marche, je vais a chaque fois chercher a comprendre le pourquoi du comment de chaque écart.
Cela permettra a terme d’avoir beaucoup plus confiance en le fait que les serveurs blockchain font bien ce qu’il sont censés faire.

La blockchain Ğ1 n’est pas conforme au standard Ed25519 et tôt ou tard d’autres s’en rendrons comptes.
De plus, il n’est pas envisageable pour moi d’utiliser une lib de crypto buguée et non auditée (l’audit de TweetNaCl ayant eu lieu après le correctif).

Ça me pose aussi des problèmes de performances, TweetNaCl étant une implémentations de Ed25519 assez lente.

Proposition a court-terme

Ma proposition : Intégrer les 2 versions de tweetNaCl dans Duniter, réaliser un passage en V12 coordonné.

Proposition à moyen-terme

Une fois que les réseaux ĞT et Ğ1 seront en V12, ne plus vérifier la signature des blocs V10 et V11 (ce n’est déjà pas vérifié en sync rapide de toute façon).
La sync vérifiant tout les hashs, et la signature du bloc genesis étant reconnu valide dans les 2 versions de TweetNaCl, on pourra maintenir la vérification de la signature du bloc genesis en mode cautious pour conserver une garantie sur l’origine.
Dunitrust ne vérifiera les signatures que en V12 directement.

J’ai quelques questions pour les développeurs des clients @Moul @vit @kimamila : quelle lib de crypto utilisez vous ? N’avez vous jamais essayer de vérifier les signatures des blocs ? Pouvez vous essayer de vérifier la signature du bloc #15144 avec votre lib de crypto ?

message:

InnerHash: 8B194B5C38CF0A38D16256405AC3E5FA5C2ABD26BE4DCC0C7ED5CC9824E6155B\nNonce: 30400000119992\n

pubkey: D9D2zaJoWYWveii1JRYLVK3J4Z7ZH3QczoKrnQeiM6mx
sig: fJusVDRJA8akPse/sv4uK8ekUuvTGj1OoKYVdMQQAACs7OawDfpsV6cEMPcXxrQTCTRMrTN/rRrl20hN5zC9DQ==

3 Likes
2

Merci à toi pour cette investigation et ces solutions :+1:

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.

Sur DuniterPy, on a deux tickets concernant la cryptographie : un pour migrer de wrapper libsodium et l’autre pour utiliser scrypt 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.

1 Like
3

Ce serait bienvenue en effet :slight_smile:

Je suis en capacité de le faire moi-même si nécessaire, une rémunération spécifique pour cette tache serait bienvenue :slight_smile:
Bien entendu si @cgeek souhaite le faire lui même je lui laisse la main :slight_smile: (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 :slight_smile:

1 Like
4

Plus de détails sur le bug en question : Arithmetic Bug in TweetNaCl

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

Le bug m[15] est bien présent dans le binding de cgeek : naclb/tweetnacl.cpp at master · duniter/naclb · GitHub

Et bien corrigé dans la dernière version de tweetnacl-usable : tweetnacl-usable/tweetnacl.c at master · ultramancool/tweetnacl-usable · GitHub

2 Likes
5

Nop, je fais ça aujourd’hui pour Duniterpy !
Bravo pour ce boulot ! :+1:

1 Like
6

Voila qui devrait intéressé @Junidev, qui avait également relevé et signé des problèmes de validation de blocs…

7

7 messages ont été scindés en un nouveau sujet : Verify block signature with duniterpy

8

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 :

  1. Le code de tweetnacl-js a été audité : GitHub - dchest/tweetnacl-js: Port of TweetNaCl cryptographic library to JavaScript
  2. 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).
  3. Ç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 :

  1. 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.
  2. Test du passage en V12 sur la g1-test uniquement.
  3. 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.
  4. 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 :slight_smile:

2 Likes
9

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 :slight_smile:

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.

4 Likes
10

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

1 Like
11

Oui tu as raison cette phrase ne sert à rien dsl, des fois j’enfonce des portes ouvertes par peur de ne pas être compris :sweat_smile:

100% d’accord :slight_smile:

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 :slight_smile:

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.

1 Like
12

J’ai passé le ticket relatif en mode confidentiel. C’est comme une CVE, mais non publiée.

1 Like
13

Ben sinon vu que tu as déjà bindé les 2 versions du dépôt tweetnacl-usable, tu n’as qu’à brancher la version « buguée » temporairement.

Ça permettrait d’éviter de perturber la Ğ1, mais aussi de passer trop de temps sur ce problème. Enfin c’est comme tu veux.

14

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 :slight_smile:

1 Like
15

J’ai également créé un ticket pour Duniter du coup, et me le suis affecté puisque je me propose de travailler dessus :slight_smile:

16

Comme tu veux.

Par contre je n’ai pas bien compris ça :

Tu signes sans clé privée ? :slight_smile:

17

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 ?)

1 Like
18

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 :sweat_smile:), 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 :slight_smile:

1 Like
19

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.

1 Like
20

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 :slight_smile:

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 :sweat_smile:

2 Likes