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

Author: iphydf

toxcore/BUILD.bazel

@@ -446,10 +446,12 @@ cc_test(
 cc_fuzz_test(
     name = "DHT_fuzz_test",
     size = "small",
+    testonly = True,
     srcs = ["DHT_fuzz_test.cc"],
     corpus = ["//tools/toktok-fuzzer/corpus:DHT_fuzz_test"],
     deps = [
         ":DHT",
+        ":DHT_test_util",
         "//c-toxcore/testing/fuzzing:fuzz_support",
     ],
 )

toxcore/DHT.c

@@ -410,7 +410,7 @@ static bool bin_pack_ip_port(Bin_Pack *bp, const Logger *logger, const 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;
     }
 
@@ -431,6 +431,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;
     }
 
@@ -2077,7 +2078,9 @@ static int friend_iplist(const DHT *dht, IP_Port *ip_portlist, uint16_t friend_n
     }
 
 #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;
@@ -2089,7 +2092,9 @@ static int friend_iplist(const DHT *dht, IP_Port *ip_portlist, uint16_t friend_n
         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 */
@@ -2098,11 +2103,15 @@ static int friend_iplist(const DHT *dht, IP_Port *ip_portlist, uint16_t friend_n
      * 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 */
@@ -2680,7 +2689,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);
+    dht->ping = ping_new(mem, mono_time, log, rng, dht);
 
     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.h"
+#include "DHT_test_util.hh"
 
 namespace {
 
@@ -26,31 +27,32 @@ 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) {
         Logger *logger = logger_new();
         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

@@ -2597,7 +2597,7 @@ static bool self_announce_group(const Messenger *m, GC_Chat *chat, Onion_Friend
     announce.base_announce.ip_port_is_set = (uint8_t)(ip_port_is_set ? 1 : 0);
 
     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_PUBLIC_KEY_SIZE);

toxcore/network.c

@@ -923,7 +923,7 @@ int send_packet(const Networking_Core *net, const IP_Port *ip_port, Packet packe
 {
     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;
@@ -986,7 +986,7 @@ int send_packet(const Networking_Core *net, const IP_Port *ip_port, Packet packe
     }
 
     const long res = net_sendto(net->ns, net->sock, packet.data, packet.length, &addr, &ipp_copy);
-    loglogdata(net->log, "O=>", packet.data, packet.length, ip_port, res);
+    loglogdata(net->log, "O=>", packet.data, packet.length, &ipp_copy, res);
 
     assert(res <= INT_MAX);
     return (int)res;
@@ -1011,7 +1011,8 @@ int sendpacket(const Networking_Core *net, const IP_Port *ip_port, const uint8_t
 non_null()
 static int receivepacket(const Network *ns, const Memory *mem, const Logger *log, Socket sock, IP_Port *ip_port, uint8_t *data, uint32_t *length)
 {
-    memset(ip_port, 0, sizeof(IP_Port));
+    ipport_reset(ip_port);
+
     Network_Addr addr = {{0}};
     addr.size = sizeof(addr.addr);
     *length = 0;
@@ -1501,14 +1502,21 @@ void ip_reset(IP *ip)
 }
 
 /** nulls out ip_port */
-void ipport_reset(IP_Port *ipport)
+void ipport_reset(IP_Port *ip_port)
 {
-    if (ipport == nullptr) {
+    if (ip_port == nullptr) {
         return;
     }
 
-    const IP_Port empty_ip_port = {{{0}}};
-    *ipport = empty_ip_port;
+    // 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;
+
+    *ip_port = empty;
 }
 
 /** nulls out ip, sets family according to flag */

toxcore/network.h

@@ -373,7 +373,7 @@ non_null()
 void ip_reset(IP *ip);
 /** nulls out ip_port */
 non_null()
-void ipport_reset(IP_Port *ipport);
+void ipport_reset(IP_Port *ip_port);
 /** nulls out ip, sets family according to flag */
 non_null()
 void ip_init(IP *ip, bool ipv6enabled);

toxcore/onion_announce.c

@@ -462,10 +462,13 @@ static int handle_announce_request_common(
         return 1;
     }
 
-    const uint16_t ping_id_data_len = CRYPTO_PUBLIC_KEY_SIZE + sizeof(*source);
-    uint8_t ping_id_data[CRYPTO_PUBLIC_KEY_SIZE + sizeof(*source)];
+    const uint16_t ping_id_data_len = CRYPTO_PUBLIC_KEY_SIZE + SIZE_IPPORT;
+    // Must zero-init, because pack_ip_port may not write to all bytes.
+    // We only need to zero-init the last few bytes, but that's more
+    // error-prone, so we just zero-init everything.
+    uint8_t ping_id_data[CRYPTO_PUBLIC_KEY_SIZE + SIZE_IPPORT] = {0};
     memcpy(ping_id_data, packet_public_key, CRYPTO_PUBLIC_KEY_SIZE);
-    memcpy(ping_id_data + CRYPTO_PUBLIC_KEY_SIZE, source, sizeof(*source));
+    pack_ip_port(onion_a->log, ping_id_data + CRYPTO_PUBLIC_KEY_SIZE, SIZE_IPPORT, source);
 
     const uint8_t *data_public_key = plain + ONION_PING_ID_SIZE + CRYPTO_PUBLIC_KEY_SIZE;
 
@@ -509,7 +512,7 @@ static int handle_announce_request_common(
     int nodes_length = 0;
 
     if (num_nodes != 0) {
-        nodes_length = pack_nodes(onion_a->log, response + nodes_offset, sizeof(nodes_list), nodes_list,
+        nodes_length = pack_nodes(onion_a->log, response + nodes_offset, num_nodes * PACKED_NODE_SIZE_IP6, nodes_list,
                                   (uint16_t)num_nodes);
 
         if (nodes_length <= 0) {

toxcore/onion_client.c

@@ -584,11 +584,11 @@ non_null()
 static int new_sendback(Onion_Client *onion_c, uint32_t num, const uint8_t *public_key, const IP_Port *ip_port,
                         uint32_t path_num, uint64_t *sendback)
 {
-    uint8_t data[sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE + sizeof(IP_Port) + sizeof(uint32_t)];
+    uint8_t data[sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE + SIZE_IPPORT + sizeof(uint32_t)];
     memcpy(data, &num, sizeof(uint32_t));
     memcpy(data + sizeof(uint32_t), public_key, CRYPTO_PUBLIC_KEY_SIZE);
-    memcpy(data + sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE, ip_port, sizeof(IP_Port));
-    memcpy(data + sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE + sizeof(IP_Port), &path_num, sizeof(uint32_t));
+    pack_ip_port(onion_c->logger, data + sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE, SIZE_IPPORT, ip_port);
+    memcpy(data + sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE + SIZE_IPPORT, &path_num, sizeof(uint32_t));
     *sendback = ping_array_add(onion_c->announce_ping_array, onion_c->mono_time, onion_c->rng, data, sizeof(data));
 
     if (*sendback == 0) {
@@ -615,15 +615,15 @@ static uint32_t check_sendback(Onion_Client *onion_c, const uint8_t *sendback, u
 {
     uint64_t sback;
     memcpy(&sback, sendback, sizeof(uint64_t));
-    uint8_t data[sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE + sizeof(IP_Port) + sizeof(uint32_t)];
+    uint8_t data[sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE + SIZE_IPPORT + sizeof(uint32_t)];
 
     if (ping_array_check(onion_c->announce_ping_array, onion_c->mono_time, data, sizeof(data), sback) != sizeof(data)) {
         return -1;
     }
 
     memcpy(ret_pubkey, data + sizeof(uint32_t), CRYPTO_PUBLIC_KEY_SIZE);
-    memcpy(ret_ip_port, data + sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE, sizeof(IP_Port));
-    memcpy(path_num, data + sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE + sizeof(IP_Port), sizeof(uint32_t));
+    unpack_ip_port(ret_ip_port, data + sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE, SIZE_IPPORT, false);
+    memcpy(path_num, data + sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE + SIZE_IPPORT, sizeof(uint32_t));
 
     uint32_t num;
     memcpy(&num, data, sizeof(uint32_t));

toxcore/ping.c

@@ -14,6 +14,7 @@
 #include "DHT.h"
 #include "ccompat.h"
 #include "crypto_core.h"
+#include "logger.h"
 #include "mem.h"
 #include "mono_time.h"
 #include "network.h"
@@ -30,6 +31,7 @@
 
 struct Ping {
     const Mono_Time *mono_time;
+    const Logger *log;
     const Random *rng;
     DHT *dht;
 
@@ -41,7 +43,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 +61,7 @@ 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));
+    pack_ip_port(ping->log, &data[CRYPTO_PUBLIC_KEY_SIZE], PING_DATA_SIZE - CRYPTO_PUBLIC_KEY_SIZE, ipp);
     ping_id = ping_array_add(ping->ping_array, ping->mono_time, ping->rng, data, sizeof(data));
 
     if (ping_id == 0) {
@@ -212,7 +214,7 @@ static int handle_ping_response(void *object, const IP_Port *source, const uint8
     }
 
     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;
@@ -336,7 +338,7 @@ void ping_iterate(Ping *ping)
 }
 
 
-Ping *ping_new(const Memory *mem, const Mono_Time *mono_time, const Random *rng, DHT *dht)
+Ping *ping_new(const Memory *mem, const Mono_Time *mono_time, const Logger *log, const Random *rng, DHT *dht)
 {
     Ping *ping = (Ping *)mem_alloc(mem, sizeof(Ping));
 
@@ -352,6 +354,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->dht = dht;
     networking_registerhandler(dht_get_net(ping->dht), NET_PACKET_PING_REQUEST, &handle_ping_request, dht);

toxcore/ping.h

@@ -18,7 +18,7 @@
 typedef struct Ping Ping;
 
 non_null()
-Ping *ping_new(const Memory *mem, const Mono_Time *mono_time, const Random *rng, DHT *dht);
+Ping *ping_new(const Memory *mem, const Mono_Time *mono_time, const Logger *log, const Random *rng, DHT *dht);
 
 non_null(1) nullable(2)
 void ping_kill(const Memory *mem, Ping *ping);