* Le second appel à MD_Update(), celui qui était dès le départ encadré par
un '#ifdef PURIFY' est totalement facultatif. Donc :
1) contrairement à ce qui a été écrit plus haut par iZnogood, un openssl
standard compilé avec -DPURIFY ne pause aucun problème . Et
2) on peut aisément comprendre qu'une lecture rapide/superficielle du code
ou du patch conduise à croire que le premier appel à MD_Update(), dans le
même fichier, puisse être traité de la même manière (ce qui n'est en fait pas le cas).
* Kurt (le développeur Debian à l'origine du patch problématique) a soumis son
patch pour revue sur la liste de diffusion openssl-dev[1], ce qui n'a pas
suscité de mise en gardes (mais l'upstream n'a pas choisi d'intégrer ce
patch, et pour cause). Cette façon d'échanger avec l'upstream présentait
cependant plusieurs graves défauts, comme le signale d'ailleurs Damien Miller
(le développeur principal d'OpenSSH)[1] :
1) openssl-dev est en pratique une liste d'utilisateur d'openssl (ou de
développeurs d'applications utilisant openssl), et pas la liste des
développeurs de la lib openssl (openssl-team), et encore moins le bugtracker.
Seulement la description de la liste sur le site d'openssl ne reflète pas ce
fait.
2) Il n'a pas du tout précisé qu'il était mainteneur du paquet debian et
qu'il comptait y intégrer ce patch et le distribuer à tout les utilisateurs.
Vu que son mail parle seulement de problème avec valgrind (utilisation de la
lib en condition de debug), et sur une m-l pour les devs de programmes
utilisateurs, son messsage a vraissemblablement été interprété comme
"salut, je n'arrive pas à valgrinder mon appli sur mon poste perso parce
qu'elle utilise openssl, est-ce que ce quick hack peut me tirer d'affaire ?"
3) Ce qu'il a envoyé sur la mailing-list n'est pas un patch, mais une
question (ce qui n'aidait pas à deviner ses réelles intentions).
4) S'il s'était donné la peine de googler "valgrind openssl", ou simplement
de chercher dans le bugtracker d'openssl, il aurait vu, des tonnes de fois,
la réponse des devs à ce problème[3].
* Certains (cf. par ex. Sytoka Modon plus haut) semblent avoir mal compris
pourquoi cette fonction peut utiliser de la mémoire non initialisée. Si
openssl fait brailler valgrind, ce n'est pas que leur code est sale. Et non,
openssl ne compte pas sur cette zone de mémoire pour vraiment contenir un
aléas solide et suffisant. La fonction :
"static void ssleay_rand_add(const void *buf, int num, double add)" ,
celle dont les appels à "MD_Update(&m,buf,j);" ont été commentés par Debian,
est une fonction interne d'openssl appellée par la fonction "publique"
(documentée et exposée aux utilisateurs de la lib) :
"int RAND_bytes(unsigned char *buf, int num);".
La page de man de RAND_bytes(3ssl) explique clairement :
"The contents of buf is mixed into the entropy pool before retrieving the
new pseudo-random bytes unless disabled at compile time (see FAQ)."
Bref, c'est un comportement parfaitement documenté, le buffer "buf" (transmis
depuis l'application utilisatrice jusqu'à MD_Update()) sera accédé en
lecture, et pas seulement en écriture.
Si l'utilisateur le souhaite, il peut "pré-remplir" le buffer "buf" avec des
données venant de /dev/random, ou autre, avant d'appeler RAND_bytes(), et ainsi
augmenter à souhait l'entropie du système tout en faisant taire valgrind. Le cas
échéant, openssl lit quand même la zone mémoire "buf" (parce que MD_Update()
ne sait pas si elle a été initialisée de la sorte, et parce que même si elle
ne l'a pas été, elle peut contenir n'importe quoi, ce qui est toujours un
petit poil d'aléas bon à prendre même si non vital). Ensuite, le MD_Update()
mixe le contenu de "buf" avec du vrai aléas obtenu de façon dépendante de la
plateforme (/dev/urandom /dev/arc4random, etc.) : on ne dépend pas du fait
que le contenu, initialisé ou pas, dans "buf" soit réellement aléatoire ou pas.
Mais on dépend du fait que l'appel à MD_Update() soit bien éffectué, sinon
ce mélange de buf et de vrai aléas n'a pas lieu. Ceci explique pourquoi une
solution telle que : "memset(buf, 0, sizeof buf); MD_Update(&m,buf,j);",
qui aurait fait taire valgrind, aurait plutôt diminué la qualité du code
et de l'entropie. Le probleme ici est que le développeur Debian a purement
et simplement enlevé l'appel à MD_Update().
* Debian et Ubuntu distribuent désormais une versions d'openssh patchée pour
rejeter les clefs fragiles, et contenant un nouvel outil (ssh-vulnkeys)
censé aider à trouver les clefs fragiles sur le système. Il sera
malheureusement nécéssaire que les autres distribs (et les *BSD, et Solaris,
etc.) fassent de même *très rapidement*. Et il serait de bon ton de la part de
ces deux distributions responsables d'y aider, et aussi de distribuer l'openssh
patché pour Sarge et Ubuntu 6.06 LTS (arrétons l'hypocrisie et les résolutions
psychorigides : même si ces deux distribs ne sont plus officiellement supportées
depuis peu (un ou deux mois), elles sont encore utilisées).
* Complètement ahurissant et irresponsable : le correctif sécurité Debian a été
commité, publiquement 5 jours avant la publication de l'advisory ![4]
Pourtant, même la doc officielle pour les développeurs Debian rappelle
l'évidence, qu'il ne faut pas faire ça.[5]
### Aussi, pour éviter que ça se reproduise, on peux facilement en déduire que :
* Les mainteneurs de paquets dans les distributions devraient toujours
soumettre leurs patchs à l'upstream, *en n'omettant pas d'indiquer qu'ils
sont maiteneurs du paquet et ce qu'ils comptent faire du patch* (qu'ils
comptent l'intégrer dans le paquet, qu'il ne s'agit pas d'un simple
quick hack sur leur poste perso pour pouvoir mieux débugguer une appli
en cours de développment avec valgrind).
* Comme le rappelle Ben Laurie[6] (dev openssl), un mainteneur de paquet ne
devrait jamais distribuer sa correction d'un problème qu'il ne comprends pas
(une résolution "à tatons") ou dans un segment de code qu'il ne comprends
pas. Surtout si ce patch touche une composant central d'une bibliothèque
vitale pour la sécurité de l'OS et très utilisée.
* Les outils automatiques de fiabilisation (de valgrind à SELinux en passant
par snort ou gcc -fstack-protector) ne peuvent rien pour prévenir ce type de
bourdes ou leurs conséquences. Autrement dit, retenons qu'ils ne remplaceront
jamais la nécessité d'une relecture et validation humaine systématique du code
par un pair "expert". Le bon vieux modèle "audit et relecture systématique par
un ancien" à la façon OpenBSD a du bon (ce qui ne signifie pas que le modèle
"prévention automatisée à la SELinux/Red Hat est moins bon, mais le premier
reste nécessaire). Debian devrait imposer la validation des patchs, au moins
sur les composants critiques.
* Le problème aurait aussi été évité si le développeur Debian avait regardé le
bugtracker d'openssl (où il a déjà été question de son problème) - et je
considère que regarder ce bugtracker est son boulot ; il aurait aussi été
évité si les développeurs openssl suivaient les "vendor patchs" dans les
distros, ou simplement les bugtrackrs des distros. Il y a de grosses marges
de progrès là (et des "meta" bugtrackers pourraient aider).
* Documenter les pièges de ce genre dans la code (un simple commentaire...) ne
mange pas de pain. Même si c'est documenté dans la page de man, le
bugtracker et sur la mailing-list (via goole).
# Voici pourquoi openssl lit de la mémoire non initialisée (et ce n'est
Posté par herodiade . En réponse à la dépêche Découverte d'une faille de sécurité critique dans OpenSSL de Debian. Évalué à 10.
un '#ifdef PURIFY' est totalement facultatif. Donc :
1) contrairement à ce qui a été écrit plus haut par iZnogood, un openssl
standard compilé avec -DPURIFY ne pause aucun problème . Et
2) on peut aisément comprendre qu'une lecture rapide/superficielle du code
ou du patch conduise à croire que le premier appel à MD_Update(), dans le
même fichier, puisse être traité de la même manière (ce qui n'est en fait pas le cas).
* Kurt (le développeur Debian à l'origine du patch problématique) a soumis son
patch pour revue sur la liste de diffusion openssl-dev[1], ce qui n'a pas
suscité de mise en gardes (mais l'upstream n'a pas choisi d'intégrer ce
patch, et pour cause). Cette façon d'échanger avec l'upstream présentait
cependant plusieurs graves défauts, comme le signale d'ailleurs Damien Miller
(le développeur principal d'OpenSSH)[1] :
1) openssl-dev est en pratique une liste d'utilisateur d'openssl (ou de
développeurs d'applications utilisant openssl), et pas la liste des
développeurs de la lib openssl (openssl-team), et encore moins le bugtracker.
Seulement la description de la liste sur le site d'openssl ne reflète pas ce
fait.
2) Il n'a pas du tout précisé qu'il était mainteneur du paquet debian et
qu'il comptait y intégrer ce patch et le distribuer à tout les utilisateurs.
Vu que son mail parle seulement de problème avec valgrind (utilisation de la
lib en condition de debug), et sur une m-l pour les devs de programmes
utilisateurs, son messsage a vraissemblablement été interprété comme
"salut, je n'arrive pas à valgrinder mon appli sur mon poste perso parce
qu'elle utilise openssl, est-ce que ce quick hack peut me tirer d'affaire ?"
3) Ce qu'il a envoyé sur la mailing-list n'est pas un patch, mais une
question (ce qui n'aidait pas à deviner ses réelles intentions).
4) S'il s'était donné la peine de googler "valgrind openssl", ou simplement
de chercher dans le bugtracker d'openssl, il aurait vu, des tonnes de fois,
la réponse des devs à ce problème[3].
* Certains (cf. par ex. Sytoka Modon plus haut) semblent avoir mal compris
pourquoi cette fonction peut utiliser de la mémoire non initialisée. Si
openssl fait brailler valgrind, ce n'est pas que leur code est sale. Et non,
openssl ne compte pas sur cette zone de mémoire pour vraiment contenir un
aléas solide et suffisant. La fonction :
"static void ssleay_rand_add(const void *buf, int num, double add)" ,
celle dont les appels à "MD_Update(&m,buf,j);" ont été commentés par Debian,
est une fonction interne d'openssl appellée par la fonction "publique"
(documentée et exposée aux utilisateurs de la lib) :
"int RAND_bytes(unsigned char *buf, int num);".
La page de man de RAND_bytes(3ssl) explique clairement :
"The contents of buf is mixed into the entropy pool before retrieving the
new pseudo-random bytes unless disabled at compile time (see FAQ)."
Bref, c'est un comportement parfaitement documenté, le buffer "buf" (transmis
depuis l'application utilisatrice jusqu'à MD_Update()) sera accédé en
lecture, et pas seulement en écriture.
Si l'utilisateur le souhaite, il peut "pré-remplir" le buffer "buf" avec des
données venant de /dev/random, ou autre, avant d'appeler RAND_bytes(), et ainsi
augmenter à souhait l'entropie du système tout en faisant taire valgrind. Le cas
échéant, openssl lit quand même la zone mémoire "buf" (parce que MD_Update()
ne sait pas si elle a été initialisée de la sorte, et parce que même si elle
ne l'a pas été, elle peut contenir n'importe quoi, ce qui est toujours un
petit poil d'aléas bon à prendre même si non vital). Ensuite, le MD_Update()
mixe le contenu de "buf" avec du vrai aléas obtenu de façon dépendante de la
plateforme (/dev/urandom /dev/arc4random, etc.) : on ne dépend pas du fait
que le contenu, initialisé ou pas, dans "buf" soit réellement aléatoire ou pas.
Mais on dépend du fait que l'appel à MD_Update() soit bien éffectué, sinon
ce mélange de buf et de vrai aléas n'a pas lieu. Ceci explique pourquoi une
solution telle que : "memset(buf, 0, sizeof buf); MD_Update(&m,buf,j);",
qui aurait fait taire valgrind, aurait plutôt diminué la qualité du code
et de l'entropie. Le probleme ici est que le développeur Debian a purement
et simplement enlevé l'appel à MD_Update().
* Debian et Ubuntu distribuent désormais une versions d'openssh patchée pour
rejeter les clefs fragiles, et contenant un nouvel outil (ssh-vulnkeys)
censé aider à trouver les clefs fragiles sur le système. Il sera
malheureusement nécéssaire que les autres distribs (et les *BSD, et Solaris,
etc.) fassent de même *très rapidement*. Et il serait de bon ton de la part de
ces deux distributions responsables d'y aider, et aussi de distribuer l'openssh
patché pour Sarge et Ubuntu 6.06 LTS (arrétons l'hypocrisie et les résolutions
psychorigides : même si ces deux distribs ne sont plus officiellement supportées
depuis peu (un ou deux mois), elles sont encore utilisées).
* Complètement ahurissant et irresponsable : le correctif sécurité Debian a été
commité, publiquement 5 jours avant la publication de l'advisory ![4]
Pourtant, même la doc officielle pour les développeurs Debian rappelle
l'évidence, qu'il ne faut pas faire ça.[5]
### Aussi, pour éviter que ça se reproduise, on peux facilement en déduire que :
* Les mainteneurs de paquets dans les distributions devraient toujours
soumettre leurs patchs à l'upstream, *en n'omettant pas d'indiquer qu'ils
sont maiteneurs du paquet et ce qu'ils comptent faire du patch* (qu'ils
comptent l'intégrer dans le paquet, qu'il ne s'agit pas d'un simple
quick hack sur leur poste perso pour pouvoir mieux débugguer une appli
en cours de développment avec valgrind).
* Comme le rappelle Ben Laurie[6] (dev openssl), un mainteneur de paquet ne
devrait jamais distribuer sa correction d'un problème qu'il ne comprends pas
(une résolution "à tatons") ou dans un segment de code qu'il ne comprends
pas. Surtout si ce patch touche une composant central d'une bibliothèque
vitale pour la sécurité de l'OS et très utilisée.
* Les outils automatiques de fiabilisation (de valgrind à SELinux en passant
par snort ou gcc -fstack-protector) ne peuvent rien pour prévenir ce type de
bourdes ou leurs conséquences. Autrement dit, retenons qu'ils ne remplaceront
jamais la nécessité d'une relecture et validation humaine systématique du code
par un pair "expert". Le bon vieux modèle "audit et relecture systématique par
un ancien" à la façon OpenBSD a du bon (ce qui ne signifie pas que le modèle
"prévention automatisée à la SELinux/Red Hat est moins bon, mais le premier
reste nécessaire). Debian devrait imposer la validation des patchs, au moins
sur les composants critiques.
* Le problème aurait aussi été évité si le développeur Debian avait regardé le
bugtracker d'openssl (où il a déjà été question de son problème) - et je
considère que regarder ce bugtracker est son boulot ; il aurait aussi été
évité si les développeurs openssl suivaient les "vendor patchs" dans les
distros, ou simplement les bugtrackrs des distros. Il y a de grosses marges
de progrès là (et des "meta" bugtrackers pourraient aider).
* Documenter les pièges de ce genre dans la code (un simple commentaire...) ne
mange pas de pain. Même si c'est documenté dans la page de man, le
bugtracker et sur la mailing-list (via goole).
[1] Post de Kurt sur openssl-dev : http://marc.info/?l=openssl-dev&m=114651085826293&w=(...)
[2] Remarque de Damien Miller : http://marc.info/?l=openbsd-misc&m=121088162320794&w(...)
[3] Par exemple ici, en 2003 http://rt.openssl.org/Ticket/Display.html?id=521&user=gu(...)
[4] http://svn.debian.org/viewsvn/pkg-openssl/openssl/trunk/cryp(...)
[5] http://www.debian.org/doc/developers-reference/ch-pkgs.en.ht(...)
[6] Ben Laurie : http://www.links.org/?p=327