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

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.

1 J'aime

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

5 J'aimes

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

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

6 J'aimes

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.

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

2 J'aimes

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

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 ( :rofl: ), mais sur le principe, qu’en pensez-vous ?

2 J'aimes

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

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.

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

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

Exactement.

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

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.

:smiley: fais-donc ! Je surveille quand même :wink:

2 J'aimes

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.

1 J'aime

Un message a été scindé en un nouveau sujet : Lancer les tests automatisés sur un poste de développement

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

Essaye une synchronisation sur un autre nœud.

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.

2 J'aimes

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

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 ! :wink:

Merci pour vos dons :heart:

+---------------------+----------------------------------------------+-----------+------------------------------------------------+
|        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                                |
+---------------------+----------------------------------------------+-----------+------------------------------------------------+                                             
3 J'aimes

Oh oh, ca ressemble à un historique sous Silkaj ça ! :slight_smile:
Allez hop un petit bouton vite fait, pour faciliter les dons à @moul (en passant par son nœud, tant qu’à faire):

Merci Moul pour Duniter v1.7.16 !
7 J'aimes

Salut @vtexier !
j’ai effectivement fait ça, c’est une manip indiquée sur l’UI du desktop :wink:
et j’ai réussit à synchroniser, mais le réseau ne me reconnait que comme mirroir. enfin, c’est ce que je voit depuis mon client #duniter-desktop

Je trouve qu’on vois beaucoup moins d’erreurs de connexion WS2P depuis cette correction.

Je ne sais pas, par contre je constate qu’un nouveau ralentissement est en cours :confused:

Oui, je pense observer un fork entre les nœuds en < 1.7.16 et les ≤ 1.7.16.


Spécial ping ! Mettez vos nœuds à jour en 1.7.16 :

@nono2357, @dom, @Florentin, @jytou, @ofontes, @charles, @Vincent_Rousseau, @Pafzedog, @poka, @Pierre_Jean_CHANCELL, @PiNguyen, @Junidev, @oaktree, @matograine, @binuts.


J’ai créé un groupe d’utilisateur pour pinger d’une traite les utilisateurs qui ont déjà fait tourner un nœud.
@Blacksmith

6 J'aimes

Ou, sinon c’est les blocs contenants un nouveau membre qui font crier les nœuds (< 1.7.16) qui essayent d’ajouter cette fameuse certification (celle de lacoteau −> SebasC).