commit 5479ffabf8ec1a3c63e9f237c6696c4ef0b74a55 Author: David Goulet <dgoulet@torproject.org> Date: Wed May 22 10:37:27 2019 -0400 sendme: Always pop last SENDME digest from circuit We must not accumulate digests on the circuit if the other end point is using another SENDME version that is not using those digests like v0. This commit makes it that we always pop the digest regardless of the version. Part of #30428 Signed-off-by: David Goulet <dgoulet@torproject.org> --- src/core/or/sendme.c | 70 +++++++++++++++++++++++++++++------------------ src/test/test_relaycell.c | 5 ++++ src/test/test_sendme.c | 4 +++ 3 files changed, 52 insertions(+), 27 deletions(-) diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index d914ba5e2..bfbb65e85 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -47,47 +47,46 @@ get_accept_min_version(void) SENDME_ACCEPT_MIN_VERSION_MAX); } -/* Return true iff the given cell digest matches the first digest in the - * circuit sendme list. */ -static bool -v1_digest_matches(const circuit_t *circ, const uint8_t *cell_digest) +/* Pop the first cell digset on the given circuit from the SENDME last digests + * list. NULL is returned if the list is uninitialized or empty. + * + * The caller gets ownership of the returned digest thus is responsible for + * freeing the memory. */ +static uint8_t * +pop_first_cell_digest(const circuit_t *circ) { - bool ret = false; - uint8_t *circ_digest = NULL; + uint8_t *circ_digest; tor_assert(circ); - tor_assert(cell_digest); - /* We shouldn't have received a SENDME if we have no digests. Log at - * protocol warning because it can be tricked by sending many SENDMEs - * without prior data cell. */ if (circ->sendme_last_digests == NULL || smartlist_len(circ->sendme_last_digests) == 0) { - log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, - "We received a SENDME but we have no cell digests to match. " - "Closing circuit."); - goto no_match; + return NULL; } - /* Pop the first element that was added (FIFO) and compare it. */ circ_digest = smartlist_get(circ->sendme_last_digests, 0); smartlist_del_keeporder(circ->sendme_last_digests, 0); + return circ_digest; +} + +/* Return true iff the given cell digest matches the first digest in the + * circuit sendme list. */ +static bool +v1_digest_matches(const uint8_t *circ_digest, const uint8_t *cell_digest) +{ + tor_assert(circ_digest); + tor_assert(cell_digest); /* Compare the digest with the one in the SENDME. This cell is invalid * without a perfect match. */ if (tor_memneq(circ_digest, cell_digest, TRUNNEL_SENDME_V1_DIGEST_LEN)) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "SENDME v1 cell digest do not match."); - goto no_match; + return false; } - /* Digests matches! */ - ret = true; - no_match: - /* This digest was popped from the circuit list. Regardless of what happens, - * we have no more use for it. */ - tor_free(circ_digest); - return ret; + /* Digests matches! */ + return true; } /* Return true iff the given decoded SENDME version 1 cell is valid and @@ -97,13 +96,13 @@ v1_digest_matches(const circuit_t *circ, const uint8_t *cell_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 circuit_t *circ) +cell_v1_is_valid(const sendme_cell_t *cell, const uint8_t *circ_digest) { tor_assert(cell); - tor_assert(circ); + tor_assert(circ_digest); const uint8_t *cell_digest = sendme_cell_getconstarray_data_v1_digest(cell); - return v1_digest_matches(circ, cell_digest); + return v1_digest_matches(circ_digest, cell_digest); } /* Return true iff the given cell version can be handled or if the minimum @@ -160,6 +159,7 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, size_t cell_payload_len) { uint8_t cell_version; + uint8_t *circ_digest = NULL; sendme_cell_t *cell = NULL; tor_assert(circ); @@ -184,10 +184,24 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, 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. */ + circ_digest = pop_first_cell_digest(circ); + if (circ_digest == NULL) { + /* We shouldn't have received a SENDME if we have no digests. Log at + * protocol warning because it can be tricked by sending many SENDMEs + * without prior data cell. */ + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "We received a SENDME but we have no cell digests to match. " + "Closing circuit."); + goto invalid; + } + /* Validate depending on the version now. */ switch (cell_version) { case 0x01: - if (!cell_v1_is_valid(cell, circ)) { + if (!cell_v1_is_valid(cell, circ_digest)) { goto invalid; } break; @@ -204,9 +218,11 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, /* Valid cell. */ sendme_cell_free(cell); + tor_free(circ_digest); return true; invalid: sendme_cell_free(cell); + tor_free(circ_digest); return false; } diff --git a/src/test/test_relaycell.c b/src/test/test_relaycell.c index d6372d395..c65279fb2 100644 --- a/src/test/test_relaycell.c +++ b/src/test/test_relaycell.c @@ -17,6 +17,7 @@ #include "core/or/circuitbuild.h" #include "core/or/circuitlist.h" #include "core/or/connection_edge.h" +#include "core/or/sendme.h" #include "core/or/relay.h" #include "test/test.h" #include "test/log_test_helpers.h" @@ -813,6 +814,10 @@ test_circbw_relay(void *arg) /* Sendme on circuit with non-full window: counted */ PACK_CELL(0, RELAY_COMMAND_SENDME, ""); + /* Recording a cell, the window is updated after decryption so off by one in + * order to record and then we process it with the proper window. */ + circ->cpath->package_window = 901; + sendme_record_cell_digest_on_circ(TO_CIRCUIT(circ), circ->cpath); circ->cpath->package_window = 900; connection_edge_process_relay_cell(&cell, TO_CIRCUIT(circ), edgeconn, circ->cpath); diff --git a/src/test/test_sendme.c b/src/test/test_sendme.c index a36c904c2..fa5ae115a 100644 --- a/src/test/test_sendme.c +++ b/src/test/test_sendme.c @@ -143,11 +143,13 @@ test_v1_build_cell(void *arg) or_circ = or_circuit_new(1, NULL); circ = TO_CIRCUIT(or_circ); + circ->sendme_last_digests = smartlist_new(); cell_digest = crypto_digest_new(); tt_assert(cell_digest); crypto_digest_add_bytes(cell_digest, "AAAAAAAAAAAAAAAAAAAA", 20); crypto_digest_get_digest(cell_digest, (char *) digest, sizeof(digest)); + smartlist_add(circ->sendme_last_digests, tor_memdup(digest, sizeof(digest))); /* SENDME v1 payload is 3 bytes + 20 bytes digest. See spec. */ ret = build_cell_payload_v1(digest, payload); @@ -157,6 +159,8 @@ test_v1_build_cell(void *arg) /* An empty payload means SENDME version 0 thus valid. */ tt_int_op(sendme_is_valid(circ, 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);