Is Tox as safe as it is painted?
Hello!
I like Tox, I respect the participants of this project and their work, which sometimes even manages to be used for its intended purpose. In an effort to help the community, I looked into the code, noticed potential problems that could lead
Recently, there has been an unhealthy tendency to overestimate the security of such systems only on the basis that they are P2P. I will set forth objective facts and supplement them with my own comments, so as not to rush loud phrases into space. I propose to draw conclusions independently.
')
I will answer the question in advance: my pull request was accepted.
Fact number 1. The master branch gets code that falls on the tests.
It all started with articles on Habré about the installation of the node .
In the comments, people complained about the complexity of assembly and installation on CentOS, so I decided to fix the assembly on CMake. After a couple of days of work, I was ready to present my share to the Tox community on Freenode, but I was met with a lack of understanding:
For autotools (sic!), it’s not.
At the same time, I noted that code that does not pass the tests in Travis CI gets into the master branch, but I was told: “we understand, we need to do something with the tests, but let it be so for the time being”.
Helping so help, I decided, and opened the code of this hopeful messenger.
Fact number 2. memset (ptr, 0, size) before calling free
My eye caught on
memset(c, 0, sizeof(Net_Crypto)); free(c); If you remember from the cycle of articles in PVS-Studio, memset can be cut by the compiler optimizer if the memory region is not used in the future. The logic of the compiler is simple: “There is no life after free, which means there will be no memory access, I will delete this meaningless memset”.
As a diligent student, I replaced memset calls in similar places with sodium_memzero, and TESTS FELL.
Fact number 3. Comparison of public keys is vulnerable to time attacks
There is a special function for public key comparisons in toxcore, which is good:
/* 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 is a special function from the NaCL / Sodium cryptographic libraries that allows you to avoid time attacks, because It works for a constant time, but does not break at the first mismatch of bytes. It should be used when comparing private data: keys, HMACs and so on.
The code base of the toxcore project is quite extensive, so this freak with a vulnerability was born:
bool id_equal(const uint8_t *dest, const uint8_t *src) { return memcmp(dest, src, crypto_box_PUBLICKEYBYTES) == 0; } But that's not all. Having id_equal or public_key_cmp or crypto_verify_32 functions in hand, developers still prefer to compare keys in their own way.
Here is a brief extract from DHT, onion routing and other critical subsystems:
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) Fact speculation number 4. The increment_nonce function is vulnerable to a time attack.
/* 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! } } The function should not depend on secret data (who wants to plunge deeper, here is the link ).
It is increment_nonce that is often used by the server to generate a new nonce. For the nonce increment, Sodium has a special function:
| Documentation | sodium_increment () can be used. |
| Code | |
Don't stop me now! No breaks, the function should thresh the baitics for a constant time, even if it has already increased its own and transferred the rest!
The irony is that the increment_nonce function is in a file that starts with the words:
This code has to be perfect. We don't mess around with encryption.
Let's take a closer look at this perfect file.
Fact number 5. In the stack, you can find keys and private data
Suspicious place:
/* 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 calls crypto_box_detached_afternm from Nacl / Sodium, I will not give the code, here is a link for those who want to check my words.
It would seem, where you can nakosyachit in 4 lines of code?
Let's take a look at Sodium source code:
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; } If you cut all the checks, you get:
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; Nothing like? This is the slightly modified encrypt_data function code from toxcore, only the guys forgot to clean the key on the stack with the sodium_memzero function ... There are lots of places like this: handle_TCP_handshake , handle_handshake , maybe somewhere else, but I'm already tired.
Fact number 6. Compiler Warnings are made for fools!
In the toxcore project, they categorically deny the need to include all compiler warnings. Well, or they do not know about them. I will not answer the question which of these two assumptions is worse.
Unused functions (especially happy with the warning in the 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__,\ ^ And a few dozen different varnings about unused variables, the comparison of signed and unsigned, and so on.
My findings
Repository quote:
We want to make it possible.
If I, a person unknowing in cryptography, could find such horrors in a day, how much can a professional find who will purposefully dig for a month?
This project is a great danger for users who rely on Tox security. Someday, developers will not take a look at the code that multiplies your privacy by zero.
How to be?
You can join our project 2tox , which we are slowly sawing with Halt to replace toxcore.
Source: https://habr.com/ru/post/276665/
All Articles