Use proper method for generating random numbers in a range

Using modulus creates a bias in its output. The libsodium randombytes_uniform() function guarantees a uniform distribution of possible outputs
TokTok · Feb 5, 2022 · 0c2c811 · 0c2c811
1 parent 7f0395b
commit 0c2c811
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 14 deletions.
diff --git a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256
@@ -1 +1 @@
-746158481ebd16d70aadc0bf4d2dc6da6a2f3ac4eb12d219b49fc6fd7e60d149  /usr/local/bin/tox-bootstrapd
+57e4c00a312ea345a31f2bb96c8d3b305c043ada0a8853034401203c1273e510  /usr/local/bin/tox-bootstrapd
diff --git a/toxcore/DHT.c b/toxcore/DHT.c
@@ -1707,10 +1707,10 @@ static uint8_t do_ping_and_sendnode_requests(DHT *dht, uint64_t *lastgetnode, co
 
     if ((num_nodes != 0) && (mono_time_is_timeout(dht->mono_time, *lastgetnode, GET_NODE_INTERVAL)
                              || *bootstrap_times < MAX_BOOTSTRAP_TIMES)) {
-        uint32_t rand_node = random_u32() % num_nodes;
+        uint32_t rand_node = random_range_u32(num_nodes);
 
         if ((num_nodes - 1) != rand_node) {
-            rand_node += random_u32() % (num_nodes - (rand_node + 1));
+            rand_node += random_range_u32(num_nodes - (rand_node + 1));
         }
 
         dht_getnodes(dht, &assoc_list[rand_node]->ip_port, client_list[rand_node]->public_key, public_key);
@@ -2049,7 +2049,8 @@ static uint32_t routeone_to_friend(const DHT *dht, const uint8_t *friend_id, con
         return 0;
     }
 
-    const int retval = send_packet(dht->net, &ip_list[random_u32() % n], *packet);
+    const uint32_t rand_idx = random_range_u32(n);
+    const int retval = send_packet(dht->net, &ip_list[rand_idx], *packet);
 
     if ((unsigned int)retval == packet->length) {
         return 1;

diff --git a/toxcore/crypto_core.c b/toxcore/crypto_core.c
@@ -164,6 +164,15 @@ uint64_t random_u64(void)
     return randnum;
 }
 
+uint32_t random_range_u32(uint32_t upper_bound)
+{
+#ifdef VANILLA_NACL
+    return random_u32() % upper_bound;
+#else
+    return randombytes_uniform(upper_bound);
+#endif  // VANILLA_NACL
+}
+
 bool public_key_valid(const uint8_t *public_key)
 {
     if (public_key[31] >= 128) { /* Last bit of key is always zero. */

diff --git a/toxcore/crypto_core.h b/toxcore/crypto_core.h
@@ -112,6 +112,14 @@ uint32_t random_u32(void);
  */
 uint64_t random_u64(void);
 
+/**
+ * Return a random 32 bit integer between 0 and upper_bound (excluded).
+ *
+ * On libsodium builds this function guarantees a uniform distribution of possible outputs.
+ * On vanilla NACL builds this function is equivalent to `random() % upper_value`.
+ */
+uint32_t random_range_u32(uint32_t upper_bound);
+
 /**
  * Fill the given nonce with random bytes.
  */

diff --git a/toxcore/onion_client.c b/toxcore/onion_client.c
@@ -261,7 +261,8 @@ static uint16_t random_nodes_path_onion(const Onion_Client *onion_c, Node_format
         }
 
         for (unsigned int i = 0; i < max_num; ++i) {
-            nodes[i] = onion_c->path_nodes[random_u32() % num_nodes];
+            const uint32_t rand_idx = random_range_u32(num_nodes);
+            nodes[i] = onion_c->path_nodes[rand_idx];
         }
     } else {
         int random_tcp = get_random_tcp_con_number(onion_c->c);
@@ -278,7 +279,8 @@ static uint16_t random_nodes_path_onion(const Onion_Client *onion_c, Node_format
             nodes[0].ip_port.ip.ip.v4.uint32 = random_tcp;
 
             for (unsigned int i = 1; i < max_num; ++i) {
-                nodes[i] = onion_c->path_nodes[random_u32() % num_nodes];
+                const uint32_t rand_idx = random_range_u32(num_nodes);
+                nodes[i] = onion_c->path_nodes[rand_idx];
             }
         } else {
             const uint16_t num_nodes_bs = min_u16(onion_c->path_nodes_index_bs, MAX_PATH_NODES);
@@ -294,7 +296,8 @@ static uint16_t random_nodes_path_onion(const Onion_Client *onion_c, Node_format
             nodes[0].ip_port.ip.ip.v4.uint32 = random_tcp;
 
             for (unsigned int i = 1; i < max_num; ++i) {
-                nodes[i] = onion_c->path_nodes_bs[random_u32() % num_nodes_bs];
+                const uint32_t rand_idx = random_range_u32(num_nodes_bs);
+                nodes[i] = onion_c->path_nodes_bs[rand_idx];
             }
         }
     }
@@ -359,7 +362,7 @@ static bool onion_node_timed_out(const Onion_Node *node, const Mono_Time *mono_t
 static int random_path(const Onion_Client *onion_c, Onion_Client_Paths *onion_paths, uint32_t pathnum, Onion_Path *path)
 {
     if (pathnum == UINT32_MAX) {
-        pathnum = random_u32() % NUMBER_ONION_PATHS;
+        pathnum = random_range_u32(NUMBER_ONION_PATHS);
     } else {
         pathnum = pathnum % NUMBER_ONION_PATHS;
     }
@@ -1581,7 +1584,7 @@ static void do_friend(Onion_Client *onion_c, uint16_t friendnum)
             }
 
             if (mono_time_is_timeout(onion_c->mono_time, list_nodes[i].last_pinged, interval)
-                    || (ping_random && random_u32() % (MAX_ONION_CLIENTS - i) == 0)) {
+                    || (ping_random && random_range_u32(MAX_ONION_CLIENTS - i) == 0)) {
                 if (client_send_announce_request(onion_c, friendnum + 1, &list_nodes[i].ip_port,
                                                  list_nodes[i].public_key, nullptr, -1) == 0) {
                     list_nodes[i].last_pinged = mono_time_get(onion_c->mono_time);
@@ -1599,10 +1602,10 @@ static void do_friend(Onion_Client *onion_c, uint16_t friendnum)
                 n = (MAX_ONION_CLIENTS / 2);
             }
 
-            if (count <= random_u32() % MAX_ONION_CLIENTS) {
+            if (count <= random_range_u32(MAX_ONION_CLIENTS)) {
                 if (num_nodes != 0) {
                     for (unsigned int j = 0; j < n; ++j) {
-                        const uint32_t num = random_u32() % num_nodes;
+                        const uint32_t num = random_range_u32(num_nodes);
                         client_send_announce_request(onion_c, friendnum + 1, &onion_c->path_nodes[num].ip_port,
                                                      onion_c->path_nodes[num].public_key, nullptr, -1);
                     }
@@ -1691,7 +1694,7 @@ static void do_announce(Onion_Client *onion_c)
 
         if (mono_time_is_timeout(onion_c->mono_time, list_nodes[i].last_pinged, interval)
                 || (mono_time_is_timeout(onion_c->mono_time, onion_c->last_announce, ONION_NODE_PING_INTERVAL)
-                    && random_u32() % (MAX_ONION_CLIENTS_ANNOUNCE - i) == 0)) {
+                    && random_range_u32(MAX_ONION_CLIENTS_ANNOUNCE - i) == 0)) {
             uint32_t path_to_use = list_nodes[i].path_used;
 
             if (list_nodes[i].unsuccessful_pings == ONION_NODE_MAX_PINGS - 1
@@ -1721,10 +1724,10 @@ static void do_announce(Onion_Client *onion_c)
             path_nodes = onion_c->path_nodes;
         }
 
-        if (count <= random_u32() % MAX_ONION_CLIENTS_ANNOUNCE) {
+        if (count <= random_range_u32(MAX_ONION_CLIENTS_ANNOUNCE)) {
             if (num_nodes != 0) {
                 for (unsigned int i = 0; i < (MAX_ONION_CLIENTS_ANNOUNCE / 2); ++i) {
-                    const uint32_t num = random_u32() % num_nodes;
+                    const uint32_t num = random_range_u32(num_nodes);
                     client_send_announce_request(onion_c, 0, &path_nodes[num].ip_port, path_nodes[num].public_key, nullptr, -1);
                 }
             }