From: Nick Mathewson Date: Tue, 10 Jun 2025 16:24:54 +0000 (-0400) Subject: Enforce that SENDME tags have the expected length X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=093c15c7568a110a382aa881d9200587916f71fc;p=thirdparty%2Ftor.git Enforce that SENDME tags have the expected length The length is no longer a constant 20, but now depends on the relay crypto algorithm in use. --- diff --git a/src/core/crypto/relay_crypto.c b/src/core/crypto/relay_crypto.c index f9d72a1f48..42dadbf8c5 100644 --- a/src/core/crypto/relay_crypto.c +++ b/src/core/crypto/relay_crypto.c @@ -51,6 +51,20 @@ relay_crypto_get_sendme_tag(relay_crypto_t *crypto, tor_assert_unreached(); } +/** Return the length of SENDME tags generated by `crypto`. */ +size_t +relay_crypto_sendme_tag_len(const relay_crypto_t *crypto) +{ + tor_assert(crypto); + switch (crypto->kind) { + case RCK_TOR1: + return SENDME_TAG_LEN_TOR1; + case RCK_CGO: + return SENDME_TAG_LEN_CGO; + } + tor_assert_unreached(); +} + /** * Handle a single layer of client-side backward encryption * with crypto of an arbitary type. diff --git a/src/core/crypto/relay_crypto.h b/src/core/crypto/relay_crypto.h index 5cb29af673..28de4c5048 100644 --- a/src/core/crypto/relay_crypto.h +++ b/src/core/crypto/relay_crypto.h @@ -54,5 +54,6 @@ void relay_crypto_assert_ok(const relay_crypto_t *crypto); const uint8_t *relay_crypto_get_sendme_tag(relay_crypto_t *crypto, size_t *len_out); +size_t relay_crypto_sendme_tag_len(const relay_crypto_t *crypto); #endif /* !defined(TOR_RELAY_CRYPTO_H) */ diff --git a/src/core/or/circuit_st.h b/src/core/or/circuit_st.h index c28900e59f..0bc20ba1a2 100644 --- a/src/core/or/circuit_st.h +++ b/src/core/or/circuit_st.h @@ -144,7 +144,17 @@ struct circuit_t { * For example, position 2 (starting at 0) means that we've received 300 * cells so the 300th cell digest is kept at index 2. * - * At maximum, this list contains 200 bytes plus the smartlist overhead. */ + * At maximum, this list contains 200 bytes plus the smartlist overhead. + * + * The elements in this list are always of length SENDME_TAG_LEN_TOR1 + * (== DIGEST_LEN, == 20). The actual digests stored in those elements + * may be smaller, however, if another relay crypto algorithm is in use. + **/ + /* Note that this is a per-circuit field, although logically it might make + * more sense for it to be a per-hop field. That doesn't matter in C tor, + * since we don't send more than a single window of cells to any given + * relay except for the exit. + */ smartlist_t *sendme_last_digests; /** Temporary field used during circuits_handle_oom. */ diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 07b2f3984b..be9c15ed39 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -7,7 +7,9 @@ * creating/parsing cells and handling the content. */ +// For access to cpath pvt_crypto field. #define SENDME_PRIVATE +#define CRYPT_PATH_PRIVATE #include "core/or/or.h" @@ -108,18 +110,19 @@ v1_tag_matches(const uint8_t *circ_digest, * cell we saw which tells us that the other side has in fact seen that cell. * See proposal 289 for more details. */ static bool -cell_v1_is_valid(const sendme_cell_t *cell, const uint8_t *circ_digest) +cell_v1_is_valid(const sendme_cell_t *cell, const uint8_t *circ_digest, + size_t circ_digest_len) { tor_assert(cell); tor_assert(circ_digest); - // XXXX TODO: make sure that length is the length we _expected_. - size_t tag_len = sendme_cell_get_data_len(cell); if (! tag_len_ok(tag_len)) return false; if (sendme_cell_getlen_data_v1_digest(cell) < tag_len) return false; + if (tag_len != circ_digest_len) + return false; const uint8_t *cell_digest = sendme_cell_getconstarray_data_v1_digest(cell); return v1_tag_matches(circ_digest, cell_digest, tag_len); @@ -174,8 +177,16 @@ cell_version_can_be_handled(uint8_t cell_version) * This is the main critical function to make sure we can continue to * send/recv cells on a circuit. If the SENDME is invalid, the circuit should * be marked for close by the caller. */ +/* + * NOTE: This function uses `layer_hint` to determine + * what the sendme tag length will be, and nothing else. + * Notably, we _don't_ keep a separate queue + * of expected tags for each layer! + */ STATIC bool -sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, +sendme_is_valid(const circuit_t *circ, + const crypt_path_t *layer_hint, + const uint8_t *cell_payload, size_t cell_payload_len) { uint8_t cell_version; @@ -204,6 +215,19 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, goto invalid; } + /* Determine the expected tag length for this sendme. */ + size_t circ_expects_tag_len; + if (layer_hint) { + circ_expects_tag_len = + relay_crypto_sendme_tag_len(&layer_hint->pvt_crypto); + } else if (CIRCUIT_IS_ORCIRC(circ)) { + const or_circuit_t *or_circ = CONST_TO_OR_CIRCUIT(circ); + circ_expects_tag_len = relay_crypto_sendme_tag_len(&or_circ->crypto); + } else { + tor_assert_nonfatal_unreached(); + goto invalid; + } + /* Pop the first element that was added (FIFO). We do that regardless of the * version so we don't accumulate on the circuit if v0 is used by the other * end point. */ @@ -216,12 +240,10 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, "We received a SENDME but we have no cell digests to match. " "Closing circuit."); goto invalid; - } - - /* Validate depending on the version now. */ + } /* Validate depending on the version now. */ switch (cell_version) { case 0x01: - if (!cell_v1_is_valid(cell, circ_digest)) { + if (!cell_v1_is_valid(cell, circ_digest, circ_expects_tag_len)) { goto invalid; } break; @@ -507,7 +529,7 @@ sendme_process_circuit_level(crypt_path_t *layer_hint, /* Validate the SENDME cell. Depending on the version, different validation * can be done. An invalid SENDME requires us to close the circuit. */ - if (!sendme_is_valid(circ, cell_payload, cell_payload_len)) { + if (!sendme_is_valid(circ, layer_hint, cell_payload, cell_payload_len)) { return -END_CIRC_REASON_TORPROTOCOL; } diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index c21726f5c8..c76b6d3947 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -69,6 +69,7 @@ STATIC ssize_t build_cell_payload_v1(const uint8_t *cell_tag, size_t tag_len, uint8_t *payload); STATIC bool sendme_is_valid(const circuit_t *circ, + const crypt_path_t *layer_hint, const uint8_t *cell_payload, size_t cell_payload_len); STATIC bool circuit_sendme_cell_is_next(int deliver_window, diff --git a/src/test/test_sendme.c b/src/test/test_sendme.c index b633f35001..cde6dd5d29 100644 --- a/src/test/test_sendme.c +++ b/src/test/test_sendme.c @@ -159,20 +159,22 @@ test_v1_build_cell(void *arg) /* Validation. */ /* An empty payload means SENDME version 0 thus valid. */ - tt_int_op(sendme_is_valid(circ, payload, 0), OP_EQ, true); + tt_int_op(sendme_is_valid(circ, NULL, payload, 0), OP_EQ, true); /* Current phoney digest should have been popped. */ tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 0); /* An unparseable cell means invalid. */ setup_full_capture_of_logs(LOG_INFO); - tt_int_op(sendme_is_valid(circ, (const uint8_t *) "A", 1), OP_EQ, false); + tt_int_op(sendme_is_valid(circ, NULL, (const uint8_t *) "A", 1), + OP_EQ, false); expect_log_msg_containing("Unparseable SENDME cell received. " "Closing circuit."); teardown_capture_of_logs(); /* No cell digest recorded for this. */ setup_full_capture_of_logs(LOG_INFO); - tt_int_op(sendme_is_valid(circ, payload, sizeof(payload)), OP_EQ, false); + tt_int_op(sendme_is_valid(circ, NULL, payload, sizeof(payload)), + OP_EQ, false); expect_log_msg_containing("We received a SENDME but we have no cell digests " "to match. Closing circuit."); teardown_capture_of_logs(); @@ -182,7 +184,8 @@ test_v1_build_cell(void *arg) sendme_record_cell_digest_on_circ(circ, NULL); tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 1); setup_full_capture_of_logs(LOG_INFO); - tt_int_op(sendme_is_valid(circ, payload, sizeof(payload)), OP_EQ, false); + tt_int_op(sendme_is_valid(circ, NULL, payload, sizeof(payload)), + OP_EQ, false); /* After a validation, the last digests is always popped out. */ tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 0); expect_log_msg_containing("SENDME v1 cell digest do not match."); @@ -193,7 +196,8 @@ test_v1_build_cell(void *arg) circ->package_window = CIRCWINDOW_INCREMENT + 1; sendme_record_cell_digest_on_circ(circ, NULL); tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 1); - tt_int_op(sendme_is_valid(circ, payload, sizeof(payload)), OP_EQ, true); + tt_int_op(sendme_is_valid(circ, NULL, payload, sizeof(payload)), + OP_EQ, true); /* After a validation, the last digests is always popped out. */ tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 0);