From 317ac882b6c95183c6a5a4fddf83f8be619651ec Mon Sep 17 00:00:00 2001 From: iphydf Date: Fri, 26 Jan 2024 16:12:40 +0000 Subject: [PATCH] fix: Don't use `memcmp` to compare `IP_Port`s. `memcmp` compares padding bytes as well, which may be arbitrary or uninitialised. --- toxcore/TCP_server.c | 2 +- toxcore/group.c | 7 +--- toxcore/list.c | 5 ++- toxcore/list.h | 6 ++- toxcore/list_test.cc | 6 +-- toxcore/net_crypto.c | 2 +- toxcore/network.c | 64 +++++++++++++++++++++++++++++- toxcore/network.h | 16 +++++++- toxcore/network_test.cc | 86 +++++++++++++++++++++++++++++++++++++++++ toxcore/util.c | 5 +++ toxcore/util.h | 3 ++ toxcore/util_test.cc | 11 ++++++ 12 files changed, 196 insertions(+), 17 deletions(-) diff --git a/toxcore/TCP_server.c b/toxcore/TCP_server.c index 5aa068181b2..52c96c35197 100644 --- a/toxcore/TCP_server.c +++ b/toxcore/TCP_server.c @@ -1043,7 +1043,7 @@ TCP_Server *new_tcp_server(const Logger *logger, const Memory *mem, const Random memcpy(temp->secret_key, secret_key, CRYPTO_SECRET_KEY_SIZE); crypto_derive_public_key(temp->public_key, temp->secret_key); - bs_list_init(&temp->accepted_key_list, CRYPTO_PUBLIC_KEY_SIZE, 8); + bs_list_init(&temp->accepted_key_list, CRYPTO_PUBLIC_KEY_SIZE, 8, memcmp); return temp; } diff --git a/toxcore/group.c b/toxcore/group.c index 4860556f6d2..acd263a9613 100644 --- a/toxcore/group.c +++ b/toxcore/group.c @@ -956,11 +956,6 @@ static bool delpeer(Group_Chats *g_c, uint32_t groupnumber, int peer_index, void return true; } -static int cmp_u64(uint64_t a, uint64_t b) -{ - return (a > b ? 1 : 0) - (a < b ? 1 : 0); -} - /** Order peers with friends first and with more recently active earlier */ non_null() static int cmp_frozen(const void *a, const void *b) @@ -972,7 +967,7 @@ static int cmp_frozen(const void *a, const void *b) return pa->is_friend ? -1 : 1; } - return cmp_u64(pb->last_active, pa->last_active); + return cmp_uint(pb->last_active, pa->last_active); } /** @brief Delete frozen peers as necessary to ensure at most `g->maxfrozen` remain. diff --git a/toxcore/list.c b/toxcore/list.c index 1096befc325..11b0c24a472 100644 --- a/toxcore/list.c +++ b/toxcore/list.c @@ -60,7 +60,7 @@ static int find(const BS_List *list, const uint8_t *data) // closest match is found if we move back to where we have already been while (true) { - const int r = memcmp(data, list->data + list->element_size * i, list->element_size); + const int r = list->cmp_callback(data, list->data + list->element_size * i, list->element_size); if (r == 0) { return i; @@ -135,7 +135,7 @@ static bool resize(BS_List *list, uint32_t new_size) } -int bs_list_init(BS_List *list, uint32_t element_size, uint32_t initial_capacity) +int bs_list_init(BS_List *list, uint32_t element_size, uint32_t initial_capacity, bs_list_cmp_cb *cmp_callback) { // set initial values list->n = 0; @@ -143,6 +143,7 @@ int bs_list_init(BS_List *list, uint32_t element_size, uint32_t initial_capacity list->capacity = 0; list->data = nullptr; list->ids = nullptr; + list->cmp_callback = cmp_callback; if (initial_capacity != 0) { if (!resize(list, initial_capacity)) { diff --git a/toxcore/list.h b/toxcore/list.h index ea01fb5fc57..1dc66d01a4a 100644 --- a/toxcore/list.h +++ b/toxcore/list.h @@ -12,6 +12,7 @@ #define C_TOXCORE_TOXCORE_LIST_H #include +#include // size_t #include #include "attributes.h" @@ -20,12 +21,15 @@ extern "C" { #endif +typedef int bs_list_cmp_cb(const void *a, const void *b, size_t size); + typedef struct BS_List { uint32_t n; // number of elements uint32_t capacity; // number of elements memory is allocated for uint32_t element_size; // size of the elements uint8_t *data; // array of elements int *ids; // array of element ids + bs_list_cmp_cb *cmp_callback; } BS_List; /** @brief Initialize a list. @@ -37,7 +41,7 @@ typedef struct BS_List { * @retval 0 failure */ non_null() -int bs_list_init(BS_List *list, uint32_t element_size, uint32_t initial_capacity); +int bs_list_init(BS_List *list, uint32_t element_size, uint32_t initial_capacity, bs_list_cmp_cb *cmp_callback); /** Free a list initiated with list_init */ nullable(1) diff --git a/toxcore/list_test.cc b/toxcore/list_test.cc index 5bffb8a1433..0d8f646c109 100644 --- a/toxcore/list_test.cc +++ b/toxcore/list_test.cc @@ -7,21 +7,21 @@ namespace { TEST(List, CreateAndDestroyWithNonZeroSize) { BS_List list; - bs_list_init(&list, sizeof(int), 10); + bs_list_init(&list, sizeof(int), 10, memcmp); bs_list_free(&list); } TEST(List, CreateAndDestroyWithZeroSize) { BS_List list; - bs_list_init(&list, sizeof(int), 0); + bs_list_init(&list, sizeof(int), 0, memcmp); bs_list_free(&list); } TEST(List, DeleteFromEmptyList) { BS_List list; - bs_list_init(&list, sizeof(int), 0); + bs_list_init(&list, sizeof(int), 0, memcmp); const uint8_t data[sizeof(int)] = {0}; bs_list_remove(&list, data, 0); bs_list_free(&list); diff --git a/toxcore/net_crypto.c b/toxcore/net_crypto.c index 441a30fbe3f..28b70d0fc26 100644 --- a/toxcore/net_crypto.c +++ b/toxcore/net_crypto.c @@ -3163,7 +3163,7 @@ Net_Crypto *new_net_crypto(const Logger *log, const Memory *mem, const Random *r networking_registerhandler(dht_get_net(dht), NET_PACKET_CRYPTO_HS, &udp_handle_packet, temp); networking_registerhandler(dht_get_net(dht), NET_PACKET_CRYPTO_DATA, &udp_handle_packet, temp); - bs_list_init(&temp->ip_port_list, sizeof(IP_Port), 8); + bs_list_init(&temp->ip_port_list, sizeof(IP_Port), 8, ipport_cmp_handler); return temp; } diff --git a/toxcore/network.c b/toxcore/network.c index 54b4257d579..c5fb31d2101 100644 --- a/toxcore/network.c +++ b/toxcore/network.c @@ -1435,6 +1435,61 @@ bool ipport_equal(const IP_Port *a, const IP_Port *b) return ip_equal(&a->ip, &b->ip); } +non_null() +static int ip4_cmp(const IP4 *a, const IP4 *b) +{ + return cmp_uint(a->uint32, b->uint32); +} + +non_null() +static int ip6_cmp(const IP6 *a, const IP6 *b) +{ + const int res = cmp_uint(a->uint64[0], b->uint64[0]); + if (res != 0) { + return res; + } + return cmp_uint(a->uint64[1], b->uint64[1]); +} + +non_null() +static int ip_cmp(const IP *a, const IP *b) +{ + const int res = cmp_uint(a->family.value, b->family.value); + if (res != 0) { + return res; + } + switch (a->family.value) { + case TOX_AF_UNSPEC: + return 0; + case TOX_AF_INET: + case TCP_INET: + case TOX_TCP_INET: + return ip4_cmp(&a->ip.v4, &b->ip.v4); + case TOX_AF_INET6: + case TCP_INET6: + case TOX_TCP_INET6: + case TCP_SERVER_FAMILY: // these happen to be ipv6 according to TCP_server.c. + case TCP_CLIENT_FAMILY: + return ip6_cmp(&a->ip.v6, &b->ip.v6); + } + // Invalid, we don't compare any further and consider them equal. + return 0; +} + +int ipport_cmp_handler(const void *a, const void *b, size_t size) +{ + const IP_Port *ipp_a = (const IP_Port *)a; + const IP_Port *ipp_b = (const IP_Port *)b; + assert(size == sizeof(IP_Port)); + + const int ip_res = ip_cmp(&ipp_a->ip, &ipp_b->ip); + if (ip_res != 0) { + return ip_res; + } + + return cmp_uint(ipp_a->port, ipp_b->port); +} + /** nulls out ip */ void ip_reset(IP *ip) { @@ -1508,7 +1563,14 @@ void ipport_copy(IP_Port *target, const IP_Port *source) return; } - *target = *source; + // Write to a temporary object first, so that padding bytes are + // uninitialised and msan can catch mistakes in downstream code. + IP_Port tmp; + tmp.ip.family = source->ip.family; + tmp.ip.ip = source->ip.ip; + tmp.port = source->port; + + *target = tmp; } const char *net_ip_ntoa(const IP *ip, Ip_Ntoa *ip_str) diff --git a/toxcore/network.h b/toxcore/network.h index aa2f9946154..4697bffb2c2 100644 --- a/toxcore/network.h +++ b/toxcore/network.h @@ -337,7 +337,7 @@ non_null() bool addr_parse_ip(const char *address, IP *to); /** - * Compares two IPAny structures. + * Compares two IP structures. * * Unset means unequal. * @@ -347,7 +347,7 @@ nullable(1, 2) bool ip_equal(const IP *a, const IP *b); /** - * Compares two IPAny_Port structures. + * Compares two IP_Port structures. * * Unset means unequal. * @@ -356,6 +356,18 @@ bool ip_equal(const IP *a, const IP *b); nullable(1, 2) bool ipport_equal(const IP_Port *a, const IP_Port *b); +/** + * @brief IP_Port comparison function with `memcmp` signature. + * + * Casts the void pointers to `IP_Port *` for comparison. + * + * @retval -1 if `a < b` + * @retval 0 if `a == b` + * @retval 1 if `a > b` + */ +non_null() +int ipport_cmp_handler(const void *a, const void *b, size_t size); + /** nulls out ip */ non_null() void ip_reset(IP *ip); diff --git a/toxcore/network_test.cc b/toxcore/network_test.cc index a86436c8db9..8969c7d8541 100644 --- a/toxcore/network_test.cc +++ b/toxcore/network_test.cc @@ -82,4 +82,90 @@ TEST(IpParseAddr, FormatsIPv6) EXPECT_EQ(std::string(ip_str), "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff"); } +TEST(IpportCmp, BehavesLikeMemcmp) +{ + auto cmp_val = [](int val) { return val < 0 ? -1 : val > 0 ? 1 : 0; }; + + IP_Port a = {0}; + IP_Port b = {0}; + + a.ip.family = net_family_ipv4(); + b.ip.family = net_family_ipv4(); + + a.port = 10; + b.port = 20; + + EXPECT_EQ( // + ipport_cmp_handler(&a, &b, sizeof(IP_Port)), -1) + << "a=" << a << "\n" + << "b=" << b; + EXPECT_EQ( // + ipport_cmp_handler(&a, &b, sizeof(IP_Port)), // + cmp_val(memcmp(&a, &b, sizeof(IP_Port)))) + << "a=" << a << "\n" + << "b=" << b; + + a.ip.ip.v4.uint8[0] = 192; + b.ip.ip.v4.uint8[0] = 10; + + EXPECT_EQ( // + ipport_cmp_handler(&a, &b, sizeof(IP_Port)), 1) + << "a=" << a << "\n" + << "b=" << b; + EXPECT_EQ( // + ipport_cmp_handler(&a, &b, sizeof(IP_Port)), // + cmp_val(memcmp(&a, &b, sizeof(IP_Port)))) + << "a=" << a << "\n" + << "b=" << b; +} + +TEST(IpportCmp, Ipv6BeginAndEndCompareCorrectly) +{ + IP_Port a = {0}; + IP_Port b = {0}; + + a.ip.family = net_family_ipv6(); + b.ip.family = net_family_ipv6(); + + a.ip.ip.v6.uint8[0] = 0xab; + b.ip.ip.v6.uint8[0] = 0xba; + + EXPECT_EQ(ipport_cmp_handler(&a, &b, sizeof(IP_Port)), -1); + + a.ip.ip.v6.uint8[0] = 0; + b.ip.ip.v6.uint8[0] = 0; + + a.ip.ip.v6.uint8[15] = 0xba; + + EXPECT_EQ(ipport_cmp_handler(&a, &b, sizeof(IP_Port)), 1); +} + +TEST(IpportCmp, UnspecAlwaysComparesEqual) +{ + IP_Port a = {0}; + IP_Port b = {0}; + + a.ip.family = net_family_unspec(); + b.ip.family = net_family_unspec(); + + a.ip.ip.v4.uint8[0] = 0xab; + b.ip.ip.v4.uint8[0] = 0xba; + + EXPECT_EQ(ipport_cmp_handler(&a, &b, sizeof(IP_Port)), 0); +} + +TEST(IpportCmp, InvalidAlwaysComparesEqual) +{ + IP_Port a = {0}; + IP_Port b = {0}; + + a.ip.family.value = 0xff; + b.ip.family.value = 0xff; + + a.ip.ip.v4.uint8[0] = 0xab; + b.ip.ip.v4.uint8[0] = 0xba; + + EXPECT_EQ(ipport_cmp_handler(&a, &b, sizeof(IP_Port)), 0); +} + } // namespace diff --git a/toxcore/util.c b/toxcore/util.c index 7a899a982d0..1851e58a080 100644 --- a/toxcore/util.c +++ b/toxcore/util.c @@ -161,6 +161,11 @@ uint64_t min_u64(uint64_t a, uint64_t b) return a < b ? a : b; } +int cmp_uint(uint64_t a, uint64_t b) +{ + return (a > b ? 1 : 0) - (a < b ? 1 : 0); +} + uint32_t jenkins_one_at_a_time_hash(const uint8_t *key, size_t len) { uint32_t hash = 0; diff --git a/toxcore/util.h b/toxcore/util.h index 16835e8ffd3..5be74a8d86c 100644 --- a/toxcore/util.h +++ b/toxcore/util.h @@ -79,6 +79,9 @@ uint16_t min_u16(uint16_t a, uint16_t b); uint32_t min_u32(uint32_t a, uint32_t b); uint64_t min_u64(uint64_t a, uint64_t b); +// Comparison function: return -1 if ab. +int cmp_uint(uint64_t a, uint64_t b); + /** @brief Returns a 32-bit hash of key of size len */ non_null() uint32_t jenkins_one_at_a_time_hash(const uint8_t *key, size_t len); diff --git a/toxcore/util_test.cc b/toxcore/util_test.cc index aca278e3165..94e653f2188 100644 --- a/toxcore/util_test.cc +++ b/toxcore/util_test.cc @@ -34,4 +34,15 @@ TEST(Util, IdCopyMakesKeysEqual) EXPECT_TRUE(pk_equal(pk1, pk2)); } +TEST(Cmp, OrdersNumbersCorrectly) +{ + EXPECT_EQ(cmp_uint(1, 2), -1); + EXPECT_EQ(cmp_uint(0, UINT32_MAX), -1); + EXPECT_EQ(cmp_uint(UINT32_MAX, 0), 1); + EXPECT_EQ(cmp_uint(UINT32_MAX, UINT32_MAX), 0); + EXPECT_EQ(cmp_uint(0, UINT64_MAX), -1); + EXPECT_EQ(cmp_uint(UINT64_MAX, 0), 1); + EXPECT_EQ(cmp_uint(UINT64_MAX, UINT64_MAX), 0); +} + } // namespace