refactor: re-implement shared key cache in separate file

The exisiting implementation is not clearly documented and used by
multiple modules.

Author: sudden6

CMakeLists.txt

@@ -284,6 +284,8 @@ set(toxcore_SOURCES
   toxcore/ping_array.h
   toxcore/ping.c
   toxcore/ping.h
+  toxcore/shared_key_cache.c
+  toxcore/shared_key_cache.h
   toxcore/state.c
   toxcore/state.h
   toxcore/TCP_client.c

other/bootstrap_daemon/docker/tox-bootstrapd.sha256

@@ -1 +1 @@
-b2bee729be40b0a0d8a4c6f0e2a527854d9a9d37712b73b915d7470bc91f5e6a  /usr/local/bin/tox-bootstrapd
+769233afaac07be03c094411e6ec8f031bde41beae475c74e154e51e51e9168b  /usr/local/bin/tox-bootstrapd

toxcore/BUILD.bazel

@@ -180,6 +180,24 @@ cc_test(
     ],
 )
 
+cc_library(
+    name = "shared_key_cache",
+    srcs = ["shared_key_cache.c"],
+    hdrs = ["shared_key_cache.h"],
+    visibility = [
+        "//c-toxcore/auto_tests:__pkg__",
+        "//c-toxcore/other:__pkg__",
+        "//c-toxcore/other/bootstrap_daemon:__pkg__",
+        "//c-toxcore/testing:__pkg__",
+        "//c-toxcore/toxav:__pkg__",
+    ],
+    deps = [
+        ":ccompat",
+        ":crypto_core",
+        ":mono_time",
+    ],
+)
+
 cc_library(
     name = "network",
     srcs = ["network.c"],
@@ -289,6 +307,7 @@ cc_library(
         ":mono_time",
         ":network",
         ":ping_array",
+        ":shared_key_cache",
         ":state",
         ":util",
     ],
@@ -326,6 +345,7 @@ cc_library(
         ":ccompat",
         ":crypto_core",
         ":mono_time",
+        ":shared_key_cache",
         ":util",
     ],
 )
@@ -366,6 +386,7 @@ cc_library(
         ":LAN_discovery",
         ":ccompat",
         ":forwarding",
+        ":shared_key_cache",
         ":timed_auth",
         ":util",
     ],
@@ -480,6 +501,7 @@ cc_library(
         ":ccompat",
         ":mono_time",
         ":onion",
+        ":shared_key_cache",
         ":timed_auth",
         ":util",
     ],

toxcore/DHT.c

@@ -18,6 +18,7 @@
 #include "mono_time.h"
 #include "network.h"
 #include "ping.h"
+#include "shared_key_cache.h"
 #include "state.h"
 #include "util.h"
 
@@ -46,6 +47,10 @@
 // TODO(sudden6): find out why we need multiple callbacks and if we really need 32
 #define DHT_FRIEND_MAX_LOCKS 32
 
