fix: Stop memcpy-ing internal data structures into packets.

Author: iphydf

toxcore/BUILD.bazel

@@ -631,6 +631,7 @@ cc_fuzz_test(
     corpus = ["//tools/toktok-fuzzer/corpus:DHT_fuzz_test"],
     deps = [
         ":DHT",
+        ":DHT_test_util",
         ":mem_test_util",
         ":net_profile",
         "//c-toxcore/testing/fuzzing:fuzz_support",

toxcore/DHT.c

@@ -1931,7 +1931,9 @@ static int friend_iplist(const DHT *_Nonnull dht, IP_Port *_Nonnull ip_portlist,
     }
 
 #ifdef FRIEND_IPLIST_PAD
-    memcpy(ip_portlist, ipv6s, num_ipv6s * sizeof(IP_Port));
+    for (int i = 0; i < num_ipv6s; ++i) {
+        ip_portlist[i] = ipv6s[i];
+    }
 
     if (num_ipv6s == MAX_FRIEND_CLIENTS) {
         return MAX_FRIEND_CLIENTS;
@@ -1943,7 +1945,9 @@ static int friend_iplist(const DHT *_Nonnull dht, IP_Port *_Nonnull ip_portlist,
         num_ipv4s_used = num_ipv4s;
     }
 
-    memcpy(&ip_portlist[num_ipv6s], ipv4s, num_ipv4s_used * sizeof(IP_Port));
+    for (int i = 0; i < num_ipv4s_used; ++i) {
+        ip_portlist[num_ipv6s + i] = ipv4s[i];
+    }
     return num_ipv6s + num_ipv4s_used;
 
 #else /* !FRIEND_IPLIST_PAD */
@@ -1952,11 +1956,15 @@ static int friend_iplist(const DHT *_Nonnull dht, IP_Port *_Nonnull ip_portlist,
      * with the shorter one...
      */
     if (num_ipv6s >= num_ipv4s) {
-        memcpy(ip_portlist, ipv6s, num_ipv6s * sizeof(IP_Port));
+        for (int i = 0; i < num_ipv6s; ++i) {
+            ip_portlist[i] = ipv6s[i];
+        }
         return num_ipv6s;
     }
 
-    memcpy(ip_portlist, ipv4s, num_ipv4s * sizeof(IP_Port));
+    for (int i = 0; i < num_ipv4s; ++i) {
+        ip_portlist[i] = ipv4s[i];
+    }
     return num_ipv4s;
 
 #endif /* !FRIEND_IPLIST_PAD */
@@ -2515,7 +2523,7 @@ DHT *new_dht(const Logger *log, const Memory *mem, const Random *rng, const Netw
     dht->hole_punching_enabled = hole_punching_enabled;
     dht->lan_discovery_enabled = lan_discovery_enabled;
 
-    dht->ping = ping_new(mem, mono_time, rng, dht, net);
+    dht->ping = ping_new(mem, mono_time, log, rng, dht, net);
 
     if (dht->ping == nullptr) {
         LOGGER_ERROR(log, "failed to initialise ping");

toxcore/DHT_fuzz_test.cc

@@ -6,6 +6,7 @@
 #include <vector>
 
 #include "../testing/fuzzing/fuzz_support.hh"
+#include "DHT_test_util.hh"
 #include "mem_test_util.hh"
 
 namespace {
@@ -29,32 +30,33 @@ void TestUnpackNodes(Fuzz_Data &input)
     CONSUME1_OR_RETURN(const bool, tcp_enabled, input);
 
     const uint16_t node_count = 5;
-    Node_format nodes[node_count];
+    std::array<Node_format, node_count> nodes;
     uint16_t processed_data_len;
     const int packed_count = unpack_nodes(
-        nodes, node_count, &processed_data_len, input.data(), input.size(), tcp_enabled);
+        nodes.data(), nodes.size(), &processed_data_len, input.data(), input.size(), tcp_enabled);
     if (packed_count > 0) {
         const Memory *mem = os_memory();
         Logger *logger = logger_new(mem);
         std::vector<uint8_t> packed(packed_count * PACKED_NODE_SIZE_IP6);
         const int packed_size
-            = pack_nodes(logger, packed.data(), packed.size(), nodes, packed_count);
+            = pack_nodes(logger, packed.data(), packed.size(), nodes.data(), packed_count);
         LOGGER_ASSERT(logger, packed_size == processed_data_len,
             "packed size (%d) != unpacked size (%d)", packed_size, processed_data_len);
         logger_kill(logger);
 
         // Check that packed nodes can be unpacked again and result in the
         // original unpacked nodes.
-        Node_format nodes2[node_count];
+        std::array<Node_format, node_count> nodes2;
         uint16_t processed_data_len2;
-        const int packed_count2 = unpack_nodes(
-            nodes2, node_count, &processed_data_len2, packed.data(), packed.size(), tcp_enabled);
+        const int packed_count2 = unpack_nodes(nodes2.data(), nodes2.size(), &processed_data_len2,
+            packed.data(), packed.size(), tcp_enabled);
         (void)packed_count2;
 #if 0
         assert(processed_data_len2 == processed_data_len);
         assert(packed_count2 == packed_count);
 #endif
-        assert(memcmp(nodes, nodes2, sizeof(Node_format) * packed_count) == 0);
+        assert(std::vector<Node_format>(nodes.begin(), nodes.begin() + packed_count)
+            == std::vector<Node_format>(nodes2.begin(), nodes2.begin() + packed_count));
     }
 }
 

toxcore/Messenger.c

@@ -2438,7 +2438,7 @@ static bool self_announce_group(const Messenger *_Nonnull m, GC_Chat *_Nonnull c
     announce.base_announce.ip_port_is_set = ip_port_is_set;
 
     if (ip_port_is_set) {
-        memcpy(&announce.base_announce.ip_port, &chat->self_ip_port, sizeof(IP_Port));
+        announce.base_announce.ip_port = chat->self_ip_port;
     }
 
     memcpy(announce.base_announce.peer_public_key, chat->self_public_key.enc, ENC_PUBLIC_KEY_SIZE);

toxcore/network.c

@@ -842,7 +842,7 @@ int net_send_packet(const Networking_Core *net, const IP_Port *ip_port, Packet p
 {
     IP_Port ipp_copy = *ip_port;
 
-    if (net_family_is_unspec(ip_port->ip.family)) {
+    if (net_family_is_unspec(ipp_copy.ip.family)) {
         // TODO(iphydf): Make this an error. Currently this fails sometimes when
         // called from DHT.c:do_ping_and_sendnode_requests.
         return -1;
@@ -905,7 +905,7 @@ int net_send_packet(const Networking_Core *net, const IP_Port *ip_port, Packet p
     }
 
     const long res = net_sendto(net->ns, net->sock, packet.data, packet.length, &addr, &ipp_copy);
-    net_log_data(net->log, "O=>", packet.data, packet.length, ip_port, res);
+    net_log_data(net->log, "O=>", packet.data, packet.length, &ipp_copy, res);
 
     assert(res <= INT_MAX);
 
@@ -934,7 +934,8 @@ int sendpacket(const Networking_Core *net, const IP_Port *ip_port, const uint8_t
  */
 static int receivepacket(const Network *_Nonnull ns, const Logger *_Nonnull log, Socket sock, IP_Port *_Nonnull ip_port, uint8_t *_Nonnull data, uint32_t *_Nonnull length)
 {
-    memset(ip_port, 0, sizeof(IP_Port));
+    ipport_reset(ip_port);
+
     Network_Addr addr = {{0}};
     addr.size = sizeof(addr.addr);
     *length = 0;
@@ -1431,13 +1432,25 @@ void ip_reset(IP *ip)
 static const IP_Port empty_ip_port = {{{0}}};
 
 /** nulls out ip_port */
-void ipport_reset(IP_Port *ipport)
+void ipport_reset(IP_Port *_Nonnull ipport)
 {
     if (ipport == nullptr) {
         return;
     }
 
+#ifdef RANDOM_PADDING
+    // Leave padding bytes as uninitialized data. This should not matter, because we
+    // then set all the actual fields to 0.
+    IP_Port empty;
+    empty.ip.family.value = 0;
+    empty.ip.ip.v6.uint64[0] = 0;
+    empty.ip.ip.v6.uint64[1] = 0;
+    empty.port = 0;
+
+    *ipport = empty;
+#else
     *ipport = empty_ip_port;
+#endif /* RANDOM_PADDING */
 }
 
 /** nulls out ip, sets family according to flag */
@@ -1546,7 +1559,7 @@ bool bin_pack_ip_port(Bin_Pack *bp, const Logger *logger, const IP_Port *ip_port
         Ip_Ntoa ip_str;
         // TODO(iphydf): Find out why we're trying to pack invalid IPs, stop
         // doing that, and turn this into an error.
-        LOGGER_TRACE(logger, "cannot pack invalid IP: %s", net_ip_ntoa(&ip_port->ip, &ip_str));
+        LOGGER_DEBUG(logger, "cannot pack invalid IP: %s", net_ip_ntoa(&ip_port->ip, &ip_str));
         return false;
     }
 
@@ -1566,6 +1579,7 @@ int pack_ip_port(const Logger *logger, uint8_t *data, uint16_t length, const IP_
     const uint32_t size = bin_pack_obj_size(bin_pack_ip_port_handler, ip_port, logger);
 
     if (size > length) {
+        LOGGER_ERROR(logger, "not enough space for packed IP: %u but need %u", length, size);
         return -1;
     }
 

toxcore/ping.c

@@ -15,10 +15,12 @@
 #include "attributes.h"
 #include "ccompat.h"
 #include "crypto_core.h"
+#include "logger.h"
 #include "mem.h"
 #include "mono_time.h"
 #include "network.h"
 #include "ping_array.h"
+#include "util.h"
 
 #define PING_NUM_MAX 512
 
@@ -30,6 +32,7 @@
 
 struct Ping {
     const Mono_Time *mono_time;
+    const Logger *log;
     const Random *rng;
     const Memory *mem;
     DHT *dht;
@@ -42,7 +45,7 @@ struct Ping {
 
 #define PING_PLAIN_SIZE (1 + sizeof(uint64_t))
 #define DHT_PING_SIZE (1 + CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_NONCE_SIZE + PING_PLAIN_SIZE + CRYPTO_MAC_SIZE)
-#define PING_DATA_SIZE (CRYPTO_PUBLIC_KEY_SIZE + sizeof(IP_Port))
+#define PING_DATA_SIZE (CRYPTO_PUBLIC_KEY_SIZE + SIZE_IPPORT)
 
 void ping_send_request(Ping *ping, const IP_Port *ipp, const uint8_t *public_key)
 {
@@ -59,7 +62,8 @@ void ping_send_request(Ping *ping, const IP_Port *ipp, const uint8_t *public_key
     // Generate random ping_id.
     uint8_t data[PING_DATA_SIZE];
     pk_copy(data, public_key);
-    memcpy(data + CRYPTO_PUBLIC_KEY_SIZE, ipp, sizeof(IP_Port));
+    const int packed_len = pack_ip_port(ping->log, &data[CRYPTO_PUBLIC_KEY_SIZE], PING_DATA_SIZE - CRYPTO_PUBLIC_KEY_SIZE, ipp);
+    memzero(&data[packed_len], SIZE_IPPORT - packed_len);
     ping_id = ping_array_add(ping->ping_array, ping->mono_time, ping->rng, data, sizeof(data));
 
     if (ping_id == 0) {
@@ -205,7 +209,7 @@ static int handle_ping_response(void *_Nonnull object, const IP_Port *_Nonnull s
     }
 
     IP_Port ipp;
-    memcpy(&ipp, data + CRYPTO_PUBLIC_KEY_SIZE, sizeof(IP_Port));
+    unpack_ip_port(&ipp, &data[CRYPTO_PUBLIC_KEY_SIZE], PING_DATA_SIZE - CRYPTO_PUBLIC_KEY_SIZE, false);
 
     if (!ipport_equal(&ipp, source)) {
         return 1;
@@ -325,7 +329,7 @@ void ping_iterate(Ping *ping)
     }
 }
 
-Ping *ping_new(const Memory *mem, const Mono_Time *mono_time, const Random *rng, DHT *dht, Networking_Core *net)
+Ping *ping_new(const Memory *mem, const Mono_Time *mono_time, const Logger *log, const Random *rng, DHT *dht, Networking_Core *net)
 {
     Ping *ping = (Ping *)mem_alloc(mem, sizeof(Ping));
 
@@ -341,6 +345,7 @@ Ping *ping_new(const Memory *mem, const Mono_Time *mono_time, const Random *rng,
     }
 
     ping->mono_time = mono_time;
+    ping->log = log;
     ping->rng = rng;
     ping->mem = mem;
     ping->dht = dht;

toxcore/ping.h

@@ -21,7 +21,7 @@
 
 typedef struct Ping Ping;
 
-Ping *_Nullable ping_new(const Memory *_Nonnull mem, const Mono_Time *_Nonnull mono_time, const Random *_Nonnull rng, DHT *_Nonnull dht, Networking_Core *_Nonnull net);
+Ping *_Nullable ping_new(const Memory *_Nonnull mem, const Mono_Time *_Nonnull mono_time, const Logger *_Nonnull log, const Random *_Nonnull rng, DHT *_Nonnull dht, Networking_Core *_Nonnull net);
 
 void ping_kill(const Memory *_Nonnull mem, Ping *_Nullable ping);
 /** @brief Add nodes to the to_ping list.