Salut tout le monde!
J'aime Tox et je respecte les participants à ce projet et leur travail. Dans le but d'aider les développeurs et les utilisateurs de Tox, j'ai examiné le code et j'ai remarqué des problèmes potentiels qui pourraient conduire à un faux sentiment de sécurité. Depuis que j'ai publié cet article en 2016 ( en russe ), de nombreuses améliorations ont été apportées à Tox, et je dirige une équipe qui a réécrit le logiciel Tox sécurisé à partir de zéro en utilisant le langage de programmation Rust (consultez Tox-rs ). Je recommande d'utiliser tox en 2019. Voyons ce qui nous a réellement fait réécrire Tox in Rust.
Article original de 2016
Il y a une tendance malsaine à surestimer la sécurité des systèmes E2E uniquement sur la base qu'ils sont E2E. Je vais présenter des faits objectifs complétés par mes propres commentaires pour que vous puissiez tirer vos propres conclusions.
Spoiler: Les développeurs de Tox sont d'accord avec mes points et ma demande d'extraction de code source a été acceptée.
Fait n ° 1. la branche principale échoue aux tests
Tout a commencé avec des articles sur Habr sur l'installation du nœud ( en russe ).
Dans les commentaires, les gens se sont plaints de la complexité de la construction et de l'installation du nœud sur CentOS, j'ai donc décidé d'écrire un système de construction sur CMake. Après quelques jours, j'étais prêt à présenter mon RP à la communauté Tox sur Freenode, mais j'ai été confronté à un manque de compréhension:
quelqu'un a contribué cmake au départ, mais d'autres développeurs ne savaient pas comment l'utiliser et ne pouvaient pas faire construire leur code, ils sont donc passés aux outils automatiques (sic!), qu'ils deviennent mieux informés maintenant.
J'ai remarqué que le code qui échoue aux tests dans Travis CI est toujours accepté dans la branche principale, mais ils ont répondu: "nous comprenons que nous devons faire quelque chose avec les tests, mais que ce soit pour le moment."
Ensuite, je me suis plongé dans une exploration plus approfondie du code de ce séduisant messager.
Fait n ° 2. memset (ptr, 0, taille) avant d'appeler gratuitement
Mon œil a attiré
memset(c, 0, sizeof(Net_Crypto)); free(c);
Si vous ne connaissez pas encore PVS-Studio et son article sur la fonction memset PVS-Studio: le compilateur peut supprimer l'appel de fonction 'memset' si cette région mémoire n'est pas utilisée par la suite. La logique du compilateur est simple: "Vous n'allez pas utiliser cette variable après avoir appelé 'free', memset n'affectera pas le comportement observé, permettez-moi de supprimer cet appel inutile à 'memset'".
En tant qu'étudiant assidu, j'ai remplacé chaque occurrence de memset($DST, 0, $SIZE) par sodium_memzero et les TESTS CRASHED.
Fait n ° 3. La comparaison des clés publiques est vulnérable aux attaques de synchronisation
Il existe une très bonne fonction spéciale pour comparer les clés publiques dans toxcore :
int public_key_cmp(const uint8_t *pk1, const uint8_t *pk2) { return crypto_verify_32(pk1, pk2); }
crypto_verify_32 - est une fonction de la bibliothèque de cryptographie NaCL / Sodium , qui peut vous aider à éviter les attaques de synchronisation, car elle fonctionne en temps constant, tandis que memcmp peut casser sur le premier octet inégal. Vous devez utiliser crypto_verify_32 pour comparer des données sensibles comme des clés.
Les comparaisons de chaînes effectuées octet par octet sont vulnérables à l'exploitation par des attaques de synchronisation, par exemple afin de forger des MAC (voir cette vulnérabilité dans la bibliothèque de chiffrement Keyczar de Google).
La base de code du projet toxcore est assez étendue, c'est pourquoi Tox est né avec un bug de vulnérabilité de synchronisation:
bool id_equal(const uint8_t *dest, const uint8_t *src) { return memcmp(dest, src, crypto_box_PUBLICKEYBYTES) == 0; }
Mais ce n'est pas tout. Les développeurs préfèrent toujours comparer les clés à leur manière en utilisant trois fonctions différentes: id_equal ou public_key_cmp et crypto_verify_32 .
Voici une courte sortie grep de DHT, du routage des oignons et d'autres sous-systèmes critiques:
if (memcmp(ping->to_ping[i].public_key, public_key, crypto_box_PUBLICKEYBYTES) == 0) { if (memcmp(public_key, onion_c->friends_list[i].real_public_key, crypto_box_PUBLICKEYBYTES) == 0) if (memcmp(public_key, onion_c->path_nodes_bs[i].public_key, crypto_box_PUBLICKEYBYTES) == 0) if (memcmp(dht_public_key, dht_public_key_temp, crypto_box_PUBLICKEYBYTES) != 0) if (Local_ip(ip_port.ip) && memcmp(friend_con->dht_temp_pk, public_key, crypto_box_PUBLICKEYBYTES) == 0)
Fait n ° 4. increment_nonce dans un temps non constant
void increment_nonce(uint8_t *nonce) { uint32_t i; for (i = crypto_box_NONCEBYTES; i != 0; --i) { ++nonce[i - 1]; if (nonce[i - 1] != 0) break;
Si de telles opérations impliquent des paramètres secrets, ces variations de synchronisation peuvent faire fuir certaines informations. Avec suffisamment de connaissances sur la mise en œuvre à portée de main, une analyse statistique minutieuse pourrait même conduire à la récupération totale des paramètres secrets.
Il y a une fonction spéciale dans Sodium pour incrémenter les nonces:
Un œuf de Pâques accidentellement ironique est que la fonction increment_nonce se trouve dans un fichier qui commence par les mots:
Ce code doit être parfait. Nous ne plaisantons pas avec le cryptage.
Examinons de plus près ce fichier parfait.
Fait n ° 5. Vous pouvez trouver des clés et des données privées dans la pile
Code gênant:
void encrypt_precompute(const uint8_t *public_key, const uint8_t *secret_key, uint8_t *enc_key) { crypto_box_beforenm(enc_key, public_key, secret_key);
encrypt_data_symmetric appelle crypto_box_detached_afternm de Nacl / Sodium, je ne mettrai pas tout le code, voici un lien à vérifier par vous-même.
Il semble difficile de se tromper en quatre lignes de code, non?
Examinons le sodium:
int crypto_box_detached(unsigned char *c, unsigned char *mac, const unsigned char *m, unsigned long long mlen, const unsigned char *n, const unsigned char *pk, const unsigned char *sk) { unsigned char k[crypto_box_BEFORENMBYTES]; int ret; (void) sizeof(int[crypto_box_BEFORENMBYTES >= crypto_secretbox_KEYBYTES ? 1 : -1]); if (crypto_box_beforenm(k, pk, sk) != 0) { return -1; } ret = crypto_box_detached_afternm(c, mac, m, mlen, n, k); sodium_memzero(k, sizeof k); return ret; }
Effacer tous les chèques que nous obtenons:
unsigned char k[crypto_box_BEFORENMBYTES]; int ret; crypto_box_beforenm(k, pk, sk); ret = crypto_box_detached_afternm(c, mac, m, mlen, n, k); sodium_memzero(k, sizeof k); return ret;
Cela vous semble familier? Oui! C'est un code de fonction encrypt_data légèrement modifié de toxcore, la seule différence est qu'ils ont oublié de nettoyer la clé de la pile avec sodium_memzero ... Et il y a aussi des erreurs dans: handle_TCP_handshake , handle_handshake , et peut-être ailleurs aussi.
Fait n ° 6. Les avertissements du compilateur sont pour les dummiez!
Les développeurs du projet toxcore nient catégoriquement la nécessité d'activer tous les avertissements du compilateur, ou ils ne les connaissent pas.
Fonctions inutilisées (je suis particulièrement satisfait des avertissements des tests):
../auto_tests/dht_test.c:351:12: warning: unused function 'test_addto_lists_ipv4' [-Wunused-function] START_TEST(test_addto_lists_ipv4) ^ ../auto_tests/dht_test.c:360:12: warning: unused function 'test_addto_lists_ipv6' [-Wunused-function] START_TEST(test_addto_lists_ipv6) ^ ../toxcore/TCP_server.c:1026:13: warning: unused function 'do_TCP_accept_new' [-Wunused-function] static void do_TCP_accept_new(TCP_Server *TCP_server) ^ ../toxcore/TCP_server.c:1110:13: warning: unused function 'do_TCP_incomming' [-Wunused-function] static void do_TCP_incomming(TCP_Server *TCP_server) ^ ../toxcore/TCP_server.c:1119:13: warning: unused function 'do_TCP_unconfirmed' [-Wunused-function] static void do_TCP_unconfirmed(TCP_Server *TCP_server) ^
../toxcore/Messenger.c:2040:28: warning: comparison of constant 256 with expression of type 'uint8_t' (aka 'unsigned char') is always false [-Wtautological-constant-out-of-range-compare] if (filenumber >= MAX_CONCURRENT_FILE_PIPES) ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~ ../toxcore/Messenger.c:2095:28: warning: comparison of constant 256 with expression of type 'uint8_t' (aka 'unsigned char') is always false [-Wtautological-constant-out-of-range-compare] if (filenumber >= MAX_CONCURRENT_FILE_PIPES) ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~ ../toxcore/Messenger.c:2110:28: warning: comparison of constant 256 with expression of type 'uint8_t' (aka 'unsigned char') is always false [-Wtautological-constant-out-of-range-compare] if (filenumber >= MAX_CONCURRENT_FILE_PIPES) ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
../auto_tests/TCP_test.c:205:24: warning: unsequenced modification and access to 'len' [-Wunsequenced] ck_assert_msg((len = recv(con->sock, data, length, 0)) == length, "wrong len %i\n", len); ^ ~~~ /usr/include/check.h:273:18: note: expanded from macro 'ck_assert_msg' _ck_assert_msg(expr, __FILE__, __LINE__,\ ^
Et quelques dizaines d'avertissements sur les variables inutilisées, la comparaison des signés et des non signés, et plus encore.
Ma conclusion
Citation du référentiel:
Nous voulons que Tox soit aussi simple que possible tout en restant aussi sécurisé que possible.
Si moi, un non-cryptographe, pouvais trouver de tels bogues horribles en une journée, imaginez combien de choses un spécialiste de la cryptographie pourrait trouver après avoir creusé exprès pour eux pendant un mois?.
Les premières versions du Tox représentaient un grand danger pour les utilisateurs qui comptent sur la sécurité de Tox. Les solutions propriétaires ne sont pas dignes de confiance et même les solutions open source ne sont pas aussi sûres que vous le souhaitez. Jetez un œil à la récente faille de sécurité dans Matrix . De nos jours, de nombreux bugs sont corrigés et Tox est la meilleure option disponible pour la sécurité et la confidentialité des utilisateurs.
La prochaine fois, je vous en dirai plus sur l'état actuel des tox-rs. Ce que nous avons implémenté dans Rust et pourquoi vous devriez l'essayer.
Reddit: commentaires