From 93aafd78c1ff74ed994426cbe07f031795923d3c Mon Sep 17 00:00:00 2001 From: jfreegman Date: Fri, 1 Mar 2024 22:12:31 -0500 Subject: [PATCH 1/2] fix: friend requests with very long messages are no longer dropped This is a band-aid solution that prevents friend requests with long messages from being dropped. However it doesn't solve the underlying problem, described here: https://github.com/TokTok/c-toxcore/issues/2719 --- toxcore/friend_connection.c | 4 ++++ toxcore/friend_requests.h | 3 ++- toxcore/onion_client.h | 2 ++ toxcore/tox.h | 2 +- 4 files changed, 9 insertions(+), 2 deletions(-) diff --git a/toxcore/friend_connection.c b/toxcore/friend_connection.c index f017b08888..6527dda53b 100644 --- a/toxcore/friend_connection.c +++ b/toxcore/friend_connection.c @@ -875,6 +875,10 @@ void set_friend_request_callback(Friend_Connections *fr_c, fr_request_cb *fr_req int send_friend_request_packet(Friend_Connections *fr_c, int friendcon_id, uint32_t nospam_num, const uint8_t *data, uint16_t length) { + // FIXME: This max packet size is too large to be handled by receiving clients + // when sent via the onion. We currently limit the length at a higher level, but + // this bounds check should be fixed to represent the max size of a packet that + // the onion client can handle. if (1 + sizeof(nospam_num) + length > ONION_CLIENT_MAX_DATA_SIZE || length == 0) { return -1; } diff --git a/toxcore/friend_requests.h b/toxcore/friend_requests.h index a78a570dfa..7e200c2d43 100644 --- a/toxcore/friend_requests.h +++ b/toxcore/friend_requests.h @@ -14,7 +14,8 @@ #include "attributes.h" #include "friend_connection.h" -#define MAX_FRIEND_REQUEST_DATA_SIZE (ONION_CLIENT_MAX_DATA_SIZE - (1 + sizeof(uint32_t))) +// FIXME: This should be the maximum size that an onion client can handle. +#define MAX_FRIEND_REQUEST_DATA_SIZE (ONION_CLIENT_MAX_DATA_SIZE - 100) typedef struct Friend_Requests Friend_Requests; diff --git a/toxcore/onion_client.h b/toxcore/onion_client.h index 61e4e6fd1c..b6a4f1a6bd 100644 --- a/toxcore/onion_client.h +++ b/toxcore/onion_client.h @@ -183,6 +183,8 @@ non_null() unsigned int onion_getfriend_dht_pubkey(const Onion_Client *onion_c, int friend_num, uint8_t *dht_key); #define ONION_DATA_IN_RESPONSE_MIN_SIZE (CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_MAC_SIZE) + +// FIXME: This is not the correct value; data this large will be dropped by the onion client. #define ONION_CLIENT_MAX_DATA_SIZE (MAX_DATA_REQUEST_SIZE - ONION_DATA_IN_RESPONSE_MIN_SIZE) /** @brief Send data of length length to friendnum. diff --git a/toxcore/tox.h b/toxcore/tox.h index d866e9a5a0..101437b77d 100644 --- a/toxcore/tox.h +++ b/toxcore/tox.h @@ -274,7 +274,7 @@ uint32_t tox_max_status_message_length(void); * * @deprecated The macro will be removed in 0.3.0. Use the function instead. */ -#define TOX_MAX_FRIEND_REQUEST_LENGTH 1016 +#define TOX_MAX_FRIEND_REQUEST_LENGTH 921 uint32_t tox_max_friend_request_length(void); From 29d1043be0bb75c2542375d2e9e661bec06c3d90 Mon Sep 17 00:00:00 2001 From: jfreegman Date: Wed, 27 Nov 2024 20:10:38 -0500 Subject: [PATCH 2/2] test: friend request test now tests min/max message sizes --- auto_tests/friend_request_test.c | 74 ++++++++++++++++++++++++-------- toxcore/friend_connection.c | 2 +- toxcore/friend_requests.h | 2 +- toxcore/onion_client.h | 2 +- 4 files changed, 59 insertions(+), 21 deletions(-) diff --git a/auto_tests/friend_request_test.c b/auto_tests/friend_request_test.c index 7dfc5bf870..18bc5d62b7 100644 --- a/auto_tests/friend_request_test.c +++ b/auto_tests/friend_request_test.c @@ -10,44 +10,57 @@ #include "../toxcore/tox.h" #include "../toxcore/util.h" #include "../testing/misc_tools.h" + #include "auto_test_support.h" #include "check_compat.h" -#define FR_MESSAGE "Gentoo" +#define REQUEST_MESSAGE "Hello, I would like to be your friend. Please respond." + +typedef struct Callback_Data { + Tox *tox1; // request sender + Tox *tox2; // request receiver + uint8_t message[TOX_MAX_FRIEND_REQUEST_LENGTH]; + uint16_t length; +} Callback_Data; -static void accept_friend_request(const Tox_Event_Friend_Request *event, - void *userdata) +static void accept_friend_request(const Tox_Event_Friend_Request *event, void *userdata) { - Tox *state_tox = (Tox *)userdata; + Callback_Data *cb_data = (Callback_Data *)userdata; const uint8_t *public_key = tox_event_friend_request_get_public_key(event); const uint8_t *data = tox_event_friend_request_get_message(event); const size_t length = tox_event_friend_request_get_message_length(event); - ck_assert_msg(length == sizeof(FR_MESSAGE) && memcmp(FR_MESSAGE, data, sizeof(FR_MESSAGE)) == 0, + ck_assert_msg(length == cb_data->length && memcmp(cb_data->message, data, cb_data->length) == 0, "unexpected friend request message"); - tox_friend_add_norequest(state_tox, public_key, nullptr); + + fprintf(stderr, "Tox2 accepts friend request.\n"); + + Tox_Err_Friend_Add err; + tox_friend_add_norequest(cb_data->tox2, public_key, &err); + + ck_assert_msg(err == TOX_ERR_FRIEND_ADD_OK, "tox_friend_add_norequest failed: %d", err); } -static void iterate2_wait(const Tox_Dispatch *dispatch, Tox *tox1, Tox *tox2) +static void iterate2_wait(const Tox_Dispatch *dispatch, Callback_Data *cb_data) { Tox_Err_Events_Iterate err; Tox_Events *events; - events = tox_events_iterate(tox1, true, &err); + events = tox_events_iterate(cb_data->tox1, true, &err); ck_assert(err == TOX_ERR_EVENTS_ITERATE_OK); - tox_dispatch_invoke(dispatch, events, tox1); + tox_dispatch_invoke(dispatch, events, cb_data); tox_events_free(events); - events = tox_events_iterate(tox2, true, &err); + events = tox_events_iterate(cb_data->tox2, true, &err); ck_assert(err == TOX_ERR_EVENTS_ITERATE_OK); - tox_dispatch_invoke(dispatch, events, tox2); + tox_dispatch_invoke(dispatch, events, cb_data); tox_events_free(events); c_sleep(ITERATION_INTERVAL); } -static void test_friend_request(void) +static void test_friend_request(const uint8_t *message, uint16_t length) { printf("Initialising 2 toxes.\n"); uint32_t index[] = { 1, 2 }; @@ -60,7 +73,7 @@ static void test_friend_request(void) tox_events_init(tox1); tox_events_init(tox2); - printf("Bootstrapping tox2 off tox1.\n"); + printf("Bootstrapping Tox2 off Tox1.\n"); uint8_t dht_key[TOX_PUBLIC_KEY_SIZE]; tox_self_get_dht_id(tox1, dht_key); const uint16_t dht_port = tox_self_get_udp_port(tox1, nullptr); @@ -70,25 +83,36 @@ static void test_friend_request(void) Tox_Dispatch *dispatch = tox_dispatch_new(nullptr); ck_assert(dispatch != nullptr); + Callback_Data cb_data = {nullptr}; + cb_data.tox1 = tox1; + cb_data.tox2 = tox2; + + ck_assert(length <= sizeof(cb_data.message)); + memcpy(cb_data.message, message, length); + cb_data.length = length; + do { - iterate2_wait(dispatch, tox1, tox2); + iterate2_wait(dispatch, &cb_data); } while (tox_self_get_connection_status(tox1) == TOX_CONNECTION_NONE || tox_self_get_connection_status(tox2) == TOX_CONNECTION_NONE); printf("Toxes are online, took %lu seconds.\n", (unsigned long)(time(nullptr) - cur_time)); const time_t con_time = time(nullptr); - printf("Tox1 adds tox2 as friend, tox2 accepts.\n"); + printf("Tox1 adds Tox2 as friend. Waiting for Tox2 to accept.\n"); tox_events_callback_friend_request(dispatch, accept_friend_request); uint8_t address[TOX_ADDRESS_SIZE]; tox_self_get_address(tox2, address); - const uint32_t test = tox_friend_add(tox1, address, (const uint8_t *)FR_MESSAGE, sizeof(FR_MESSAGE), nullptr); + Tox_Err_Friend_Add err; + const uint32_t test = tox_friend_add(tox1, address, message, length, &err); + + ck_assert_msg(err == TOX_ERR_FRIEND_ADD_OK, "tox_friend_add failed: %d", err); ck_assert_msg(test == 0, "failed to add friend error code: %u", test); do { - iterate2_wait(dispatch, tox1, tox2); + iterate2_wait(dispatch, &cb_data); } while (tox_friend_get_connection_status(tox1, 0, nullptr) != TOX_CONNECTION_UDP || tox_friend_get_connection_status(tox2, 0, nullptr) != TOX_CONNECTION_UDP); @@ -104,6 +128,20 @@ int main(void) { setvbuf(stdout, nullptr, _IONBF, 0); - test_friend_request(); + fprintf(stderr, "Testing friend request with the smallest allowed message length.\n"); + test_friend_request((const uint8_t *)"a", 1); + + fprintf(stderr, "Testing friend request with an average sized message length.\n"); + test_friend_request((const uint8_t *)REQUEST_MESSAGE, sizeof(REQUEST_MESSAGE) - 1); + + uint8_t long_message[TOX_MAX_FRIEND_REQUEST_LENGTH]; + + for (uint16_t i = 0; i < sizeof(long_message); ++i) { + long_message[i] = 'a'; + } + + fprintf(stderr, "Testing friend request with the largest allowed message length.\n"); + test_friend_request(long_message, sizeof(long_message)); + return 0; } diff --git a/toxcore/friend_connection.c b/toxcore/friend_connection.c index 6527dda53b..9c44288511 100644 --- a/toxcore/friend_connection.c +++ b/toxcore/friend_connection.c @@ -875,7 +875,7 @@ void set_friend_request_callback(Friend_Connections *fr_c, fr_request_cb *fr_req int send_friend_request_packet(Friend_Connections *fr_c, int friendcon_id, uint32_t nospam_num, const uint8_t *data, uint16_t length) { - // FIXME: This max packet size is too large to be handled by receiving clients + // TODO(Jfreegman): This max packet size is too large to be handled by receiving clients // when sent via the onion. We currently limit the length at a higher level, but // this bounds check should be fixed to represent the max size of a packet that // the onion client can handle. diff --git a/toxcore/friend_requests.h b/toxcore/friend_requests.h index 7e200c2d43..4f3ed0a8ab 100644 --- a/toxcore/friend_requests.h +++ b/toxcore/friend_requests.h @@ -14,7 +14,7 @@ #include "attributes.h" #include "friend_connection.h" -// FIXME: This should be the maximum size that an onion client can handle. +// TODO(Jfreegman): This should be the maximum size that an onion client can handle. #define MAX_FRIEND_REQUEST_DATA_SIZE (ONION_CLIENT_MAX_DATA_SIZE - 100) typedef struct Friend_Requests Friend_Requests; diff --git a/toxcore/onion_client.h b/toxcore/onion_client.h index b6a4f1a6bd..92be110322 100644 --- a/toxcore/onion_client.h +++ b/toxcore/onion_client.h @@ -184,7 +184,7 @@ unsigned int onion_getfriend_dht_pubkey(const Onion_Client *onion_c, int friend_ #define ONION_DATA_IN_RESPONSE_MIN_SIZE (CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_MAC_SIZE) -// FIXME: This is not the correct value; data this large will be dropped by the onion client. +// TODO(Jfreegman): This is not the correct value; data this large will be dropped by the onion client. #define ONION_CLIENT_MAX_DATA_SIZE (MAX_DATA_REQUEST_SIZE - ONION_DATA_IN_RESPONSE_MIN_SIZE) /** @brief Send data of length length to friendnum.