From 0894dcb895da4fe4cebfee1bcdd3544075b0b8ad Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 17 Apr 2025 16:44:17 -0400 Subject: [PATCH] Abolish relay_header_t, except for testing. With this change we no longer have a separate and possibly divergent encoder for cells. Also, abolish the accessors in relay_cell.c: It turns out that they don't make sense with CGO. --- src/core/crypto/relay_crypto.c | 61 +++++++++------- src/core/crypto/relay_crypto.h | 3 +- src/core/or/crypt_path.c | 2 +- src/core/or/include.am | 1 - src/core/or/or.h | 6 ++ src/core/or/relay.c | 7 +- src/core/or/relay.h | 3 + src/core/or/relay_cell.c | 129 --------------------------------- src/core/or/relay_cell.h | 12 +-- 9 files changed, 52 insertions(+), 172 deletions(-) delete mode 100644 src/core/or/relay_cell.c diff --git a/src/core/crypto/relay_crypto.c b/src/core/crypto/relay_crypto.c index 92075d7118..b33d33215f 100644 --- a/src/core/crypto/relay_crypto.c +++ b/src/core/crypto/relay_crypto.c @@ -19,27 +19,33 @@ #include "core/or/relay.h" #include "core/crypto/relay_crypto.h" #include "core/or/sendme.h" +#include "lib/cc/ctassert.h" #include "core/or/cell_st.h" #include "core/or/or_circuit_st.h" #include "core/or/origin_circuit_st.h" -/** Update digest from the payload of cell. Assign integrity part to +/* TODO CGO: This file will be largely incorrect when we have + * CGO crypto. */ + +/* Offset of digest within relay cell body for v0 cells. */ +#define V0_DIGEST_OFFSET 5 +#define V0_DIGEST_LEN 4 +#define V0_RECOGNIZED_OFFSET 1 + +/** Update digest{ from the payload of cell. Assign integrity part to * cell. */ void -relay_set_digest(crypto_digest_t *digest, cell_t *cell) +relay_set_digest_v0(crypto_digest_t *digest, cell_t *cell) { - char integrity[4]; - relay_header_t rh; + char integrity[V0_DIGEST_LEN]; crypto_digest_add_bytes(digest, (char*)cell->payload, CELL_PAYLOAD_SIZE); - crypto_digest_get_digest(digest, integrity, 4); + crypto_digest_get_digest(digest, integrity, V0_DIGEST_LEN); // log_fn(LOG_DEBUG,"Putting digest of %u %u %u %u into relay cell.", // integrity[0], integrity[1], integrity[2], integrity[3]); - relay_header_unpack(&rh, cell->payload); - memcpy(rh.integrity, integrity, 4); - relay_header_pack(cell->payload, &rh); + memcpy(cell->payload + V0_DIGEST_OFFSET, integrity, V0_DIGEST_LEN); } /** Does the digest for this circuit indicate that this cell is for us? @@ -49,25 +55,25 @@ relay_set_digest(crypto_digest_t *digest, cell_t *cell) * and cell to their original state and return 0. */ static int -relay_digest_matches(crypto_digest_t *digest, cell_t *cell) +relay_digest_matches_v0(crypto_digest_t *digest, cell_t *cell) { uint32_t received_integrity, calculated_integrity; - relay_header_t rh; crypto_digest_checkpoint_t backup_digest; + CTASSERT(sizeof(uint32_t) == V0_DIGEST_LEN); + crypto_digest_checkpoint(&backup_digest, digest); - relay_header_unpack(&rh, cell->payload); - memcpy(&received_integrity, rh.integrity, 4); - memset(rh.integrity, 0, 4); - relay_header_pack(cell->payload, &rh); + memcpy(&received_integrity, cell->payload + V0_DIGEST_OFFSET, V0_DIGEST_LEN); + memset(cell->payload + V0_DIGEST_OFFSET, 0, V0_DIGEST_LEN); // log_fn(LOG_DEBUG,"Reading digest of %u %u %u %u from relay cell.", // received_integrity[0], received_integrity[1], // received_integrity[2], received_integrity[3]); crypto_digest_add_bytes(digest, (char*) cell->payload, CELL_PAYLOAD_SIZE); - crypto_digest_get_digest(digest, (char*) &calculated_integrity, 4); + crypto_digest_get_digest(digest, (char*) &calculated_integrity, + V0_DIGEST_LEN); int rv = 1; @@ -77,8 +83,8 @@ relay_digest_matches(crypto_digest_t *digest, cell_t *cell) /* restore digest to its old form */ crypto_digest_restore(digest, &backup_digest); /* restore the relay header */ - memcpy(rh.integrity, &received_integrity, 4); - relay_header_pack(cell->payload, &rh); + memcpy(cell->payload + V0_DIGEST_OFFSET, &received_integrity, + V0_DIGEST_LEN); rv = 0; } @@ -86,6 +92,12 @@ relay_digest_matches(crypto_digest_t *digest, cell_t *cell) return rv; } +static inline bool +relay_cell_is_recognized_v0(const cell_t *cell) +{ + return get_uint16(cell->payload + V0_RECOGNIZED_OFFSET) == 0; +} + /** Apply cipher to CELL_PAYLOAD_SIZE bytes of in * (in place). * @@ -146,8 +158,6 @@ relay_decrypt_cell(circuit_t *circ, cell_t *cell, cell_direction_t cell_direction, crypt_path_t **layer_hint, char *recognized) { - relay_header_t rh; - tor_assert(circ); tor_assert(cell); tor_assert(recognized); @@ -170,10 +180,10 @@ relay_decrypt_cell(circuit_t *circ, cell_t *cell, /* decrypt one layer */ cpath_crypt_cell(thishop, cell->payload, true); - relay_header_unpack(&rh, cell->payload); - if (rh.recognized == 0) { + if (relay_cell_is_recognized_v0(cell)) { /* it's possibly recognized. have to check digest to be sure. */ - if (relay_digest_matches(cpath_get_incoming_digest(thishop), cell)) { + if (relay_digest_matches_v0(cpath_get_incoming_digest(thishop), + cell)) { *recognized = 1; *layer_hint = thishop; return 0; @@ -196,10 +206,9 @@ relay_decrypt_cell(circuit_t *circ, cell_t *cell, relay_crypt_one_payload(crypto->f_crypto, cell->payload); - relay_header_unpack(&rh, cell->payload); - if (rh.recognized == 0) { + if (relay_cell_is_recognized_v0(cell)) { /* it's possibly recognized. have to check digest to be sure. */ - if (relay_digest_matches(crypto->f_digest, cell)) { + if (relay_digest_matches_v0(crypto->f_digest, cell)) { *recognized = 1; return 0; } @@ -248,7 +257,7 @@ void relay_encrypt_cell_inbound(cell_t *cell, or_circuit_t *or_circ) { - relay_set_digest(or_circ->crypto.b_digest, cell); + relay_set_digest_v0(or_circ->crypto.b_digest, cell); /* Record cell digest as the SENDME digest if need be. */ sendme_record_sending_cell_digest(TO_CIRCUIT(or_circ), NULL); diff --git a/src/core/crypto/relay_crypto.h b/src/core/crypto/relay_crypto.h index 6256a100d7..c6eaf28f45 100644 --- a/src/core/crypto/relay_crypto.h +++ b/src/core/crypto/relay_crypto.h @@ -36,7 +36,6 @@ void relay_crypt_one_payload(crypto_cipher_t *cipher, uint8_t *in); void -relay_set_digest(crypto_digest_t *digest, cell_t *cell); +relay_set_digest_v0(crypto_digest_t *digest, cell_t *cell); #endif /* !defined(TOR_RELAY_CRYPTO_H) */ - diff --git a/src/core/or/crypt_path.c b/src/core/or/crypt_path.c index f12bd964e3..76711a5445 100644 --- a/src/core/or/crypt_path.c +++ b/src/core/or/crypt_path.c @@ -201,7 +201,7 @@ cpath_get_incoming_digest(const crypt_path_t *cpath) void cpath_set_cell_forward_digest(crypt_path_t *cpath, cell_t *cell) { - relay_set_digest(cpath->pvt_crypto.f_digest, cell); + relay_set_digest_v0(cpath->pvt_crypto.f_digest, cell); } /************ cpath sendme API ***************************/ diff --git a/src/core/or/include.am b/src/core/or/include.am index 09ec617ce2..0b9323b909 100644 --- a/src/core/or/include.am +++ b/src/core/or/include.am @@ -30,7 +30,6 @@ LIBTOR_APP_A_SOURCES += \ src/core/or/protover.c \ src/core/or/reasons.c \ src/core/or/relay.c \ - src/core/or/relay_cell.c \ src/core/or/relay_msg.c \ src/core/or/scheduler.c \ src/core/or/scheduler_kist.c \ diff --git a/src/core/or/or.h b/src/core/or/or.h index 8204554245..3e18d65bbd 100644 --- a/src/core/or/or.h +++ b/src/core/or/or.h @@ -595,6 +595,11 @@ typedef struct destroy_cell_t destroy_cell_t; typedef struct destroy_cell_queue_t destroy_cell_queue_t; typedef struct ext_or_cmd_t ext_or_cmd_t; +#ifdef TOR_UNIT_TESTS +/* This is a vestigial type used only for testing. + * All current code should instead use relay_msg_t and related accessors. + */ + /** Beginning of a RELAY cell payload. */ typedef struct { uint8_t command; /**< The end-to-end relay command. */ @@ -603,6 +608,7 @@ typedef struct { char integrity[4]; /**< Used to tell whether cell is corrupted. */ uint16_t length; /**< How long is the payload body? */ } relay_header_t; +#endif typedef struct socks_request_t socks_request_t; typedef struct entry_port_cfg_t entry_port_cfg_t; diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 8aa74fafbb..a32416b314 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -506,6 +506,7 @@ relay_lookup_conn(circuit_t *circ, const relay_msg_t *msg, return NULL; /* probably a begin relay cell */ } +#ifdef TOR_UNIT_TESTS /** Pack the relay_header_t host-order structure src into * network-order in the buffer dest. See tor-spec.txt for details * about the wire format. @@ -532,6 +533,7 @@ relay_header_unpack(relay_header_t *dest, const uint8_t *src) memcpy(dest->integrity, src+5, 4); dest->length = ntohs(get_uint16(src+9)); } +#endif /** Convert the relay command into a human-readable string. */ const char * @@ -593,7 +595,6 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *orig_circ, const char *filename, int lineno)) { cell_t cell; - relay_header_t rh; cell_direction_t cell_direction; circuit_t *circ = orig_circ; @@ -619,6 +620,7 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *orig_circ, tor_assert(circ); + size_t msg_body_len; { relay_cell_fmt_t cell_format = relay_msg_get_format(circ, cpath_layer); relay_msg_t msg; @@ -633,6 +635,7 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *orig_circ, // There is no need to copy the payload here, so long as we don't // free it. msg.body = (uint8_t *) payload; + msg_body_len = msg.length; if (relay_msg_encode_cell(cell_format, &msg, &cell) < 0) { // This already gave a BUG warning, so no need to log. @@ -709,7 +712,7 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *orig_circ, /* Let's assume we're well-behaved: Anything that we decide to send is * valid, delivered data. */ - circuit_sent_valid_data(origin_circ, rh.length); + circuit_sent_valid_data(origin_circ, msg_body_len); } int ret = circuit_package_relay_cell(&cell, circ, cell_direction, diff --git a/src/core/or/relay.h b/src/core/or/relay.h index a63a465bf2..47dbd18ca4 100644 --- a/src/core/or/relay.h +++ b/src/core/or/relay.h @@ -28,8 +28,11 @@ int circuit_receive_relay_cell(cell_t *cell, circuit_t *circ, cell_direction_t cell_direction); size_t cell_queues_get_total_allocation(void); +#ifdef TOR_UNIT_TESTS void relay_header_pack(uint8_t *dest, const relay_header_t *src); void relay_header_unpack(relay_header_t *dest, const uint8_t *src); +#endif + MOCK_DECL(int, relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *circ, uint8_t relay_command, const char *payload, diff --git a/src/core/or/relay_cell.c b/src/core/or/relay_cell.c deleted file mode 100644 index 32fcfae0f9..0000000000 --- a/src/core/or/relay_cell.c +++ /dev/null @@ -1,129 +0,0 @@ -/* Copyright (c) 2023, The Tor Project, Inc. */ -/* See LICENSE for licensing information */ - -/** - * \file relay_cell.c - * \brief This file handles most of the encoding and parsing of relay cells for - * all supported versions. - **/ - -#include -#include -#define RELAY_CELL_PRIVATE - -#include "core/or/relay_cell.h" -#include "core/or/relay.h" - -#include "lib/arch/bytes.h" -#include "lib/crypt_ops/crypto_rand.h" -#include "lib/log/util_bug.h" - -/* - * NOTE: As a starter of this file, it is important to note that trunnel is not - * used due to its heavy reliance on memory allocation. Parsing and encoding - * cells is a critical piece of the fast path and MUST be done with little - * as possible memory allocation or copy. - * - * And so, you will notice the direct access into a cell_t memory and relying - * on offset instead of copying data into an object representing a header. - */ - -/* - * Relay cell header static values. - * - * We don't copy the header back into a structure for performance reason. When - * accessing the header, we simply use offset within the cell_t payload. - * - * NOTE: We might want to move to a nicer static data structure containing all - * these and indexed by version but for now with the very few relay cell - * protocol and considering the future of C-tor this is simpler and enough. - */ - -/* The "recognized" field by version. */ -#define RECOGNIZED_OFFSET_V0 (1) -#define RECOGNIZED_OFFSET_V1 (0) - -/* The "digest" field by version. */ -#define DIGEST_OFFSET_V0 (5) -#define DIGEST_OFFSET_V1 (2) - -/** - * TODO #41051: Remove this entirely. - * - * I've moved this functionality into relay_msg.c, where it lives - * more happily. This function shouldn't have any callsites left - * at the end of this branch. - **/ -void -relay_cell_pad_payload(cell_t *cell, size_t data_len) -{ - (void)cell; - (void)data_len; -} - -/** Return true iff the given cell recognized field is zero. */ -bool -relay_cell_is_recognized(const cell_t *cell) -{ - switch (cell->relay_cell_proto) { - case 0: return get_uint16(cell->payload + RECOGNIZED_OFFSET_V0) == 0; - case 1: return get_uint16(cell->payload + RECOGNIZED_OFFSET_V1) == 0; - default: - /* Reaching this means we've failed to validate the supported relay cell - * version. */ - tor_fragile_assert(); - return false; - } -} - -/** Return a pointer from inside the given cell pointing to the start of the - * relay cell digest for the given protocol version. - * - * This is part of the fast path. No memory allocation. */ -uint8_t * -relay_cell_get_digest(cell_t *cell) -{ - switch (cell->relay_cell_proto) { - case 0: return cell->payload + DIGEST_OFFSET_V0; - case 1: return cell->payload + DIGEST_OFFSET_V1; - default: - /* Reaching this means we've failed to validate the supported relay cell - * version. Return the start of the payload, it will simply never - * validate and ultimately will close the circuit. */ - tor_fragile_assert(); - return cell->payload; - } -} - -/** Return the relay cell digest length based on the given protocol version. */ -size_t -relay_cell_get_digest_len(const cell_t *cell) -{ -/* Specified in tor-spec.txt */ -#define RELAY_CELL_DIGEST_LEN_V0 (4) -#define RELAY_CELL_DIGEST_LEN_V1 (14) - - switch (cell->relay_cell_proto) { - case 0: return RELAY_CELL_DIGEST_LEN_V0; - case 1: return RELAY_CELL_DIGEST_LEN_V1; - default: - /* Reaching this means we've failed to validate the supported relay cell - * version. This length will simply never validate and ultimately the - * circuit will be closed. */ - tor_fragile_assert(); - return 0; - } -} - -/** Set the given cell_digest value into the cell for the given relay cell - * protocol version. - * - * This is part of the fast path. No memory allocation. */ -void -relay_cell_set_digest(cell_t *cell, uint8_t *new_cell_digest) -{ - uint8_t *cell_digest_ptr = relay_cell_get_digest(cell); - size_t cell_digest_len = relay_cell_get_digest_len(cell); - - memcpy(cell_digest_ptr, new_cell_digest, cell_digest_len); -} diff --git a/src/core/or/relay_cell.h b/src/core/or/relay_cell.h index 9468355196..ca3db7aa00 100644 --- a/src/core/or/relay_cell.h +++ b/src/core/or/relay_cell.h @@ -13,17 +13,7 @@ #include "core/or/cell_st.h" -/* TODO #41051: Most of these functions no longer make sense under CGO, - * and we are only going to use the new proto format with CGO. */ - -/* Getters. */ -bool relay_cell_is_recognized(const cell_t *cell); -uint8_t *relay_cell_get_digest(cell_t *cell); -size_t relay_cell_get_digest_len(const cell_t *cell); - -/* Setters. */ -void relay_cell_set_digest(cell_t *cell, uint8_t *cell_digest); -void relay_cell_pad_payload(cell_t *cell, size_t data_len); +/* TODO #41051: Fold this file into relay_msg.h */ /* * NOTE: The following are inlined for performance reasons. These values are -- 2.47.2