+/* Settings for the shared key cache */
+#define MAX_KEYS_PER_SLOT 4
+#define KEYS_TIMEOUT 600
+
 typedef struct DHT_Friend_Callback {
     dht_ip_cb *ip_callback;
     void *data;
@@ -107,8 +112,8 @@ struct DHT {
     uint32_t       loaded_num_nodes;
     unsigned int   loaded_nodes_index;
 
-    Shared_Keys shared_keys_recv;
-    Shared_Keys shared_keys_sent;
+    Shared_Key_Cache *shared_keys_recv;
+    Shared_Key_Cache *shared_keys_sent;
 
     struct Ping   *ping;
     Ping_Array    *dht_ping_array;
@@ -255,74 +260,22 @@ unsigned int bit_by_bit_cmp(const uint8_t *pk1, const uint8_t *pk2)
     return i * 8 + j;
 }
 
-/**
- * Shared key generations are costly, it is therefore smart to store commonly used
- * ones so that they can be re-used later without being computed again.
- *
- * If a shared key is already in shared_keys, copy it to shared_key.
- * Otherwise generate it into shared_key and copy it to shared_keys
- */
-void get_shared_key(const Mono_Time *mono_time, Shared_Keys *shared_keys, uint8_t *shared_key,
-                    const uint8_t *secret_key, const uint8_t *public_key)
-{
-    uint32_t num = -1;
-    uint32_t curr = 0;
-
-    for (uint32_t i = 0; i < MAX_KEYS_PER_SLOT; ++i) {
-        const int index = public_key[30] * MAX_KEYS_PER_SLOT + i;
-        Shared_Key *const key = &shared_keys->keys[index];
-
-        if (key->stored) {
-            if (pk_equal(public_key, key->public_key)) {
-                memcpy(shared_key, key->shared_key, CRYPTO_SHARED_KEY_SIZE);
-                ++key->times_requested;
-                key->time_last_requested = mono_time_get(mono_time);
-                return;
-            }
-
-            if (num != 0) {
-                if (mono_time_is_timeout(mono_time, key->time_last_requested, KEYS_TIMEOUT)) {
-                    num = 0;
-                    curr = index;
-                } else if (num > key->times_requested) {
-                    num = key->times_requested;
-                    curr = index;
-                }
-            }
-        } else if (num != 0) {
-            num = 0;
-            curr = index;
-        }
-    }
-
-    encrypt_precompute(public_key, secret_key, shared_key);
-
-    if (num != UINT32_MAX) {
-        Shared_Key *const key = &shared_keys->keys[curr];
-        key->stored = true;
-        key->times_requested = 1;
-        memcpy(key->public_key, public_key, CRYPTO_PUBLIC_KEY_SIZE);
-        memcpy(key->shared_key, shared_key, CRYPTO_SHARED_KEY_SIZE);
-        key->time_last_requested = mono_time_get(mono_time);
-    }
-}
-
 /**
  * Copy shared_key to encrypt/decrypt DHT packet from public_key into shared_key
  * for packets that we receive.
  */
-void dht_get_shared_key_recv(DHT *dht, uint8_t *shared_key, const uint8_t *public_key)
+const uint8_t *dht_get_shared_key_recv(DHT *dht, const uint8_t *public_key)
 {
-    get_shared_key(dht->mono_time, &dht->shared_keys_recv, shared_key, dht->self_secret_key, public_key);
+    return shared_key_cache_lookup(dht->shared_keys_recv, public_key);
 }
 
 /**
  * Copy shared_key to encrypt/decrypt DHT packet from public_key into shared_key
  * for packets that we send.
  */
-void dht_get_shared_key_sent(DHT *dht, uint8_t *shared_key, const uint8_t *public_key)
+const uint8_t *dht_get_shared_key_sent(DHT *dht, const uint8_t *public_key)
 {
-    get_shared_key(dht->mono_time, &dht->shared_keys_sent, shared_key, dht->self_secret_key, public_key);
+    return shared_key_cache_lookup(dht->shared_keys_sent, public_key);
 }
 
 #define CRYPTO_SIZE (1 + CRYPTO_PUBLIC_KEY_SIZE * 2 + CRYPTO_NONCE_SIZE)
@@ -1063,8 +1016,7 @@ static bool send_announce_ping(DHT *dht, const uint8_t *public_key, const IP_Por
                                             public_key, CRYPTO_PUBLIC_KEY_SIZE);
     memcpy(plain + CRYPTO_PUBLIC_KEY_SIZE, &ping_id, sizeof(ping_id));
 
-    uint8_t shared_key[CRYPTO_SHARED_KEY_SIZE];
-    dht_get_shared_key_sent(dht, shared_key, public_key);
+    const uint8_t *shared_key = dht_get_shared_key_sent(dht, public_key);
 
     uint8_t request[1 + CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_NONCE_SIZE + sizeof(plain) + CRYPTO_MAC_SIZE];
 
@@ -1092,8 +1044,7 @@ static int handle_data_search_response(void *object, const IP_Port *source,
 
     VLA(uint8_t, plain, plain_len);
     const uint8_t *public_key = packet + 1;
-    uint8_t shared_key[CRYPTO_SHARED_KEY_SIZE];
-    dht_get_shared_key_recv(dht, shared_key, public_key);
+    const uint8_t *shared_key = dht_get_shared_key_recv(dht, public_key);
 
     if (decrypt_data_symmetric(shared_key,
                                packet + 1 + CRYPTO_PUBLIC_KEY_SIZE,
@@ -1522,15 +1473,12 @@ bool dht_getnodes(DHT *dht, const IP_Port *ip_port, const uint8_t *public_key, c
     memcpy(plain, client_id, CRYPTO_PUBLIC_KEY_SIZE);
     memcpy(plain + CRYPTO_PUBLIC_KEY_SIZE, &ping_id, sizeof(ping_id));
 
-    uint8_t shared_key[CRYPTO_SHARED_KEY_SIZE];
-    dht_get_shared_key_sent(dht, shared_key, public_key);
+    const uint8_t *shared_key = dht_get_shared_key_sent(dht, public_key);
 
     const int len = dht_create_packet(dht->rng,
                                       dht->self_public_key, shared_key, NET_PACKET_GET_NODES,
                                       plain, sizeof(plain), data, sizeof(data));
 
-    crypto_memzero(shared_key, sizeof(shared_key));
-
     if (len != sizeof(data)) {
         LOGGER_ERROR(dht->log, "getnodes packet encryption failed");
         return false;
@@ -1605,9 +1553,7 @@ static int handle_getnodes(void *object, const IP_Port *source, const uint8_t *p
     }
 
     uint8_t plain[CRYPTO_NODE_SIZE];
-    uint8_t shared_key[CRYPTO_SHARED_KEY_SIZE];
-
-    dht_get_shared_key_recv(dht, shared_key, packet + 1);
+    const uint8_t *shared_key = dht_get_shared_key_recv(dht, packet + 1);
     const int len = decrypt_data_symmetric(
                         shared_key,
                         packet + 1 + CRYPTO_PUBLIC_KEY_SIZE,
@@ -1616,16 +1562,13 @@ static int handle_getnodes(void *object, const IP_Port *source, const uint8_t *p
                         plain);
 
     if (len != CRYPTO_NODE_SIZE) {
-        crypto_memzero(shared_key, sizeof(shared_key));
         return 1;
     }
 
     sendnodes_ipv6(dht, source, packet + 1, plain, plain + CRYPTO_PUBLIC_KEY_SIZE, sizeof(uint64_t), shared_key);
 
     ping_add(dht->ping, packet + 1, source);
 
-    crypto_memzero(shared_key, sizeof(shared_key));
-
     return 0;
 }
 
@@ -1670,17 +1613,14 @@ static bool handle_sendnodes_core(void *object, const IP_Port *source, const uin
     }
 
     VLA(uint8_t, plain, 1 + data_size + sizeof(uint64_t));
-    uint8_t shared_key[CRYPTO_SHARED_KEY_SIZE];
-    dht_get_shared_key_sent(dht, shared_key, packet + 1);
+    const uint8_t *shared_key = dht_get_shared_key_sent(dht, packet + 1);
     const int len = decrypt_data_symmetric(
                         shared_key,
                         packet + 1 + CRYPTO_PUBLIC_KEY_SIZE,
                         packet + 1 + CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_NONCE_SIZE,
                         1 + data_size + sizeof(uint64_t) + CRYPTO_MAC_SIZE,
                         plain);
 
-    crypto_memzero(shared_key, sizeof(shared_key));
-
     if ((unsigned int)len != SIZEOF_VLA(plain)) {
         return false;
     }
@@ -2796,6 +2736,15 @@ DHT *new_dht(const Logger *log, const Random *rng, const Network *ns, Mono_Time
 
     crypto_new_keypair(rng, dht->self_public_key, dht->self_secret_key);
 
+    dht->shared_keys_recv = shared_key_cache_new(mono_time, dht->self_secret_key, KEYS_TIMEOUT, MAX_KEYS_PER_SLOT);
+    dht->shared_keys_sent = shared_key_cache_new(mono_time, dht->self_secret_key, KEYS_TIMEOUT, MAX_KEYS_PER_SLOT);
+
+    if (dht->shared_keys_recv == nullptr || dht->shared_keys_sent == nullptr) {
+        kill_dht(dht);
+        return nullptr;
+    }
+
+
     dht->dht_ping_array = ping_array_new(DHT_PING_ARRAY_SIZE, PING_TIMEOUT);
 
     if (dht->dht_ping_array == nullptr) {
@@ -2858,12 +2807,12 @@ void kill_dht(DHT *dht)
     networking_registerhandler(dht->net, NET_PACKET_LAN_DISCOVERY, nullptr, nullptr);
     cryptopacket_registerhandler(dht, CRYPTO_PACKET_NAT_PING, nullptr, nullptr);
 
+    shared_key_cache_free(dht->shared_keys_recv);
+    shared_key_cache_free(dht->shared_keys_sent);
     ping_array_kill(dht->dht_ping_array);
     ping_kill(dht->ping);
     free(dht->friends_list);
     free(dht->loaded_nodes_list);
-    crypto_memzero(&dht->shared_keys_recv, sizeof(dht->shared_keys_recv));
-    crypto_memzero(&dht->shared_keys_sent, sizeof(dht->shared_keys_sent));
     crypto_memzero(dht->self_secret_key, sizeof(dht->self_secret_key));
     free(dht);
 }

toxcore/DHT.h

@@ -254,24 +254,6 @@ non_null(1, 4) nullable(3)
 int unpack_nodes(Node_format *nodes, uint16_t max_num_nodes, uint16_t *processed_data_len, const uint8_t *data,
                  uint16_t length, bool tcp_enabled);
 
-
-/*----------------------------------------------------------------------------------*/
-/* struct to store some shared keys so we don't have to regenerate them for each request. */
-#define MAX_KEYS_PER_SLOT 4
-#define KEYS_TIMEOUT 600
-
-typedef struct Shared_Key {
-    uint8_t public_key[CRYPTO_PUBLIC_KEY_SIZE];
-    uint8_t shared_key[CRYPTO_SHARED_KEY_SIZE];
-    uint32_t times_requested;
-    bool stored;
-    uint64_t time_last_requested;
-} Shared_Key;
-
-typedef struct Shared_Keys {
-    Shared_Key keys[256 * MAX_KEYS_PER_SLOT];
-} Shared_Keys;
-
 /*----------------------------------------------------------------------------------*/
 
 typedef int cryptopacket_handler_cb(void *object, const IP_Port *ip_port, const uint8_t *source_pubkey,
@@ -295,31 +277,19 @@ non_null() const uint8_t *dht_get_friend_public_key(const DHT *dht, uint32_t fri
 
 /*----------------------------------------------------------------------------------*/
 
-/**
- * Shared key generations are costly, it is therefore smart to store commonly used
- * ones so that they can be re-used later without being computed again.
- *
- * If a shared key is already in shared_keys, copy it to shared_key.
- * Otherwise generate it into shared_key and copy it to shared_keys
- */
-non_null()
-void get_shared_key(
-    const Mono_Time *mono_time, Shared_Keys *shared_keys, uint8_t *shared_key,
-    const uint8_t *secret_key, const uint8_t *public_key);
-
 /**
  * Copy shared_key to encrypt/decrypt DHT packet from public_key into shared_key
  * for packets that we receive.
  */
 non_null()
-void dht_get_shared_key_recv(DHT *dht, uint8_t *shared_key, const uint8_t *public_key);
+const uint8_t *dht_get_shared_key_recv(DHT *dht, const uint8_t *public_key);
 
 /**
  * Copy shared_key to encrypt/decrypt DHT packet from public_key into shared_key
  * for packets that we send.
  */
 non_null()
-void dht_get_shared_key_sent(DHT *dht, uint8_t *shared_key, const uint8_t *public_key);
+const uint8_t *dht_get_shared_key_sent(DHT *dht, const uint8_t *public_key);
 
 /**
  * Sends a getnodes request to `ip_port` with the public key `public_key` for nodes

toxcore/Makefile.inc

@@ -61,6 +61,8 @@ libtoxcore_la_SOURCES = ../third_party/cmp/cmp.c \
                         ../toxcore/Messenger.c \
                         ../toxcore/ping.h \
                         ../toxcore/ping.c \
+                        ../toxcore/shared_key_cache.h \
+                        ../toxcore/shared_key_cache.c \
                         ../toxcore/state.h \
                         ../toxcore/state.c \
                         ../toxcore/tox.h \