Duniter v1.7.16 : Bug à l’ajout d’une certification d’un non membre dans le bloc à calculer

[Moul]

Pour les plus pressés :

Télécharger Duniter 1.7.16

---

Bon, il se trouve que j’ai trouvé la source d’un bug dans Duniter.

C’est la raison pour laquelle la Ğ1 a un retard de pas moins de trois heures.

Ce genre de journaux m’ont emmené à la source :

2019-04-07T23:14:15+02:00 - error:  Error: ruleCertificationFromMember
    at Function.checkBlock (/home/moul/duniter/app/lib/blockchain/DuniterBlockchain.js:123:19)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

Il se trouve qu’il y a une certification de lecoteau vers SebasC qui traine dans les piscines de certains nœuds. Les nœuds qui n’ont pas cette certification en piscine trouvent des blocs valides.
Mais les autres nœuds, qui ont cette certification, n’écrivent pas de blocs valides.

Car…, lecoteau n’est plus membre depuis le 4 avril 2019.

Il a émit son certificat le 22 mars. Il ne s’agit pas d’un bug client. Même si des clients pouvaient envoyer des certificats même pour une identité non membre, c’est aux nœuds de vérifier ça.

Or, Duniter, ajoute quand même cette certification dans le bloc. Puis, le bloc se voit refusé par lui-même et les autres nœuds, car il ne respecte pas la règle suivante : une certification est inscriptible uniquement pour une identité membre

Duniter vérifie bien que l’identité émettrice est bien membre avant d’ajouter cette certification au bloc à générer, par contre cette vérification est buggé et la condition passe. lecoteau est considéré membre

Pour le correctif, j’ai copié la méthode juste en dessous qui semble bien faire les choses. Je suis étonné que cette méthode ait bien fonctionné jusqu’à maintenant et que ça n’est pas explosé plus tôt. La gestion d’erreur devait surement bien fonctionner.

Désolé pour le blâme, mais voici le commit incriminé qui introduit le bug que je corrige.

---

Voilà, je vous livre cette analyse et ce correctif et cette v1.7.16 après y avoir passé l’après-midi et la soirée au lieu de les passer sur Silkaj.

Par ici les junes (désolé, pas trouvé d’émoticone sans UNL ) pour l’analyse et la proposition de correctif. Un petit don ne serait pas de refus. C’est quand même peu ces 20 DU par mois.

Hop, bloc numéro 210 172 généré et accepté par le réseau avec un nœud possédant cette certification et ce correctif.

[elois]

Putain mais bien sur, c’est trivial en plus, l’index des identités peut contenir des identités qui ne sont plus membres

Dsl a l’époque je ne maîtrisais pas encore bien les indexs de Duniter (c’était il y a 1 an et demi)

Aussi le Node et ses appels asynchrones partout me rend très mal a l’aise, j’ai jamais réussi a m’y retrouver dans ce langage (c’est pas faute d’avoir essayé), c’est en partie ce pourquoi je ne contribue plus au code de Duniter.

@Moul ce serait peut etre l’ocassion de rédiger un TU pour vérouiller le comportement

[Moul]

Je vous livre la v1.7.16 toute chaude sur un plat .

Mettez vos nœuds à jour !

[Moul]

@elois

ce serait peut etre l’ocassion de rédiger un TU pour vérouiller le comportement

Bonne idée, je vais tenter de m’y atteler demain.

[elois]

@Moul

Je suis étonné que cette méthode ait bien fonctionné jusqu’à maintenant et que ça n’est pas explosé plus tôt.

Ça ne pouvait se produire que lorsqu’un compte membre émet des certifications puis expire puis que l’une de ces certifications émises est en mesure d’être écrite.
Ça n’était encore jamais arrivé jusque la car les membres qui expirent sont des membres inactifs donc qui ne certifient pas.
Le membre en question (lecoteau) doit être un membre actif mais qui ne sait pas qu’il faut se renouveler tout les ans et qui n’a pas été notifié de son expiration a venir sur son cesium. En gros c’est les notif de cesium qui nous prémunissais de ce bug dormant.

[kimamila]

