Всем привет!
Мне нравится 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 %i\n", 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.
Комментарии (77)
Gorthauer87
05.02.2016 16:49+7Что-то мне начинает казаться, что не безопаснее ли будет это всё переписать на rust'е? Он за очень многие вещи сразу по рукам даст.
humbug
05.02.2016 16:57+11Ну что значит переписать? Нет документации, тестов, ожидаемого поведения, описания протокола. В процессе переписывания я расшифровываю их код, приходит понимание того, что они хотели сделать. Пишу юнит тесты так, чтобы можно было подсунуть им разную имплементацию: мою или toxcore. Таким образом есть ненулевая вероятность устроить безболезненный перевод гуя с одной библиотеки на другую.
Вы можете начать переписывать с C++ на Rust с использованием моего набора тестов. Это будет куда проще, чем начинать с C.Gorthauer87
05.02.2016 17:22А что об этом авторы toxcore говорят?
Вообще, можно только некоторые самые опасные функции на rust перенести.humbug
05.02.2016 17:25+1«Самых опасных» функций у них нет. Они используют криптографические библиотеки и делегируют тестирование на другой проект. Проблема в том, что у них везде сумасшедшая и недокументированная адресная арифметика. Там надо переписывать 50% на Rust.
Gorthauer87
05.02.2016 17:46+8Куча арифметики, раздолбайское отношение к коду в мастере, напрашивается вопрос, а сам протокол и алгоритмы то там адекватные?
questor
05.02.2016 17:12-5Открыл репозиторий kpp/2tox и что я вижу? Два файла (ридми и лицензия), закоммиченные месяц назад. Негусто. Когда что-то реальное в ствол будет сливаться из других (а там их штук пять) веток? Хотелось бы увидеть что-то более значимое, чем пустой репозиторий.
Halt
05.02.2016 17:20Если правки вносились бы в master по желанию левой пятки, то 2tox ничем бы тогда принципиально не отличался от toxcore.
questor
05.02.2016 17:18+4И ещё вопрос к автору: с одной стороны вы пишете, что не специалист по безопасности, с другой стороны замахнулись на собственную реализацию toxcore. Можно как экспы хапнуть, так и опозориться — не страшно было начинать? Что вообще двигало?
humbug
05.02.2016 17:37+19Я не зря написал в заголовке статьи: они приняли мой PR, указанные выше проблемы являются установленными фактами и признанными проблемы безопасности. Из этого следует несколько возможных выводов:
- Я, не специалист, знаю больше, чем они, и значит могу писать свою реализацию
- Я не знаю больше, чем они, но написать хуже, чем они, просто невозможно
Собственная реализация просто необходима, потому что иначе это будут заплатки в их коде, которые будут повышать доверие к проекту, но по факту, он как был дырявым, таковым и будет. Мне понадобился месяц в IRC канале, чтобы переубедить их, что надо использовать sodium_memzero, который имеется только в библиотеке Sodium, и, таким образом, полностью уйти от NaCl.
Да и вообще, я написал эту статью, чтобы предостеречь людей от бездумного доверия Tox. Что с этим делать — решать лично вам.
Halt
05.02.2016 17:44+8От себя добавлю следующее: даже не будучи специалистом по безопасности, можно помочь проекту эту самую безопасность повысить. Для начала, стоит поднять общее качество кода и снизить порог вхождения в него.
Как известно, хороший код отличается от плохого в том числе и метрикой «WTF per minute». Роман потратил огромное количество времени и сил чтобы разобраться в «хитросплетениях сюжета». Эта информация покоится либо в головах разработчиков, либо явно выражена в коде/комментариях.
Если код будет написан аккуратно, все основные моменты будут хорошо и прозрачно документированы, если все сложные и потенциально опасные места будут отмечены явно, то специалистам по безопасности (коими мы не являемся) будет гораздо проще увидеть проблемы.
Разумеется и желание их исправлять будет больше, если с кодом работать приятно и не приходится просеивать лапшу через дуршлаг.
Eklykti
05.02.2016 21:12+2> WTF per minute
grep -Ri fuck /usr/src/linuxHalt
05.02.2016 21:32+2Не забудьте поделить на количество строк во всем проекте и нормировать на суммарное время их прочтения.
shweew
08.02.2016 10:22+3Прикольно, не знал раньше:
Факи../usr/src/linux/fs/notify/inotify/inotify_user.c: * fucked up somewhere.
/usr/src/linux/fs/xfs/xfs_btree.h: case XFS_BTNUM_MAX: ASSERT(0); /* fucking gcc */; break; \
/usr/src/linux/fs/xfs/xfs_btree.h: case XFS_BTNUM_MAX: ASSERT(0); /* fucking gcc */; break; \
/usr/src/linux/arch/sparc/kernel/head_32.S: /* XXX Fucking Cypress… */
/usr/src/linux/arch/parisc/kernel/sys_parisc.c:/* Fucking broken ABI */
/usr/src/linux/arch/m68k/include/asm/sun3ints.h:/* master list of VME vectors — don't fuck with this */
/usr/src/linux/arch/x86/kernel/cpu/cpufreq/powernow-k7.c: * Some Athlon laptops have really fucked PST tables.
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't even give the
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't even give the
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't try to access
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't even give the
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't even give the
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't try to access
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't even give the
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't even give the
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't try to access
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't even give the
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't even give the
/usr/src/linux/arch/mips/pci/ops-bridge.c: * IOC3 is fucked fucked beyond believe… Don't try to access
/usr/src/linux/arch/mips/sgi-ip22/ip22-setup.c: * fucking with the memory controller because it needs to know the
/usr/src/linux/Documentation/DocBook/kernel-locking.tmpl: If you don't see why, please stay the fuck away from my code.
/usr/src/linux/net/ipv4/netfilter/nf_nat_snmp_basic.c: * (And this is the fucking 'basic' method).
/usr/src/linux/net/core/skbuff.c: /* Fuck, we are miserable poor guys… */
/usr/src/linux/lib/vsprintf.c: * Wirzenius wrote this portably, Torvalds fucked it up :-)
/usr/src/linux/drivers/media/video/bt819.c: BUG? Why does turning the chroma comb on fuck up color?
/usr/src/linux/drivers/mtd/mtd_blkdevs.c: registered, to prevent the link/init ordering from fucking
/usr/src/linux/drivers/ide/cmd640.c: * These chips are basically fucked by design, and getting this driver
/usr/src/linux/drivers/scsi/qlogicpti.h:/* Am I fucking pedantic or what? */
/usr/src/linux/drivers/net/sunhme.c:/* Only Sun can take such nice parts and fuck up the programming interface
/usr/src/linux/drivers/net/sunhme.c: /* This card is _fucking_ hot… */
/usr/src/linux/drivers/watchdog/shwdt.c: * brain-damage, it's managed to fuck things up one step further…
Scratch
05.02.2016 17:44+5Спасибо вам большое за эту статью. Не думал, что в токсе возникнет конкуренция, но это должно быть хорошо )
amarao
05.02.2016 17:47+5Две совместимые реализации tox'а вместо одной — это же отлично!
Gorthauer87
05.02.2016 17:50+2Что-то мне кажется, что это чудеса. В какой-то момент появится желание чуть чуть переделать протокол, ну например, чтобы наконец можно было с нескольких устройств юзать один id, или хотя бы чтобы можно было несколько id группировать в один контакт.
Я уж молчу про то, что уязвимости ещё бывает не из за деталей реализации, а by design.Halt
05.02.2016 17:55Такое уже давно происходит, например в проекте isotoxin, когда базовая функциональность поддерживается всеми клиентами, но «близкие родственники» могут предоставлять еще и дополнительную нестандартную функциональность. Тем не менее, не ошибается только тот кто ничего не делает.
Gorthauer87
05.02.2016 17:56Просто я боюсь при таком раздолбайстве core группы как-бы не закончилось всё форком :)
Halt
05.02.2016 18:19Мне кажется, при таком подходе любой форк будет лучше ванильки. Не обязательно этот.
NetBUG
05.02.2016 21:28Главное, чтобы не получилось как с XEPами.
Надеюсь, вокруг проекта недостаточно бюрократов, чтобы выработать свой какой-нибудь TOXEP.
humbug
05.02.2016 17:56Согласен. Только у меня большое подозрение, что они не могут реализовать некоторые хотелки из-за запутанного кода. А отлавливать уязвимости by design будет проще в красивом и понятном коде, как высказался выше Halt.
Gorthauer87
05.02.2016 18:11-1Звучит логично, но захотят ли они потом вашу либу делать референсной, вместо своей. Да и есть ли у проекта люди уровня Николая Дурова?
Halt
05.02.2016 18:20Тут уже пусть люди ногами голосуют. Если протокол будет-более менее совместим, остальное будет делом техники.
Конечно, нарушение совместимости протокола — это самое серьезное что можно сделать, и в эту сторону надо смотреть очень осторожно.
amarao
05.02.2016 18:54А вот две несовместимые реализации — это уже печально. Если своё новое добавлять, то обычная реализация с новой должны работать настолько хорошо, насколько работают друг с другом две худшие реализации (то есть без деградации).
StrangerInRed
05.02.2016 18:03-5Имхо если переписываете с нуля — переписывайте на swift (JFF).
Вместо memset(0, ptr, size) юзаю хеширование данных с рандомом.humbug
05.02.2016 19:07+2В данном случае уместно использовать sodium_memzero, потому что библиотека Sodium уже влинковывается.
nikitasius
05.02.2016 18:27+4Можете присоединиться к нашему проекту 2tox, который мы потихоньку пилим с Halt на замену toxcore.
Через два месяца (как с момента публикации статьи и ноде) появится новая статья на хабре от других кодеров, приблизительно в таком же стиле, но в конце будет:
Можете присоединиться к нашему проекту 3tox, который мы потихоньку пилим с username на замену toxcore и 2tox.
С одной стороны это замечательно, и это опенсорс. На этом плюсы кончились. Из минусов: сейчас tox таки начитает развиваться, и количество нод с декабря февраль выросло с 19 до 37.
Вместо запила второго, третьего и, вероятно, четвертого репозитария полезнее писать код вместе с разрабами, для связи есть #tox-dev и #tox.
Там адекватные ребята. Самый нервный — грейхантер, у него вечно бомбит на критику. Автор toxic'а (фригман) его противовес.
У них сейчас дилемма — с тем, что они задумали (хотя бы новые группы с правами) им придется свернуть со старого курса «простоты» и даже как-то хранить инфу на нодах, о чего они отплевывались 2 недели назад, или изобретать что-то более сложное для этого.Halt
05.02.2016 18:39Прошу прощения, вы статью читали? Автор общается с разработчиками. Исправления описанных проблем уже влиты в оригинальный репозиторий, — это раз. Второе — сама структура оригинального проекта мешает рефакторингу внедрению новых изменений.
nikitasius
05.02.2016 18:47+2Конечно прочитал. Так же я поднял логи IRC за месяц по каналам #tox-dev и #tox и не увидел grep'ом там ваших бесед (halt/humbug/kpp), а только упоминания 2tox самими разрабами токса и их ботом (который новости постит с гитхаба).
Для меня все равно загадка, почему не поговорить с ними по душам и не сделать toxcore 2.0 совместно?humbug
05.02.2016 19:00+4Грепайте по LearnC.
Для меня все равно загадка, почему не поговорить с ними по душам и не сделать toxcore 2.0 совместно?
Я начал писать 2.0 и попытался обосновать свое решение данной статьей. Вы знаете, я не из тех людей, который переманивает разработчиков путём переговоров на IRC канале. Я не собираюсь заманивать их воздушными мечтами о том, что «вот сейчас-то наконец-то взлетит». Я тихо пилю код.
humbug
05.02.2016 18:54+1Я не понимаю ваших претензий. Даже Столманн был бы мной доволен, я починил баги, отправил исправления в toxcore. Несмотря на то, что я решил создать свой репозитарий, я помогаю основному.
2tox и 3tox. И пускай! Чем больше, тем лучше! Только если это будет обоснованное решение, а не веление мизинца левой ноги. Лично я не хочу форкаться от toxcore, потому что тогда код будет заражен их багами. Вы же не считаете, что я нашел все ошибки?)) Я продемонстрировал качество кода и общее отношение к подходу разработки.
BalinTomsk
05.02.2016 18:34-9Вместо memset(c, 0, sizeof(Net_Crypto)); лучше использовать ZeroMemory
Gorthauer87
05.02.2016 19:06+4windows only?
encyclopedist
05.02.2016 19:10+7И даже на Windows не поможет. Из официальной документации к ZeroMemory:
To avoid any undesired effects of optimizing compilers, use the SecureZeroMemory function.
humbug
05.02.2016 19:07+1В данном случае уместно использовать sodium_memzero, потому что библиотека Sodium уже влинковывается. Более того, это кроссплатформенно.
Mugik
05.02.2016 20:01-20Ещё с прошлой публикации покопался в исходника, поустанавливал клиенты. Ничего прорывного, классного не увидел. Те, кто действительно беспокоится о своей безопасности не станут использовать написанные не им самим приложения, да и вряд ли люди, которые нуждаются в секретности пересылают информацию в мессенджерах. Простые смертные будут писать в вк или использовать скайп.
Открытый, опен-сорсный, ну ок. Век мессенджеров прошел. Теперь соц сети правят балом. Не понимаю почему это вызывает такой интерес.
nikitasius
05.02.2016 20:34+5Факт №1. В master-ветку попадает код, который падает на тестах
Время вопросов по статье, а именно по вашей стилистике. Я не С/C++ кодер, но открыв toxcore/jobs/105970831 я вижу:
FAIL: onion_test../auto_tests/onion_test.c:136:F:basic:test_basic:0: Onion failed initializing. ../auto_tests/onion_test.c:372:F:announce:test_announce:0: Failed to create onions. 0mayorovp
05.02.2016 20:55+5Нет, не слишком громко. Если тесты падают на CI — но их ремонт откладывается в неопределенное будущее, это означает, что тестам в принципе не уделяется достаточного внимания. А в таком случае уже не важно что тесты проходят на машине разработчика — с таким подходом там наверняка покрытие тестами недостаточное.
nikitasius
05.02.2016 21:02-5У вас никнейм на humbug не похож, значит вы не автор статьи не можете за него ответить.
Что до различия на клиенте:
make checkя такой же вызываю, аgcc (Debian 4.9.2-10) 4.9.2и, опять таки, читайте что я написал про окружение, в котором проходит тест. И когда в моем случае он прошел, а когда зафейлил.mayorovp
05.02.2016 21:13+2Вот именно. Чтобы тесты стали нормальными — надо просто вынести их с веб-сервера. Раз это до сих пор не сделано — значит, на тесты всем наплевать.
farcaller
05.02.2016 20:57+6Если CI флапает в красный через раз — толку от таких тестов ноль. Их все просто игнорируют.
humbug
06.02.2016 00:18+5Вы молодец, вас зря минусуют. Дайте мне время ответить на ваши вопросы.
nikitasius
08.02.2016 19:04Как дела с ответами?
humbug
08.02.2016 20:18Упс. Тесты упали не из-за моего коммита, они просто флоппируют. Встаем на ветку, удаляем коммиты(возникли из ребейза, ребята вмержили, получились конфликты)
405854e
b9ef248
bd62c6a
47c1e5f
c597f07
bf7a7ef
e4f86e2
61f8e65
Build log (сначала фейлится 2 теста, потом 1)nikitasius
09.02.2016 02:33Но у меня то на день тестов они все нормально прошли. И фейл был только у сетевых из-за работающей паралельно ноды. При остановке ноды они все были пройдены. Нода обновлена и теперь работает свежая.
humbug
09.02.2016 03:08Все верно. На момент написания статьи у меня фейлилось, потом меня попросили ребейзнуться, сейчас этой баги нет.
nikitasius
09.02.2016 11:33Так а в чем проблема то была с сетевыми тестами?
Я же в тот самый день (5го февраля) тестил — и тесты прошли (лог в комменте).humbug
09.02.2016 15:06Вы читали мое сообщение? Я же говорю, удалите коммиты(ревертните, ребейзнитесь или еще как). На момент написания статей их не было в мастере, тесты падали. Потом я отребейзился на мастер, какой-то из этих коммитов починил флоппирование теста.
Ой всё.
zim32
05.02.2016 21:18>>> Именно increment_nonce частенько используется сервером для генерации нового nonce.
Извиняюсь за нубский вопрос, т.е. получается что токс все-таки имеет централизированый сервер? Или что имеется в виду под словом сервер?
Prototik
05.02.2016 22:34+2Есть DHT ноды. Нужно ведь кому-то анонсировать маршруты и как-то коммутировать клиентов. Аналог торрент-сервера из мира торрентов: данные не хранит, но знает, где их можно достать.
robert_ayrapetyan
06.02.2016 19:35Странно, в Kad 15 летней давности уже умели обходиться без «коммутирующих» нод, зачем в Tox их добавили в архитектуру?
ValdikSS
07.02.2016 09:45Предполагаю, чтобы уменьшить количество передаваемых данных внутри DHT, что актуально в то время (до введения TCP-relay) для телефонов было.
Randl
05.02.2016 23:49+4Если ваши пулл-реквесты принимают, то почему бы не продолжить контрибьютить вместо того, чтобы пилить свой проект? Зачем распылять силы? Неужели это тот случай, когда проще переписать, чем отрефакторить? Или есть какие то фундаментальные разногласия с разработчиками?
serf
06.02.2016 01:53+2Насколько я понял потому что проблема системная, код ведь не сам такой стал, его таким сделали разработчики, которых такое состояние вроде как устраивает.
Randl
06.02.2016 02:47+4Ну опять-таки поговорить с разработчиками, объяснить, предложить решения, ограничения какие-то или код ревью. Было ли что это из этого сделано, или заранее признано безрезультатным? Кроме ответа по поводу тестов, никакой реакции разработчиков на замечания в статье нет.
Конечно, Tox относительно небольшой проект и его возможно переписать с нуля. С другой стороны, именно из-за малого сообщество, хорошо бы было сконцентрировать все силы вместе.humbug
06.02.2016 04:19+2Помните историю, когда у них увели бабки и домен? Я, как дочь офицера, могу отметить неоднозначность этой ситуации.
Обсуждение WTF строчки кода:
<LearnC> Hi! onion_client.c: is_path_used iterates over onion_paths but uses only the last of nodes. is it OK? <linux-modder> that last bit made no sense are you asking if compiling that way is ok or confirming that the .c file is written right ? <stal> LearnC: it's probably fine <LearnC> it makes no sence to pass pointer to the whole array if you use only 1 value <stal> i don't think there was a conscious decision to write it that way <stal> maybe gentoo just did it that way when he factored that code out of random_path
Единственная реакция — перед строчкой добавили комментарий.
Мы не слышали про ntohl и htonl:
/* Redefinitions of variables for safe transfer over wire. */ #define TOX_AF_INET 2 #define TOX_AF_INET6 10 #define TOX_TCP_INET 130 #define TOX_TCP_INET6 138 if (data[len_processed] == TOX_AF_INET) { ipv6 = 0; host_family = AF_INET; } else if (data[len_processed] == TOX_TCP_INET) { if (!tcp_enabled) return -1; ipv6 = 0; host_family = TCP_INET; } else if (data[len_processed] == TOX_AF_INET6) { ipv6 = 1; host_family = AF_INET6; } else if (data[len_processed] == TOX_TCP_INET6) { if (!tcp_enabled) return -1; ipv6 = 1; host_family = TCP_INET6; } else { return -1; }
Мы экономим байтики(длина пакета меняется в зависимости от IP адреса ноды):
uint32_t size = PACKED_NODE_SIZE_IP4; if (packed_length + size > length) return -1; data[packed_length] = net_family; memcpy(data + packed_length + 1, &nodes[i].ip_port.ip.ip4, SIZE_IP4); memcpy(data + packed_length + 1 + SIZE_IP4, &nodes[i].ip_port.port, sizeof(uint16_t)); memcpy(data + packed_length + 1 + SIZE_IP4 + sizeof(uint16_t), nodes[i].public_key, crypto_box_PUBLICKEYBYTES); packed_length += size; } else if (ipv6 == 1) { uint32_t size = PACKED_NODE_SIZE_IP6; if (packed_length + size > length) return -1; data[packed_length] = net_family; memcpy(data + packed_length + 1, &nodes[i].ip_port.ip.ip6, SIZE_IP6); memcpy(data + packed_length + 1 + SIZE_IP6, &nodes[i].ip_port.port, sizeof(uint16_t)); memcpy(data + packed_length + 1 + SIZE_IP6 + sizeof(uint16_t), nodes[i].public_key, crypto_box_PUBLICKEYBYTES); packed_length += size;
Ну и касательно моего личного отношения к обитателям их канала: мне их жаль.
<LearnC> grayhatter, hi <LearnC> I have just answered your comment in my PR #1506 <LearnC> Can we communicate a little bit closely? * Rowen_Stipe puffs on a pipe while wearing a bathrobe,"How closely you intend to get?" <cisc_> you sure that's a pipe? * cisc_ is now known as cisc <danieli> it's something nasty in a pipe disguise * Rowen_Stipe takes a large hit and holds it,"Yeah, It's a pipe alright" exhales a plume of smoke. <LearnC> haha thats funny to joke on a non native englishspeaker <danieli> english is my fourth language <danieli> don't complain <danieli> (was it sarcasm? I don't even know anymre) <danieli> anymore* <Rowen_Stipe> LearnC: Even if you wern't a nativ enlighs epeaker the joke would of been made.
Randl, я люблю свои глаза, поэтому копаться в какулях не люблю. Переписывание кода начинается с тестов, а не с переписывания) Но к тестам у них особое отношение, следовательно…Randl
06.02.2016 13:11+4В любом более или менее крупном проекте в какой-то монет появляется неидеальный код, костыли, откровенный говнокод и т.д. Не знаю ни одного проекта, которому удалось этого избежать.
Ваше решение понятно — вам не хочется рыться в чужом говнокоде. С точки зрения сообщества, развития Tox я считаю ваше решение непродуктивным.
Это классика опенсорса — если нет людей которые работают над проектом за деньги, то каждый делает то, что ему нравится — а мало кому нравится вычитывать говнокод или фиксить баги. Само наличие такого кода — результат такого подхода.
Насчёт личного отношения — хорошие программисты редко бывают хорошими людьми. С тем же Линусом много кому неприятно работать…
naething
06.02.2016 22:08+1Не обижайтесь уж так на них, пацаны просто пошутили немного («closely» действительно звучит как будто вы хотите «сблизиться» с ним). Хотя грамматическая ошибка в последней строке («would of» вместо «would've»/«would have») намекает на что, что там сидят школьники. Я бы предпочел, если бы криптографией занимались люди более образованые (с ученой степенью в идеале).
Randl
07.02.2016 08:21+1Хороший теоретик не всегда хороший практик. Теоретический Computer science ближе к математике, чем к программированию. Чтобы выявить уязвимости в коде существует аудит, который у Tox в планах.
Кроме того, неграмотный человек != плохой программист.
З. Ы. Бакалавр тоже ученая степень. Так что она хотя бы у части разработчиков наверняка есть
ValdikSS
07.02.2016 09:48+3намекает на что, что там сидят школьники.
Ой, да сколько людей путают your и you're.naething
07.02.2016 21:10А еще «there/their/they are», да. Но этого немного разного порядка ошибки. Впрочем, неважно. Слово «школьники» было употреблено в переносном смысле. Посыл был в том, что если человек делает такую ошибку, я бы предпочел не доверять ему писать код, от которого может зависеть моя безопасность. Криптографию программировать должны грамотные люди, а не студенты-второкурсники, которым море по колено :)
nikitasius
06.02.2016 22:44Вот, 16 строк до и после ника, для полноты картины. Вся техничка вырезана, ибо там ипшники и хабр не ест большие комменты (
^.*?has left #tox.*?$,^.*?is now known as.*?$,^.*?has joined.*?$,^.*?has quit.*?$). Если там какие-то диалоги между--и--бещ вас, это значит вы или вошли или вышли в этот момент.
grep -A 16 -B 16 LearnC irc.freenode.#tox.weechatlog >> LearnC.tox.txt2016-01-24 15:45:28 t-ask If I remove someone from my contacts list, do I appear then as offline for that person?
2016-01-24 15:45:46 t-ask btw. are there any developments in bringing tox to ubuntu phones?
2016-01-24 15:46:32 Rowen_Stipe t-ask: Yes, that's what happens. They just see you going perminately offline, you're not removed from their friendlist.
2016-01-24 15:47:32 t-ask Rowen_Stipe: ok, fine. Btw. is qtox still the best choice for desktop usage?
2016-01-24 15:47:47 t-ask "best" in quotes ;)
2016-01-24 15:48:01 Rowen_Stipe It has the most features to it certainly.
2016-01-24 15:48:22 Rowen_Stipe But best is always a relative term with Tox clients
--
2016-01-24 19:31:03 cisc_ www.dailydot.com/politics/anonymity-homeland-security-erik-barnett
2016-01-24 19:31:26 cisc_ what a fucking moron that guy is, how he is even getting paid taxpayer dollars is beyond me
2016-01-24 19:33:37 Rowen_Stipe cisc_: Cause the people that aren't half dead are working in the privet sector.
2016-01-24 19:37:24 cisc_ maybe the American people should be able to see everything he does online, in real time
2016-01-24 19:38:04 @zero-one we're back at SCaLE again
2016-01-24 19:38:09 cisc_ oh wait.. when it comes to the congress & the senate they're immune to their own laws
2016-01-24 19:38:13 @zero-one join the groupchat via groupbot
2016-01-24 19:38:34 @zero-one password is "scale14x"
2016-01-24 19:38:53 @zero-one everyone is hungover and shit, so the floor is pretty empty right now
2016-01-24 19:39:17 Impyy lol
2016-01-24 19:39:28 Rowen_Stipe XD
2016-01-24 19:42:18 LearnC grayhatter, hi
2016-01-24 19:42:43 LearnC I have just answered your comment in my PR #1506
2016-01-24 19:43:45 LearnC Can we communicate a little bit closely?
2016-01-24 19:46:25 * Rowen_Stipe puffs on a pipe while wearing a bathrobe,"How closely you intend to get?"
2016-01-24 19:46:56 cisc_ you sure that's a pipe?
2016-01-24 19:47:33 danieli it's something nasty in a pipe disguise
2016-01-24 19:48:04 * Rowen_Stipe takes a large hit and holds it,"Yeah, It's a pipe alright" exhales a plume of smoke.
2016-01-24 19:48:09 LearnC haha thats funny to joke on a non native englishspeaker
2016-01-24 19:48:23 danieli english is my fourth language
2016-01-24 19:48:29 danieli don't complain
2016-01-24 19:48:42 danieli (was it sarcasm? I don't even know anymre)
2016-01-24 19:48:45 danieli anymore*
2016-01-24 19:49:10 Rowen_Stipe LearnC: Even if you wern't a nativ enlighs epeaker the joke would of been made.
2016-01-24 19:49:21 LearnC would have*
2016-01-24 19:50:31 danieli LearnC: it's not "illegal" to use "of" instead of "have" in british english, it's just a relaxation of pronounciation
2016-01-24 19:51:23 LearnC do you want to discuss a my intention of close communication or to discuss that you don't correctly zero memory?
2016-01-24 19:51:40 LearnC That is what I found and I created a pull request
2016-01-24 19:52:13 LearnC you cannot use memset before free memory and rely on that kind of code
2016-01-24 19:52:21 danieli >memset
2016-01-24 19:52:26 danieli >reliable
2016-01-24 19:52:33 LearnC no
2016-01-24 19:53:37 Impyy LearnC, do you think there's a way to keep nacl compatibility?
2016-01-24 19:54:04 LearnC If there is a function in NaCl to zero memory than yes
2016-01-24 19:55:13 Impyy I think that function is a libsodium feature, it doesn't exist in nacl
2016-01-24 19:55:19 LearnC Implementing such a function may lead to security problems, unless there are some good skilled cryptographers who are both know how compiler works
2016-01-24 19:55:33 LearnC Impyy, no, its not a feature. msdn.microsoft.com/en-us/library/windows/hardware/ff562768%28v=vs.85%29.aspx
2016-01-24 19:56:25 Impyy LearnC, consider me corrected
2016-01-24 19:56:45 Impyy anyways I've never really understood why it's important to have nacl compatibility
2016-01-24 19:56:58 Impyy solely depending on libsodium seems fine
2016-01-24 19:57:26 LearnC I understand there might be some backward compatibility you want to save
2016-01-24 19:57:36 @nurupo it's either we copy the function from libsodium sources and use that or drop nacl support
2016-01-24 19:57:52 @nurupo i'm not sure how important is nacl support
2016-01-24 19:58:11 LearnC And it's up to irungentoo to decide
2016-01-24 19:59:00 LearnC What about timing attacks on publick keys comparison?
2016-01-24 19:59:46 LearnC Do you agree we have to use crypto functions provided by NaCl/Sodium to cmp keys or any sensitive data instead of memcmp?
2016-01-24 20:01:59 LearnC danieli, or maybe you think >memcmp is >reliable ?
2016-01-24 20:03:39 LearnC Rowen_Stipe, or maybe memcmp is pronounced more relaxed in British English than sodium_memcmp?
2016-01-24 20:03:43 danieli LearnC: depends on the context, usually not
2016-01-24 20:04:05 @nurupo Impyy: nacl is written by a well known cryptographer and has hardware optimizations. at the time tox started, sodium was quite slow and didn't even build on windows, so it wasn
2016-01-24 20:04:25 @nurupo *so it wasn't ready for use yet
2016-01-24 20:04:33 LearnC nurupo, the aim of the project to be secure, right?
2016-01-24 20:04:46 @nurupo right now though, we might consider dropping nacl
2016-01-24 20:05:04 @nurupo LearnC: talk to irungentoo about this
2016-01-24 20:05:16 LearnC thats why I am here
2016-01-24 20:05:22 LearnC w8 4 him
2016-01-24 20:06:50 @nurupo he is usually not on irc during weekends
2016-01-24 20:06:51 cisc dropping nacl and switch to what?
2016-01-24 20:07:05 @nurupo cisc: sodium
2016-01-24 20:07:12 cisc ok
2016-01-24 20:14:22 leer10 zero-one: which group has the password scale14x?
2016-01-24 20:17:21 Rowen_Stipe leer10: message groupbot info
2016-01-24 20:17:42 leer10 Rowen_Stipe cool got it :)
2016-01-24 20:46:37 __init__ is here anybody who tested new groupchats?
2016-01-24 20:47:25 -- Mode #tox [+o SylvieLorxu] by ChanServ
2016-01-24 20:49:25 @grayhatter LearnC: yeah, what did you want to discuss?
2016-01-24 20:50:03 LearnC we have already discussed it with other people in this channel
2016-01-24 20:50:12 LearnC you may read logs
2016-01-24 20:50:50 __init__ grayhatter, update wiki.tox.chat/clients
2016-01-24 20:52:51 @grayhatter __init__: update what?
2016-01-24 20:53:00 @grayhatter LearnC: did you see my GH comment?
2016-01-24 20:53:07 __init__ info about utox
2016-01-24 20:53:37 @grayhatter it's seems updated
2016-01-24 20:53:45 @grayhatter what part did you think was outdated?
2016-01-24 20:54:09 __init__ do you have contact aliases and save file encryption?
2016-01-24 20:54:29 LearnC grayhatter, no. I have read it right now. gray
gtbear
06.02.2016 08:47-1Можно понять разработчиков — появился х*р с горы и начал критиковать их код. Не имея особого авторитета у разработчиков, на что можно рассчитывать? Что они сразу кинутся мержить ваши PR? Очень врядле. Вместо того чтобы разобраться в ситуации почему так сложилось и какие есть пути выхода из нее — вы просто решили «взять и переписать». Зачастую (если не всегда) судьба opensource продуктов зависит не от умения писать код, а от умения налаживать взаимодействие команды.
Ivan_83
06.02.2016 10:15Хзхз.
Всем плевать на тайминг атаки на локалохосте, так что пользы от этого нисколько.
Тох это хороший концепт, но учитывая сколько он у меня вообще не работал и сколько падал то я думаю что его пишет какая то школота которая нихрена не понимает ни в сетях ни в работе с мультимедия.
Но я пользуюсь. Только для чатов, потому что видео/звук они не осилили нормально сделать.
И это я ещё в код не заглядывал и не пытался понять как там крипта применяется.
Предлагать переписать тох на го, руст, пхп, перл, джава — огромная глупость.
Тох во многом жив благодаря тому что любой хипстер может взять ихнюю сишную либу и приделать к своему мега клиенту на го/раст/вала/пхп/питон и тп.
Думаю что проще взять и написать своё с нуля, чем бодаться со стадом :)
На совместимость плевать ибо я хз что там у токса с криптой и надо мне оно для себя, но я бы сделал лучше: 4 симметричных шифра применяемых последовательно с различными ключами, и 4 алгоритма ассиметричной крипты также применямых раздельно.
Фактически что то типа: (AES+ECDSA) + (ГОСТ кузнечик + ГОСТ2012) + (RSA+трифиш) + (чача20+ед25519). (~1024+ бит симметричного ключа)
Такой фарш обратно не провернуть совсем никак.
Оно для основного канала управления и текстовых чатов, для файлов и мультимедии такая матрёшка очень трудна в плане вычислений, и имеет смысл оставлять только два алгоритма на временных ключах, переданных по основному каналу.
Сколько DH будет жрать вообще страшно представить :)Halt
06.02.2016 13:23+3Никто не предлагает нарушать совместимость реализаций. toxcore предоставляет сишный интерфейс. Если выставлять наружу его же, то любая совместимая реализация может быть прозрачно использована. Речь только о качестве кода внутри.
Если уж тут говорят о Rust, то надо не забывать, что он совершенно прозрачно интегрируется с C. На странице документации по сырым указателям можно посмотреть пример вызова оригинального malloc.
novoxudonoser
07.02.2016 13:40-3Неплохо, не плохо, можт можно и поучаствовать.
Главное введите возможность подключение центрального сервера для синхронизации и запоминания истории.novoxudonoser
07.02.2016 17:14Вообщем то в этой ветке вся основная мысля — https://habrahabr.ru/post/272937/#comment_8734719
Halt
Кстати, в обсуждении проекта 2tox, идет дискуссия на тему переписывания частей ядра на Rust. Мне кажется это отличная идея и отличное применение языка.
Предлагаю желающим подключаться.
Revertis
Всё время при прочтении статьи думал о том, что если бы они писали Tox на Rust, не было бы таких проблем.
Gorthauer87
Они могли бы обмазаться unsafe и получилось ровно тоже самое, увы.
Halt
В этом смысле проще. Каждый unsafe в rust коде надо еще обосновать. Отсутствие обоснования — автоматический довод в пользу рефакторинга.
Плюс к этому, все unsafe-ы легко ищутся по тексту.
Gorthauer87
Тоже верно, но запутать код так, чтобы там расплодились логические бомбы rust не помешает.
Halt
Разумеется. Но так можно сказать про любой язык и любой проект, написанный с его помощью.
Gorthauer87
Было бы круто иметь вменяемые тесты, и ещё подробное описание протокола. Оно-то вроде бы есть?