Всем привет!
Мне нравится Tox, я уважаю участников этого проекта и их труд, который иногда даже удается использовать по назначению. В стремлении помочь сообществу, я заглянул в код, заметил потенциальные проблемы, которые могут привести людей в погонах к вам домой к весьма печальным последствиям.
В последнее время есть нездоровая тенденция переоценивать защищенность подобных систем только на основании того, что они P2P. Буду излагать объективные факты и дополнять их своими комментариями, чтобы не бросаться громкими фразами в пространство. Выводы предлагаю делать самостоятельно.
Заранее отвечу на вопрос: мой pull request был принят.
Факт №1. В master-ветку попадает код, который падает на тестах
Всё началось со статей на хабре про установку ноды.
В комментах люди жаловались на сложность сборки и установки на CentOS, поэтому я решил запилить сборку на CMake. После пары дней работы я был готов презентовать своё поделие Tox-сообществу на Freenode, но меня встретили непониманием:
someone has contributed cmake initially, but other developers didn't know how to use it and couldn't make it build their code, so they switched to autotools (sic!), which they become to know better now.
Параллельно я отметил, что в master-ветку попадает код, который не проходит тесты в Travis CI, но мне ответили: «мы понимаем, с тестами нужно что-то делать, но пусть пока будет так».
Помогать так помогать, решил я и открыл код этого падающего надежды мессенджера.
Факт №2. memset(ptr, 0, size) перед вызовом free
Мой глаз зацепился за
memset(c, 0, sizeof(Net_Crypto));
free(c);
Если вы помните из цикла статей PVS-Studio, memset может быть вырезан оптимизатором компилятора, если регион памяти в будущем не будет использоваться. Логика компилятора проста: «После free жизни нет, значит и обращений к памяти не будет, удалю-ка я этот бессмысленный memset».
Как прилежный ученик, я заменил вызовы memset в подобных местах на sodium_memzero, и ТЕСТЫ УПАЛИ.
Факт №3. Сравнение публичных ключей уязвимо к атакам по времени
Для сравнений публичных ключей в toxcore есть специальная функция, и это хорошо:
/* compare 2 public keys of length crypto_box_PUBLICKEYBYTES, not vulnerable to timing attacks.
returns 0 if both mem locations of length are equal,
return -1 if they are not. */
int public_key_cmp(const uint8_t *pk1, const uint8_t *pk2)
{
return crypto_verify_32(pk1, pk2);
}
crypto_verify_32 — специальная функция из криптографических библиотек NaCL/Sodium, которая позволяет избежать атак по времени, т.к. работает за константное время, а не делает break при первом несовпадении байт. Использовать её следует при сравнении приватных данных: ключей, HMAC'ов и проч.
Кодовая база проекта toxcore довольно обширна, поэтому родился вот такой уродец с уязвимостью:
bool id_equal(const uint8_t *dest, const uint8_t *src)
{
return memcmp(dest, src, crypto_box_PUBLICKEYBYTES) == 0;
}
Но и это не всё. Имея на руках функции id_equal или public_key_cmp или crypto_verify_32, разработчики все равно предпочитают сравнивать ключи своим способом.
Вот краткая выжимка из кода DHT, onion маршрутизации и других критичных подсистем:
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)
Факт-домысел №4. Функция increment_nonce уязвима к атаке по времени
/* Increment the given nonce by 1. */
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; // <=== sic!
}
}
Функция не должна зависеть от секретных данных(кто хочет окунуться глубже, вот линка).
Именно increment_nonce частенько используется сервером для генерации нового nonce. Для инкремента nonce у Sodium есть специальная функция:
| Документация | sodium_increment() can be used to increment nonces in constant time. |
| Код |
|
Don't stop me now! Никаких break'ов, функция должна молотить байтики за константное время, даже если она уже проинкрементила своё и перенесла остаток!
Ирония заключается в том, что функция increment_nonce лежит в файле, который начинается со слов:
This code has to be perfect. We don't mess around with encryption.
Давайте присмотримся к этому идеальному файлу пристальней.
Факт №5. В стеке можно найти ключи и приватные данные
Подозрительное место:
/* Precomputes the shared key from their public_key and our secret_key.
* This way we can avoid an expensive elliptic curve scalar multiply for each
* encrypt/decrypt operation.
* enc_key has to be crypto_box_BEFORENMBYTES bytes long.
*/
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); // Nacl/Sodium function
}
/* Encrypts plain of length length to encrypted of length + 16 using the
* public key(32 bytes) of the receiver and the secret key of the sender and a 24 byte nonce.
*
* return -1 if there was a problem.
* return length of encrypted data if everything was fine.
*/
int encrypt_data(const uint8_t *public_key, const uint8_t *secret_key, const uint8_t *nonce,
const uint8_t *plain, uint32_t length, uint8_t *encrypted)
{
uint8_t k[crypto_box_BEFORENMBYTES];
encrypt_precompute(public_key, secret_key, k); // toxcore function
return encrypt_data_symmetric(k, nonce, plain, length, encrypted); // toxcore function
}
encrypt_data_symmetric вызывает crypto_box_detached_afternm из Nacl/Sodium, код приводить не буду, вот ссылка для желающих проверить мои слова.
Казалось бы, где можно накосячить в 4 строчках кода?
Давайте заглянем в исходный код 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;
}
Если вырезать все проверки, получится:
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;
Ничего не напоминает? Да это же слегка измененный код функции encrypt_data из toxcore, только ребята забыли почистить за собой ключик на стеке функцией sodium_memzero… Таких мест полным-полно: handle_TCP_handshake, handle_handshake, возможно где-то еще, но я уже устал.
Факт №6. Варнинги компилятора созданы для дураков!
В проекте toxcore категорически отрицают необходимость включения всех предупреждений компилятора. Ну или они про них не знают. Я не берусь ответить на вопрос, какое из этих двух предположений хуже.
Неиспользуемые функции (особенно радует срабатывание такого предупреждения в тестах):
../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 %in", len);
^ ~~~
/usr/include/check.h:273:18: note: expanded from macro 'ck_assert_msg'
_ck_assert_msg(expr, __FILE__, __LINE__,
^
И еще несколько десятков разных варнингов про неиспользуемые переменные, сравнение знакового и беззнакового и проч.
Мои выводы
Цитата из репозитория:
We want Tox to be as simple as possible while remaining as secure as possible.
Если я, человек, несведущий в криптографии, смог найти такие ужасы за день, сколько сможет найти профессионал, который будет целенаправленно рыть в течение месяца?
Этот проект представляет большую опасность для пользователей, которые полагаются на защищенность Tox. Когда-нибудь разработчики не глядя примут код, который помножит вашу приватность на ноль.
Как быть?
Можете присоединиться к нашему проекту 2tox, который мы потихоньку пилим с Halt на замену toxcore.
Автор: humbug