Saurais dire @moul si la v1.6 était impactée pour l’erreur dans le code ? (Je cherche la cause des noeuds v1.6 qui partent toujours sur un fork)

Grand merci pour le correctif !

[Moul]

Ce bug a été publié dans la 1.6.15 le 19 janvier 2018. Ce bug a vécu au moins quatorze mois.
L’ampleur du bug n’est pas anodine sans être folle pour autant. Voici tous les endroits impactés par cette commande pratique de vérification pour savoir si une identité est membre ou non :

grep -rni "dal\.isMember(" **/*.ts
app/lib/blockchain/DuniterBlockchain.ts:175:    const isMember = await dal.isMember(block.issuer);
app/lib/streams/router.ts:105:        let isMember = await this.dal.isMember(p.pubkey);
app/modules/prover/lib/blockGenerator.ts:347:            let isMember = await this.dal.isMember(cert.from)
app/modules/prover/lib/blockGenerator.ts:422:            const isMember = await this.dal.isMember(cert.from);
app/modules/prover/lib/blockGenerator.ts:768:                const isSignatoryAMember = await this.dal.isMember(cert.from);
app/modules/prover/lib/permanentProver.ts:88:          const isMember = await dal.isMember(selfPubkey);
app/modules/ws2p/lib/WS2PCluster.ts:248:      isMember = !!(await this.server.dal.isMember(pub))
app/modules/ws2p/lib/WS2PCluster.ts:509:            return (this.server.dal.isMember(this.server.conf.pair.pub)) ? freeMemberRoom:freeMirorRoom
app/modules/ws2p/lib/WS2PCluster.ts:749:    const isMember = await this.server.dal.isMember(pubkey)
app/modules/ws2p/lib/WS2PCluster.ts:826:    const isMemberPeer = await this.server.dal.isMember(pub)
app/service/BlockchainService.ts:437:    return this.dal.isMember(this.selfPubkey)
app/service/MembershipService.ts:64:      const isMember = await this.dal.isMember(entry.issuer);
app/service/PeeringService.ts:128:              const isMember = await this.dal.isMember(this.conf.pair.pub)

Point pas point, je vais lister les potentielles failles ligne par ligne.

---

app/lib/blockchain/DuniterBlockchain.ts:175:    const isMember = await dal.isMember(block.issuer);

• Majeur : bloc généré par un non membre accepté par un nœud. Surement la raison pour laquelle Remuniter affichait des blocs de non membres. D’autres règles devaient empêcher que le réseau suive ce bloc.

---

app/lib/streams/router.ts:105:        let isMember = await this.dal.isMember(p.pubkey);

• ~Mineur (je ne suis pas sûr de comprendre l’ampleur) : fiche de paire réseau d’un nœud UP (BMA ou WS2P) ajoutée à une liste membre ou une liste non membre.

---

app/modules/prover/lib/blockGenerator.ts:347:            let isMember = await this.dal.isMember(cert.from)
app/modules/prover/lib/blockGenerator.ts:422:            const isMember = await this.dal.isMember(cert.from);
app/modules/prover/lib/blockGenerator.ts:768:                const isSignatoryAMember = await this.dal.isMember(cert.from);

• Majeur : ajout de certifications pour une identité devenant membre. Vérification que les émetteurs de certifications sont membres.
• ??? : pas sûr de comprendre ce que ce code fait.
• Majeur : corrigé dans ce post. Source de découverte de ce bug.

---

app/modules/ws2p/lib/WS2PCluster.ts:248:      isMember = !!(await this.server.dal.isMember(pub))
app/modules/ws2p/lib/WS2PCluster.ts:509:            return (this.server.dal.isMember(this.server.conf.pair.pub)) ? freeMemberRoom:freeMirorRoom
app/modules/ws2p/lib/WS2PCluster.ts:749:    const isMember = await this.server.dal.isMember(pubkey)
app/modules/ws2p/lib/WS2PCluster.ts:826:    const isMemberPeer = await this.server.dal.isMember(pub)

Ampleur difficile à juger. Surement pas de gros bug, mais de potentielles failles de sécurité réseau WS2P.

• gestion fiches de paires WS2P
• émission d’emplacements (freeroom) de connexion WS2P privées disponibles qu’un nœud membre propose, mais pas un nœud non membre.
• Priorité plus élevée pour gérer la mise en relation des nœuds via WS2P
• code non utilisé, peu être retiré. La fonctionnalité semble avoir été migré sur la ligne du dessus.

En gros, un nœud non membre pouvait théoriquement se comporter comme un nœud membre d’un point de vue gestion de la couche réseau WS2P.

---

app/service/BlockchainService.ts:437:    return this.dal.isMember(this.selfPubkey)

• code non utilisé, à retirer.

---

app/service/MembershipService.ts:64:      const isMember = await this.dal.isMember(entry.issuer);

• devait quitter cette fonction avec ce message d’erreur ucode: 2008, message: "A non-member cannot leave" Donc, des identités non membres étaient sauvegardées en base de donnés, alors que ça n’aurait pas dû être fait.

---

app/service/PeeringService.ts:128:              const isMember = await this.dal.isMember(this.conf.pair.pub)

• mineur : accepte les fiches de paire malgré tout

---

Je m’occupe de retirer le code plus utiliser. C’est moi qui les ait trouvés.

[Moul]

J’ai fait entrer le mille-huit-centièmes membre grâce à mon correctif \o/

[cgeek]

@Moul

Duniter vérifie bien que l’identité émettrice est bien membre avant d’ajouter cette certification au bloc à générer, par contre cette vérification est buggé et la condition passe.

Plus précisément : le code qui vérifie que l’émetteur est membre n’est pas le même selon qu’il s’agit de la génération d’un bloc ou de sa vérification. Celui de la vérification de bloc fonctionne bien, tandis que le code de génération de bloc que tu as corrigé était différent et incorrect.

Petit laïus à ce sujet : on peut voir ici que le code de protocole était bien respecté, ce qui n’a pas empêché un bug critique d’être présent dans le « meta-code » qui enrobe le protocole, ici celui de génération de bloc.

En tout cas @Moul : bien joué pour ta trouvaille et le correctif associé, ainsi que la livraison qui a suivi. Il manque juste un test automatisé pour verrouiller ce comportement. Comme pour @bpresles, je te fais un don personnel de 100 DU car vraiment vous me faites non-seulement gagner un temps que je n’ai même pas, mais en plus vous permettez à la Ğ1 de poursuivre sa route sans moi, et ça j’en suis très heureux

Par contre je vais diminuer les dons la prochaine fois car si vous continuez je vais vite être à sec

[cgeek]

@elois

Dsl a l’époque je ne maîtrisais pas encore bien les indexs de Duniter (c’était il y a 1 an et demi)

Ce n’est certainement pas ta faute, le problème c’est surtout qu’il n’existait pas de test automatisé qui couvrait ce cas. D’ailleurs encore aujourd’hui, il suffit d’un refactoring et ce bug peut se reproduire. Donc, ticket#1354.

@elois

c’est en partie ce pourquoi je ne contribue plus au code de Duniter

Ce n’est pas une bonne raison mais tu en as d’autres.

[Inso]

Nordstrom s’est mis à jour tout seul quand j’ai push sur le repo mes avancées https://g1.nordstrom.duniter.org/

@cgeek

Par contre je vais diminuer les dons la prochaine fois car si vous continuez je vais vite être à sec

Sinon, pourquoi pas mettre en place un bug bounty rémunéré par la caisse de développeurs de Duniter ?

Bon faudrait pas que ça devienne un incitation à introduire des bugs pour pouvoir en corriger ensuite ( ), mais sur le principe, qu’en pensez-vous ?

[cgeek]

@Moul

D’autres règles devaient empêcher que le réseau suive ce bloc.

Ce code est spécifique au bloc zéro. Il est désormais inutile pour la Ğ1, mais nous pouvons le laisser car ce n’est pas non plus du code mort (surtout si demain d’autres veulent lancer leur propre monnaie).

@Moul

~Mineur (je ne suis pas sûr de comprendre l’ampleur) : fiche de paire réseau d’un nœud UP (BMA ou WS2P) ajoutée à une liste membre ou une liste non membre.

C’est du routing BMA. Quand un nœud reçoit un nouveau document (peu importe son origine, WS2P, BMA, ou le serveur lui-même [cas d’un bloc forgé]), celui-ci le propage à d’autres nœuds BMA. Cette propagation se fait à un maximum de nœuds membres et à quelques nœuds miroir.

Pas trop gênant ici.

@Moul

Majeur : ajout de certifications pour une identité devenant membre. Vérification que les émetteurs de certifications sont membres.

Oui. Besoin d’un test (ticket#1355).

@Moul

??? : pas sûr de comprendre ce que ce code fait.

Comme au-dessus. On contrôle que l’émetteur est membre pour une certification externe. Majeur. Besoin d’un test (ticket#1355 aussi).

@Moul

En gros, un nœud non membre pouvait théoriquement se comporter comme un nœud membre d’un point de vue gestion de la couche réseau WS2P.

Exactement.

@Moul

code non utilisé, à retirer.

Bien vu et il faut retirer les méthodes appelantes, jusqu’à celle non utilisée de plus haut niveau. Ticket#1356.

@Moul

Donc, des identités non membres étaient sauvegardées en base de donnés, alors que ça n’aurait pas dû être fait.

Non, il s’agit simplement des adhésions de sortie (OUT) qui auraient pu être enregistrées en piscine si un ancien membre en avait envoyé une. D’ailleurs c’est un cas à tester si ce n’est pas déjà fait. Ticket#1357.

Côté sécurité, pas de faille car la méthode findLeavers voit ensuite l’identité testée sur le champ .member. Donc, pas de soucis. Mais encore une fois, un test serait le bienvenue pour verrouiller ce comportement.

@Moul

C’est moi qui les ait trouvés.

fais-donc ! Je surveille quand même

[cgeek]

Je suis plutôt pour, en valorisant le ticket avec un niveau afin que le bounty soit fonction de la difficulté / de l’intérêt porté à sa réalisation.

Par exemple il y a des tickets difficiles mais pas prioritaires, et des tickets simples mais à grosse valeur ajoutée comme les tests automatisés verrous.

[Candidesk8]

Bonjour,

hier, j’ai téléchargé la desktop 1.7.15 et la synchro à bloqué à 99 % de Apply ( la mise en mode veille auto de ubuntu 18.04.1 y est-elle pour qqc ? )

Aujourd’hui, je télécharge la desktop 1.7.16, pas de synchro proposée, le noeud se met à “fonctionner” mais je ne trouve qu’un seul peer et voici mes logs :
http://hastebin.com/ibowuleqal

Est-ce que c’est normal ?
Suis-je connecté au bon peer?
Si cela n’est pas le cas, comment se connecter à d’autres noeuds ?

Merci pour vos retours

[Moul]

Essaye une synchronisation sur un autre nœud.

[Moul]

Voici un beau réseau qui rattrape son retard :

Current block: n°210275, generated on the 2019-04-07 15:52:34
Generation of next block n°210276 possible by at least 34/49 members
Common Proof-of-Work difficulty level: 76, hash starting with `0000[0-3]*`
|        uid        |        match         |   Π diffi    |   Σ diffi |
|-------------------+----------------------+--------------+-----------|
|     gerard94      | 00000000000000000000 | 1.7 × 10^181 |      2404 |
|     vincentux     | 00000000000000000000 | 4.1 × 10^90  |      1202 |
|        Pol        | 00000000000000000000 | 3.9 × 10^56  |       752 |
|       moul        | 00000000000000000000 | 5.7 × 10^45  |       608 |
|        Dom        | 00000000000000000000 | 3.1 × 10^34  |       454 |
|       cgeek       | 00000000000000000000 | 5.0 × 10^28  |       378 |
|      charles      | 0000000000000000000[ | 2.3 × 10^23  |       307 |
|      vincent      | 000000000000000000[0 | 5.7 × 10^22  |       300 |
|      nicoleC      | 00000000000000[0-A]* | 3.6 × 10^17  |       229 |
|      Muisec       | 00000000000000[0-E]* | 7.2 × 10^16  |       225 |
|      ji_emme      |   000000000[0-3]*    | 8.2 × 10^11  |       156 |
|       elois       |   000000000[0-4]*    | 7.6 × 10^11  |       155 |
|    SimonLefort    |   000000000[0-4]*    | 7.6 × 10^11  |       155 |
|        vit        |   000000000[0-5]*    | 6.9 × 10^11  |       154 |
|      Fabwice      |   000000000[0-7]*    | 5.5 × 10^11  |       152 |
|     Mententon     |   000000000[0-8]*    | 4.8 × 10^11  |       151 |
|       deem        |     00000[0-E]*      |  1.0 × 10^6  |        81 |
|     Granxis8      |        00000*        |  1.0 × 10^6  |        80 |
|     Framasky      |        00000*        |  1.0 × 10^6  |        80 |
|  MarcelDoppagne   |        00000*        |  1.0 × 10^6  |        80 |
|     b_presles     |        00000*        |  1.0 × 10^6  |        80 |
|       jytou       |        00000*        |  1.0 × 10^6  |        80 |
|    Fredlassave    |        00000*        |  1.0 × 10^6  |        80 |
|       LenaB       |      0000[0-1]*      |  9.2 × 10^5  |        78 |
|     nono2357      |      0000[0-1]*      |  9.2 × 10^5  |        78 |
|    DustyFellow    |      0000[0-2]*      |  8.5 × 10^5  |        77 |
|      Attilax      |      0000[0-2]*      |  8.5 × 10^5  |        77 |
|     pafzedog      |      0000[0-3]*      |  7.9 × 10^5  |        76 |
|      binuts       |      0000[0-3]*      |  7.9 × 10^5  |        76 |
|      paidge       |      0000[0-3]*      |  7.9 × 10^5  |        76 |
|      oaktree      |      0000[0-3]*      |  7.9 × 10^5  |        76 |
|      ofontes      |      0000[0-3]*      |  7.9 × 10^5  |        76 |
|     BnimajneB     |      0000[0-4]*      |  7.2 × 10^5  |        75 |
| jeanlucdonnadieu  |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|    mathieuBize    |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|    OlivLutinus    |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|      Scott76      |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|       Nadou       |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|      Maxoud       |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|      Petrus       |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|       inso        |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|     PiNguyen      |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|      Vivakvo      |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|     Spartacus     |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|      Jokeur       |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|       FredB       |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|     DamageCo      |      0000[0-4]*      |  7.2 × 10^5  |        75 |
|      jardin       |      0000[0-4]*      |  7.2 × 10^5  |        75 |
| AldelaideDoppagne |      0000[0-4]*      |  7.2 × 10^5  |        75 |

Il reste une heure et demie à rattraper. Et la difficulté a chuté à 75.
Je pense aussi qu’on a battu le record d’identités dans la fenêtre courante : 49.

[cgeek]

Va pas falloir que la difficulté baisse trop non plus, là 75 ça devient limite avec autant de nœuds dispos !

[vit]

J’ai eu le même problème, j’ai supprimé tout les fichiers/dossiers dans duniter_default sauf config.json.
Il a fallut plusieurs tentatives en changeant de nœud avant de réussir à synchroniser complètement.
Persévère !

[Moul]

Merci pour vos dons

+---------------------+----------------------------------------------+-----------+------------------------------------------------+
|        Date         |                   Issuers                    |  Amount   |                    Comment                     |
+=====================+==============================================+===========+================================================+
| 2019-04-07 22:59:00 | D9D2zaJoWYWveii1JRYLVK3J4Z7ZH3QczoKrnQeiM6mx | 100       | merci pour le debug de duniter et du process   |
|                     |                                              |           | de release aussi                               |
+---------------------+----------------------------------------------+-----------+------------------------------------------------+
| 2019-04-07 21:51:41 | CmFKubyqbmJWbhyH2eEPVSSs4H4NeXGDfrETzEnRFtPd | 100.700   | V1.7.16                                        |
+---------------------+----------------------------------------------+-----------+------------------------------------------------+
| 2019-04-07 01:18:44 | 2ny7YAdmzReQxAayyJZsyVYwYhVyax2thKcGknmQy5nQ | 1007      | Duniter v1.7.16                                |
+---------------------+----------------------------------------------+-----------+------------------------------------------------+