From 533fc55f7e79f1688bce0e48720e734d826fd859 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 22 Jan 2024 11:55:04 -0500 Subject: [PATCH] prop346: Adapt old CC extension parsing to new code Signed-off-by: David Goulet --- src/core/crypto/onion_crypto.c | 99 +++++++++-- src/core/or/circuitbuild.c | 44 ++++- src/core/or/circuitbuild.h | 5 +- src/core/or/congestion_control_common.c | 223 ++++++------------------ src/core/or/congestion_control_common.h | 20 +-- 5 files changed, 189 insertions(+), 202 deletions(-) diff --git a/src/core/crypto/onion_crypto.c b/src/core/crypto/onion_crypto.c index a5f58cdabc..e892084115 100644 --- a/src/core/crypto/onion_crypto.c +++ b/src/core/crypto/onion_crypto.c @@ -139,9 +139,10 @@ parse_ntor3_server_ext(const uint8_t *data, size_t data_len, ret = parse_subproto_extension(field, params_out); break; case TRUNNEL_EXT_TYPE_CC_FIELD_REQUEST: - /* Ignore CC field, it is parsed elsewhere. */ + ret = congestion_control_ntor3_parse_ext_request(field, params_out); break; default: + /* We refuse unknown extensions. */ ret = false; break; } @@ -279,8 +280,9 @@ onion_skin_create(int type, return -1; size_t msg_len = 0; uint8_t *msg = NULL; - if (client_circ_negotiation_message(node, &msg, &msg_len) < 0) + if (!client_circ_negotiation_message(node, &msg, &msg_len)) { return -1; + } uint8_t *onion_skin = NULL; size_t onion_skin_len = 0; int status = onion_skin_ntor3_create( @@ -318,6 +320,81 @@ onion_skin_create(int type, return r; } +/* Avoid code duplication into one convenient macro. */ +#define TRN_ADD_FIELD(ext, field) \ + do { \ + trn_extension_add_fields(ext, field); \ + trn_extension_set_num(ext, trn_extension_get_num(ext) + 1); \ + } while (0) + +/** Build the ntorv3 extension response server side. + * + * The ext_circ_params must be coherent and valid values that we are ready to + * send back. The values in this object are also used to know if we have to + * build the response or not. + * + * Return true on success and the resp_msg_out contains the encoded extension. + * On error, false and everything is untouched. */ +static bool +build_ntor3_ext_response_server(const circuit_params_t *our_ns_params, + const circuit_params_t *ext_circ_params, + uint8_t **resp_msg_out, + size_t *resp_msg_len_out) +{ + uint8_t *encoded = NULL; + trn_extension_field_t *response = NULL; + trn_extension_t *ext = trn_extension_new(); + + /* Build the congestion control response. */ + if (ext_circ_params->cc_enabled) { + response = congestion_control_ntor3_build_ext_response(our_ns_params); + if (!response) { + goto err; + } + TRN_ADD_FIELD(ext, response); + } + + /* Encode response extension. */ + ssize_t encoded_len = trn_extension_encoded_len(ext); + if (BUG(encoded_len < 0)) { + goto err; + } + encoded = tor_malloc_zero(encoded_len); + if (BUG(trn_extension_encode(encoded, encoded_len, ext) < 0)) { + goto err; + } + *resp_msg_out = encoded; + *resp_msg_len_out = encoded_len; + + trn_extension_free(ext); + return true; + + err: + tor_free(encoded); + trn_extension_free(ext); + return false; +} + +/** Return true iff the given circ_params are coherent and valid based on + * our_ns_params and global configuration. + * + * For congestion control, the cc_enabled can be flipped depending on what is + * enabled server side. + * + * This function runs in a worker thread, so it can only inspect + * arguments and local variables. */ +static bool +validate_ntor3_params_server(const circuit_params_t *our_ns_params, + circuit_params_t *circ_params) +{ + /* Validation is done through changing the value from what we can do server + * side. Essentially, is congestion control enabled on our end or not? */ + circ_params->cc_enabled = + circ_params->cc_enabled && our_ns_params->cc_enabled; + + return true; +} + /** * Takes a param request message from the client, compares it to our * consensus parameters, and creates a reply message and output @@ -345,20 +422,20 @@ negotiate_v3_ntor_server_circ_params(const uint8_t *param_request_msg, goto err; } - /* Parse request. */ - ret = congestion_control_parse_ext_request(param_request_msg, - param_request_len); - if (ret < 0) { + /* Passing validation means our params_out are now valid and coherent and + * thus can be safely used it in our response and configuration. */ + if (!validate_ntor3_params_server(our_ns_params, params_out)) { goto err; } - params_out->cc_enabled = ret && our_ns_params->cc_enabled; - /* Build the response. */ - ret = congestion_control_build_ext_response(our_ns_params, params_out, - resp_msg_out, resp_msg_len_out); - if (ret < 0) { + /* Build the response extension if any. */ + if (!build_ntor3_ext_response_server(our_ns_params, params_out, + resp_msg_out, resp_msg_len_out)) { goto err; } + + /* At this point, the responses have been built and successfully encoded so + * we can set our sendme increment and start using it. */ params_out->sendme_inc_cells = our_ns_params->sendme_inc_cells; /* Success. */ diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c index d6e022e7fa..1c3c7f832d 100644 --- a/src/core/or/circuitbuild.c +++ b/src/core/or/circuitbuild.c @@ -2701,19 +2701,47 @@ circuit_upgrade_circuits_from_guard_wait(void) * given relay. Assumes we are using ntor v3, or some later version that * supports parameter negotiatoin. * - * On success, return 0 and pass back a message in the `out` parameters. - * Otherwise, return -1. + * On success, return true and pass back a message in the `out` parameters. + * Otherwise, return false. **/ -int -client_circ_negotiation_message(const extend_info_t *ei, - uint8_t **msg_out, +bool +client_circ_negotiation_message(const extend_info_t *ei, uint8_t **msg_out, size_t *msg_len_out) { + bool ret = false; + uint8_t *encoded = NULL; + trn_extension_t *ext = NULL; + trn_extension_field_t *field = NULL; + tor_assert(ei && msg_out && msg_len_out); - if (!ei->exit_supports_congestion_control) { - return -1; + ext = trn_extension_new(); + + if (congestion_control_enabled() && ei->exit_supports_congestion_control) { + field = congestion_control_build_ext_request(); + if (field) { + trn_extension_add_fields(ext, field); + trn_extension_set_num(ext, trn_extension_get_num(ext) + 1); + } } - return congestion_control_build_ext_request(msg_out, msg_len_out); + /* Encode extension. */ + ssize_t encoded_len = trn_extension_encoded_len(ext); + if (BUG(encoded_len < 0)) { + goto end; + } + encoded = tor_malloc_zero(encoded_len); + if (BUG(trn_extension_encode(encoded, encoded_len, ext) < 0)) { + tor_free(encoded); + goto end; + } + *msg_out = encoded; + *msg_len_out = encoded_len; + + /* Free everything, we've encoded the request now. */ + ret = true; + + end: + trn_extension_free(ext); + return ret; } diff --git a/src/core/or/circuitbuild.h b/src/core/or/circuitbuild.h index c76259fc29..1c659566ae 100644 --- a/src/core/or/circuitbuild.h +++ b/src/core/or/circuitbuild.h @@ -71,9 +71,8 @@ circuit_deliver_create_cell,(circuit_t *circ, const struct create_cell_t *create_cell, int relayed)); -int client_circ_negotiation_message(const extend_info_t *ei, - uint8_t **msg_out, - size_t *msg_len_out); +bool client_circ_negotiation_message(const extend_info_t *ei, + uint8_t **msg_out, size_t *msg_len_out); #ifdef CIRCUITBUILD_PRIVATE STATIC circid_t get_unique_circ_id_by_chan(channel_t *chan); diff --git a/src/core/or/congestion_control_common.c b/src/core/or/congestion_control_common.c index 7dc9dbafdd..5a0285aaf8 100644 --- a/src/core/or/congestion_control_common.c +++ b/src/core/or/congestion_control_common.c @@ -997,211 +997,96 @@ congestion_control_dispatch_cc_alg(congestion_control_t *cc, /** * Build an extension field request to negotiate congestion control. * - * If congestion control is enabled, field TRUNNEL_EXT_TYPE_CC_FIELD_REQUEST - * is created in msg_out. It is a single 0-length field that signifies that we - * want to use congestion control. The length of msg_out is provided via - * msg_len_out. - * - * If congestion control is not enabled, a payload with 0 extensions is created - * and returned. - * - * If there is a failure building the request, -1 is returned, else 0. - * - * *msg_out must be freed if the return value is 0. + * Return the extension field ready for encoding or NULL on error. */ -int -congestion_control_build_ext_request(uint8_t **msg_out, size_t *msg_len_out) +trn_extension_field_t * +congestion_control_build_ext_request(void) { - uint8_t *request = NULL; - trn_extension_t *ext = NULL; trn_extension_field_t *field = NULL; - ext = trn_extension_new(); + /* Build the extension field that will hold the CC field. */ + field = trn_extension_field_new(); + trn_extension_field_set_field_type(field, + TRUNNEL_EXT_TYPE_CC_FIELD_REQUEST); - /* With congestion control enabled, add the request, else it is an empty - * request in the payload. */ - - if (congestion_control_enabled()) { - /* Build the extension field that will hold the CC field. */ - field = trn_extension_field_new(); - trn_extension_field_set_field_type(field, - TRUNNEL_EXT_TYPE_CC_FIELD_REQUEST); - - /* No payload indicating a request to use congestion control. */ - trn_extension_field_set_field_len(field, 0); - - /* Build final extension. */ - trn_extension_add_fields(ext, field); - trn_extension_set_num(ext, 1); - } + /* No payload indicating a request to use congestion control. */ + trn_extension_field_set_field_len(field, 0); - /* Encode extension. */ - ssize_t ret = trn_extension_encoded_len(ext); - if (BUG(ret < 0)) { - goto err; - } - size_t request_len = ret; - request = tor_malloc_zero(request_len); - ret = trn_extension_encode(request, request_len, ext); - if (BUG(ret < 0)) { - tor_free(request); - goto err; - } - *msg_out = request; - *msg_len_out = request_len; - - /* Free everything, we've encoded the request now. */ - ret = 0; - - err: - trn_extension_free(ext); - return (int)ret; + return field; } /** * Parse a congestion control ntorv3 request payload for extensions. * - * On parsing failure, -1 is returned. - * - * If congestion control request is present, return 1. If it is not present, - * return 0. + * Return true iff the extension can be successfully parsed. * * WARNING: Called from CPU worker! Must not access any global state. + * WARNING: Data set in params out is NOT validated for coherence. */ -int -congestion_control_parse_ext_request(const uint8_t *msg, const size_t msg_len) +bool +congestion_control_ntor3_parse_ext_request(const trn_extension_field_t *ext, + circuit_params_t *params_out) { - ssize_t ret = 0; - trn_extension_t *ext = NULL; - size_t num_fields = 0; + tor_assert(ext); + tor_assert(trn_extension_field_get_field_type(ext) == + TRUNNEL_EXT_TYPE_CC_FIELD_REQUEST); - /* Parse extension from payload. */ - ret = trn_extension_parse(&ext, msg, msg_len); - if (ret < 0) { - goto end; - } - - /* No extension implies no support for congestion control. In this case, we - * simply return 0 to indicate CC is disabled. */ - if ((num_fields = trn_extension_get_num(ext)) == 0) { - ret = 0; - goto end; - } - - /* Go over all fields. If any field is TRUNNEL_EXT_TYPE_CC_FIELD_REQUEST, - * then congestion control is enabled. Ignore unknown fields. */ - for (size_t f = 0; f < num_fields; f++) { - const trn_extension_field_t *field = trn_extension_get_fields(ext, f); - if (field == NULL) { - ret = -1; - goto end; - } - - /* For congestion control to be enabled, we only need the field type. */ - if (trn_extension_field_get_field_type(field) == - TRUNNEL_EXT_TYPE_CC_FIELD_REQUEST) { - ret = 1; - break; - } - } - - end: - trn_extension_free(ext); - return (int)ret; + /* The presence of the extension is enough to indicate we want it enabled. */ + params_out->cc_enabled = true; + return true; } -/** - * Given our observed parameters for circuits and congestion control, - * as well as the parameters for the resulting circuit, build a response - * payload using extension fields into *msg_out, with length specified in - * *msg_out_len. - * - * If congestion control will be enabled, the extension field for - * TRUNNEL_EXT_TYPE_CC_FIELD_RESPONSE will contain the sendme_inc value. +/** Given our observed parameters for circuits and congestion control, as well + * as the parameters for the resulting circuit, build a response payload + * extension field. * - * If congestion control won't be enabled, an extension payload with 0 - * fields will be created. + * The extension field for TRUNNEL_NTORV3_EXT_TYPE_CC_RESPONSE will contain the + * sendme_inc value. * - * Return 0 if an extension payload was created in *msg_out, and -1 on - * error. + * Return the extension field on success else NULL. * - * *msg_out must be freed if the return value is 0. - * - * WARNING: Called from CPU worker! Must not access any global state. - */ -int -congestion_control_build_ext_response(const circuit_params_t *our_params, - const circuit_params_t *circ_params, - uint8_t **msg_out, size_t *msg_len_out) + * WARNING: Called from CPU worker! Must not access any global state. */ +trn_extension_field_t * +congestion_control_ntor3_build_ext_response(const circuit_params_t *our_params) { ssize_t ret; - uint8_t *request = NULL; - trn_extension_t *ext = NULL; trn_extension_field_t *field = NULL; trn_extension_field_cc_t *cc_field = NULL; tor_assert(our_params); - tor_assert(circ_params); - tor_assert(msg_out); - tor_assert(msg_len_out); - - ext = trn_extension_new(); - - if (circ_params->cc_enabled) { - /* Build the extension field that will hold the CC field. */ - field = trn_extension_field_new(); - trn_extension_field_set_field_type(field, - TRUNNEL_EXT_TYPE_CC_FIELD_RESPONSE); - - /* Build the congestion control field response. */ - cc_field = trn_extension_field_cc_new(); - trn_extension_field_cc_set_sendme_inc(cc_field, - our_params->sendme_inc_cells); - - ret = trn_extension_field_cc_encoded_len(cc_field); - if (BUG(ret <= 0)) { - trn_extension_field_free(field); - goto err; - } - size_t field_len = ret; - trn_extension_field_set_field_len(field, field_len); - trn_extension_field_setlen_field(field, field_len); - - uint8_t *field_array = trn_extension_field_getarray_field(field); - ret = trn_extension_field_cc_encode(field_array, - trn_extension_field_getlen_field(field), cc_field); - if (BUG(ret <= 0)) { - trn_extension_field_free(field); - goto err; - } - /* Build final extension. */ - trn_extension_add_fields(ext, field); - trn_extension_set_num(ext, 1); - } + /* Build the extension field that will hold the CC field. */ + field = trn_extension_field_new(); + trn_extension_field_set_field_type(field, + TRUNNEL_EXT_TYPE_CC_FIELD_RESPONSE); - /* Encode extension. */ - ret = trn_extension_encoded_len(ext); - if (BUG(ret < 0)) { + /* Build the congestion control field response. */ + cc_field = trn_extension_field_cc_new(); + trn_extension_field_cc_set_sendme_inc(cc_field, + our_params->sendme_inc_cells); + + ret = trn_extension_field_cc_encoded_len(cc_field); + if (BUG(ret <= 0)) { + trn_extension_field_free(field); + field = NULL; goto err; } - size_t request_len = ret; - request = tor_malloc_zero(request_len); - ret = trn_extension_encode(request, request_len, ext); - if (BUG(ret < 0)) { - tor_free(request); + size_t field_len = ret; + trn_extension_field_set_field_len(field, field_len); + trn_extension_field_setlen_field(field, field_len); + + uint8_t *field_array = trn_extension_field_getarray_field(field); + ret = trn_extension_field_cc_encode(field_array, + trn_extension_field_getlen_field(field), cc_field); + if (BUG(ret <= 0)) { + trn_extension_field_free(field); + field = NULL; goto err; } - *msg_out = request; - *msg_len_out = request_len; - - /* We've just encoded the extension, clean everything. */ - ret = 0; err: - trn_extension_free(ext); trn_extension_field_cc_free(cc_field); - return (int)ret; + return field; } /** Return true iff the given sendme increment is within the acceptable diff --git a/src/core/or/congestion_control_common.h b/src/core/or/congestion_control_common.h index e185a5d29e..a9454f493c 100644 --- a/src/core/or/congestion_control_common.h +++ b/src/core/or/congestion_control_common.h @@ -13,6 +13,8 @@ #include "core/or/crypt_path_st.h" #include "core/or/circuit_st.h" +#include "trunnel/extension.h" + /* The maximum whole number of cells that can fit in a * full TLS record. This is 31. */ #define TLS_RECORD_MAX_CELLS ((16 * 1024) / CELL_MAX_NETWORK_SIZE) @@ -66,17 +68,13 @@ void congestion_control_new_consensus_params(const networkstatus_t *ns); bool congestion_control_enabled(void); -int congestion_control_build_ext_request(uint8_t **msg_out, - size_t *msg_len_out); -int congestion_control_parse_ext_request(const uint8_t *msg, - const size_t msg_len); -int congestion_control_build_ext_response(const circuit_params_t *our_params, - const circuit_params_t *circ_params, - uint8_t **msg_out, - size_t *msg_len_out); -int congestion_control_parse_ext_response(const uint8_t *msg, - const size_t msg_len, - circuit_params_t *params_out); +bool congestion_control_ntor3_parse_ext_request( + const trn_extension_field_t *ext, + circuit_params_t *params_out); +trn_extension_field_t *congestion_control_build_ext_request(void); +trn_extension_field_t *congestion_control_ntor3_build_ext_response( + const circuit_params_t *our_params); + bool congestion_control_validate_sendme_increment(uint8_t sendme_inc); char *congestion_control_get_control_port_fields(const origin_circuit_t *); -- 2.47.3