From 9ce17ef881aebfb137fb31235ad00d870cf444e7 Mon Sep 17 00:00:00 2001 From: iphydf Date: Wed, 24 Jan 2024 20:45:41 +0000 Subject: [PATCH] fix: Stop memcpy-ing internal data structures into packets. --- toxcore/BUILD.bazel | 2 ++ toxcore/DHT.c | 18 +++++++++++++----- toxcore/DHT_fuzz_test.cc | 16 +++++++++------- toxcore/Messenger.c | 2 +- toxcore/network.c | 25 +++++++++++++++++-------- toxcore/network.h | 2 +- toxcore/onion_announce.c | 10 ++++++---- toxcore/onion_client.c | 15 ++++++++------- toxcore/ping.c | 13 +++++++++---- toxcore/ping.h | 2 +- 10 files changed, 67 insertions(+), 38 deletions(-) diff --git a/toxcore/BUILD.bazel b/toxcore/BUILD.bazel index 1e9d7e0cbea..83dedfd41a2 100644 --- a/toxcore/BUILD.bazel +++ b/toxcore/BUILD.bazel @@ -447,10 +447,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", ], ) diff --git a/toxcore/DHT.c b/toxcore/DHT.c index 0073a289c11..0b8a9f96a10 100644 --- a/toxcore/DHT.c +++ b/toxcore/DHT.c @@ -1938,7 +1938,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; @@ -1950,7 +1952,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 */ @@ -1959,11 +1963,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 */ @@ -2541,7 +2549,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"); diff --git a/toxcore/DHT_fuzz_test.cc b/toxcore/DHT_fuzz_test.cc index e741e9b259f..494d77536f7 100644 --- a/toxcore/DHT_fuzz_test.cc +++ b/toxcore/DHT_fuzz_test.cc @@ -6,6 +6,7 @@ #include #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 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 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 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(nodes.begin(), nodes.begin() + packed_count) + == std::vector(nodes2.begin(), nodes2.begin() + packed_count)); } } diff --git a/toxcore/Messenger.c b/toxcore/Messenger.c index 2655ae382e5..343d7c87836 100644 --- a/toxcore/Messenger.c +++ b/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); diff --git a/toxcore/network.c b/toxcore/network.c index 1442f1a5867..0a3022e60c1 100644 --- a/toxcore/network.c +++ b/toxcore/network.c @@ -924,7 +924,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; @@ -987,7 +987,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; @@ -1012,7 +1012,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; @@ -1502,14 +1503,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 */ @@ -1619,7 +1627,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; } @@ -1640,6 +1648,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; } diff --git a/toxcore/network.h b/toxcore/network.h index 6aea1cb33cc..ea58cd773b7 100644 --- a/toxcore/network.h +++ b/toxcore/network.h @@ -374,7 +374,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); diff --git a/toxcore/onion_announce.c b/toxcore/onion_announce.c index d3699ab09d8..e54a1991d4f 100644 --- a/toxcore/onion_announce.c +++ b/toxcore/onion_announce.c @@ -23,6 +23,7 @@ #include "onion.h" #include "shared_key_cache.h" #include "timed_auth.h" +#include "util.h" #define PING_ID_TIMEOUT ONION_ANNOUNCE_TIMEOUT @@ -462,10 +463,11 @@ 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; + uint8_t ping_id_data[CRYPTO_PUBLIC_KEY_SIZE + SIZE_IPPORT]; memcpy(ping_id_data, packet_public_key, CRYPTO_PUBLIC_KEY_SIZE); - memcpy(ping_id_data + CRYPTO_PUBLIC_KEY_SIZE, source, sizeof(*source)); + const int packed_len = pack_ip_port(onion_a->log, &ping_id_data[CRYPTO_PUBLIC_KEY_SIZE], SIZE_IPPORT, source); + memzero(&ping_id_data[CRYPTO_PUBLIC_KEY_SIZE + packed_len], SIZE_IPPORT - packed_len); const uint8_t *data_public_key = plain + ONION_PING_ID_SIZE + CRYPTO_PUBLIC_KEY_SIZE; @@ -509,7 +511,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) { diff --git a/toxcore/onion_client.c b/toxcore/onion_client.c index 94591319b67..af2ddb76a25 100644 --- a/toxcore/onion_client.c +++ b/toxcore/onion_client.c @@ -584,11 +584,12 @@ 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)); + memcpy(&data[sizeof(uint32_t)], public_key, CRYPTO_PUBLIC_KEY_SIZE); + const int packed_len = pack_ip_port(onion_c->logger, &data[sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE], SIZE_IPPORT, ip_port); + memzero(&data[sizeof(uint32_t) + CRYPTO_PUBLIC_KEY_SIZE + packed_len], SIZE_IPPORT - packed_len); + 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 +616,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)); diff --git a/toxcore/ping.c b/toxcore/ping.c index dc7658a2163..adfa8eb78cd 100644 --- a/toxcore/ping.c +++ b/toxcore/ping.c @@ -14,10 +14,12 @@ #include "DHT.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; DHT *dht; @@ -41,7 +44,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) { @@ -212,7 +216,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 +340,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 +356,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); diff --git a/toxcore/ping.h b/toxcore/ping.h index 55ed44d3991..0bccdf3018d 100644 --- a/toxcore/ping.h +++ b/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);