Hi everyone!

I like Tox and respect the participants of this project and their work. In an effort to help Tox developers and users, I looked into the code and noticed potential problems that could lead to a false sense of security. Since I originally published this article in 2016 (in Russian), many improvements have been made to Tox, and I lead a team that re-wrote secure Tox software from scratch using the Rust programming language (check out Tox-rs). I DO recommend using tox in 2019. Let's take a look what actually made us rewrite Tox in Rust.

Original article of 2016

There is an unhealthy tendency to overestimate the security of E2E systems only on the basis that they are E2E. I will present objective facts supplemented with my own comments for you to draw your own conclusions.

Spoiler: The Tox developers agree with my points and my source code pull request was accepted.

Fact №1. master branch fails tests

It all started with articles on Habr about installing the node(in Russian).
In the comments, people complained about the complexity of building and installing the node on CentOS, so I decided to write a build system on CMake. After a few days, I was ready to present my PR to the Tox community on Freenode, but I was met with a lack of understanding:

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.

I noted that code that fails the tests in Travis CI still gets accepted into the master branch, but they answered: "we understand we need to do something with the tests, but let it be for now."

Next, I dived into looking deeper at the code of this attractive messenger.

Fact №2. memset(ptr, 0, size) before calling free

My eye caught

memset(c, 0, sizeof(Net_Crypto));
free(c);

If you still are not familiar with PVS-Studio and its article about memset function PVS-Studio: the compiler can delete the 'memset' function call if that memory region is not used afterwards. Compiler's logic is straightforward: "You are not going to use this variable after calling 'free', memset will not affect the observed behavior, let me delete this useless call to 'memset'".

As a diligent student I replaced each occurrence of memset($DST, 0, $SIZE) with sodium_memzero and the TESTS CRASHED.

Fact №3. Comparison of public keys is vulnerable to timing attacks

There is a really great special function to compare public keys in 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 — is a function from the NaCL/Sodium crypto library, that can help you to avoid timing attacks, because it works in constant time, while memcmp can break on the first unequal byte. You should use crypto_verify_32 to compare sensitive data like keys.

String comparisons performed byte-per-byte are vulnerable to exploitation by timing attacks, for example in order to forge MACs (see this vulnerability in Google's Keyczar crypto library).

The code base of the toxcore project is quite extensive, which is why Tox was born with a timing vulnerability bug:

bool id_equal(const uint8_t *dest, const uint8_t *src)
{
    return memcmp(dest, src, crypto_box_PUBLICKEYBYTES) == 0;
}

But that's not all. The developers still prefer to compare keys their own way using three different functions: id_equal or public_key_cmp and crypto_verify_32.
Here is a short grep output 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 №4. increment_nonce in a non constant time

/* 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!
    }
}

If such operations involve secret parameters, these timing variations can leak some information. With enough knowledge of the implementation is at hand, a careful statistical analysis could even lead to the total recovery of secret parameters.

There is a special function in Sodium to increment nonces:

Docs	sodium_increment() can be used to increment nonces in constant time.
Code	`void sodium_increment(unsigned char *n, const size_t nlen) { size_t i = 0U; uint_fast16_t c = 1U; for (; i < nlen; i++) { c += (uint_fast16_t) n[i]; n[i] = (unsigned char) c; c >>= 8; } }`

An accidentally ironic easter egg is that the increment_nonce function is located 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 №5. You can find keys and private data in the stack

Troublesome code:

/* 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 put the whole code, here is a link to check for yourself.

It seems difficult to make a mistake in four lines of code, doesn't it?

Let's dig into 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;
}

Erasing all checks we 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;

Does it look familiar? Yes! It's a slightly modified code of function encrypt_data from toxcore, the only difference is that they forgot to clean the key on the stack with sodium_memzero… And there are also mistakes in: handle_TCP_handshake, handle_handshake, and maybe somewhere else too.

Fact №6. Compiler warnings are for dummiez!

The devs from the toxcore project flatly deny the necessity of turning on all compiler warnings, or they do not know about them.

Unused functions (I am particularly pleased with the warnings in 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 warnings about unused variables, the comparison of signed and unsigned, and more.

My conclusion

Quote from the repository:

We want Tox to be as simple as possible while remaining as secure as possible.

If I, a non-cryptographer, could find such horrible bugs in a day, imagine how many things can a cryptography specialist might find after purposely dig for them for a month?.

The early versions of the Tox posed a great danger to users who rely on Tox security. Proprietary solutions are not trust-worthy and even open source solutions are not as safe as you want them to be. Take a look at the recent security breach in Matrix. Nowadays a lot of bugs are fixed and Tox is the best option available for security and privacy for users.

Next time I will tell you more about the current state of tox-rs. What we have implemented in Rust and why you should try it.

Reddit: comments

Long journey to Tox-rs. Part 1