diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 896180156b..712e59dc5b 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -121,8 +121,8 @@ problem file-size /src/core/or/policies.c 3249 problem function-size /src/core/or/policies.c:policy_summarize() 107 problem function-size /src/core/or/protover.c:protover_all_supported() 117 problem file-size /src/core/or/relay.c 3173 -problem function-size /src/core/or/relay.c:circuit_receive_relay_cell() 123 -problem function-size /src/core/or/relay.c:relay_send_command_from_edge_() 112 +problem function-size /src/core/or/relay.c:circuit_receive_relay_cell() 127 +problem function-size /src/core/or/relay.c:relay_send_command_from_edge_() 116 problem function-size /src/core/or/relay.c:connection_ap_process_end_not_open() 194 problem function-size /src/core/or/relay.c:connection_edge_process_relay_cell_not_open() 139 problem function-size /src/core/or/relay.c:connection_edge_process_relay_cell() 520 diff --git a/src/core/crypto/relay_crypto.c b/src/core/crypto/relay_crypto.c index 74cccd2223..8a285131a8 100644 --- a/src/core/crypto/relay_crypto.c +++ b/src/core/crypto/relay_crypto.c @@ -100,12 +100,22 @@ relay_crypto_get_sendme_digest(relay_crypto_t *crypto) return crypto->sendme_digest; } -/** Record the b_digest from crypto and put it in the sendme_digest. */ +/** Record the cell digest, indicated by is_foward_digest or not, as the + * SENDME cell digest. */ void -relay_crypto_record_sendme_digest(relay_crypto_t *crypto) +relay_crypto_record_sendme_digest(relay_crypto_t *crypto, + bool is_foward_digest) { + struct crypto_digest_t *digest; + tor_assert(crypto); - crypto_digest_get_digest(crypto->b_digest, (char *) crypto->sendme_digest, + + digest = crypto->b_digest; + if (is_foward_digest) { + digest = crypto->f_digest; + } + + crypto_digest_get_digest(digest, (char *) crypto->sendme_digest, sizeof(crypto->sendme_digest)); } @@ -161,11 +171,6 @@ relay_decrypt_cell(circuit_t *circ, cell_t *cell, if (relay_digest_matches(cpath_get_incoming_digest(thishop), cell)) { *recognized = 1; *layer_hint = thishop; - /* This cell is for us. Keep a record of this cell because we will - * use it in the next SENDME cell. */ - if (sendme_circuit_cell_is_next(thishop->deliver_window)) { - cpath_sendme_circuit_record_inbound_cell(thishop); - } return 0; } } @@ -213,6 +218,9 @@ relay_encrypt_cell_outbound(cell_t *cell, crypt_path_t *thishop; /* counter for repeated crypts */ cpath_set_cell_forward_digest(layer_hint, cell); + /* Record cell digest as the SENDME digest if need be. */ + sendme_record_sending_cell_digest(TO_CIRCUIT(circ), layer_hint); + thishop = layer_hint; /* moving from farthest to nearest hop */ do { @@ -237,11 +245,8 @@ relay_encrypt_cell_inbound(cell_t *cell, { relay_set_digest(or_circ->crypto.b_digest, cell); - /* We are about to send this cell outbound on the circuit. Keep a record of - * this cell if we are expecting that the next cell is a SENDME. */ - if (sendme_circuit_cell_is_next(TO_CIRCUIT(or_circ)->package_window)) { - sendme_circuit_record_outbound_cell(or_circ); - } + /* Record cell digest as the SENDME digest if need be. */ + sendme_record_sending_cell_digest(TO_CIRCUIT(or_circ), NULL); /* encrypt one layer */ relay_crypt_one_payload(or_circ->crypto.b_crypto, cell->payload); diff --git a/src/core/crypto/relay_crypto.h b/src/core/crypto/relay_crypto.h index 7f09219c7f..9478f8d359 100644 --- a/src/core/crypto/relay_crypto.h +++ b/src/core/crypto/relay_crypto.h @@ -28,7 +28,10 @@ void relay_crypto_clear(relay_crypto_t *crypto); void relay_crypto_assert_ok(const relay_crypto_t *crypto); uint8_t *relay_crypto_get_sendme_digest(relay_crypto_t *crypto); -void relay_crypto_record_sendme_digest(relay_crypto_t *crypto); + +void relay_crypto_record_sendme_digest(relay_crypto_t *crypto, + bool is_foward_digest); + void relay_crypt_one_payload(crypto_cipher_t *cipher, uint8_t *in); diff --git a/src/core/or/circuit_st.h b/src/core/or/circuit_st.h index a68547ecb1..499bf93d6b 100644 --- a/src/core/or/circuit_st.h +++ b/src/core/or/circuit_st.h @@ -115,12 +115,11 @@ struct circuit_t { * list can not contain more than 10 digests of DIGEST_LEN bytes (20). * * At position i in the list, the digest corresponds to the - * ((CIRCWINDOW_INCREMENT * i) - 1)-nth cell received since we expect the - * (CIRCWINDOW_INCREMENT * i)-nth cell to be the SENDME and thus containing - * the previous cell digest. + * (CIRCWINDOW_INCREMENT * i)-nth cell received since we expect a SENDME to + * be received containing that cell digest. * - * For example, position 2 (starting at 0) means that we've received 299 - * cells and the 299th cell digest is kept at index 2. + * 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. */ smartlist_t *sendme_last_digests; diff --git a/src/core/or/crypt_path.c b/src/core/or/crypt_path.c index a4b7190e21..6d5245510f 100644 --- a/src/core/or/crypt_path.c +++ b/src/core/or/crypt_path.c @@ -204,15 +204,6 @@ cpath_set_cell_forward_digest(crypt_path_t *cpath, cell_t *cell) /************ cpath sendme API ***************************/ -/** Keep the current inbound cell digest for the next SENDME digest. This part - * is only done by the client as the circuit came back from the Exit. */ -void -cpath_sendme_circuit_record_inbound_cell(crypt_path_t *cpath) -{ - tor_assert(cpath); - relay_crypto_record_sendme_digest(&cpath->pvt_crypto); -} - /** Return the sendme_digest of this cpath. */ uint8_t * cpath_get_sendme_digest(crypt_path_t *cpath) @@ -220,6 +211,15 @@ cpath_get_sendme_digest(crypt_path_t *cpath) return relay_crypto_get_sendme_digest(&cpath->pvt_crypto); } +/** Record the cell digest, indicated by is_foward_digest or not, as the + * SENDME cell digest. */ +void +cpath_sendme_record_cell_digest(crypt_path_t *cpath, bool is_foward_digest) +{ + tor_assert(cpath); + relay_crypto_record_sendme_digest(&cpath->pvt_crypto, is_foward_digest); +} + /************ other cpath functions ***************************/ /** Return the first non-open hop in cpath, or return NULL if all diff --git a/src/core/or/crypt_path.h b/src/core/or/crypt_path.h index 30c14b3dce..9850610ef7 100644 --- a/src/core/or/crypt_path.h +++ b/src/core/or/crypt_path.h @@ -27,6 +27,9 @@ cpath_crypt_cell(const crypt_path_t *cpath, uint8_t *payload, bool is_decrypt); struct crypto_digest_t * cpath_get_incoming_digest(const crypt_path_t *cpath); +void cpath_sendme_record_cell_digest(crypt_path_t *cpath, + bool is_foward_digest); + void cpath_set_cell_forward_digest(crypt_path_t *cpath, cell_t *cell); diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 066ef19dbf..5197ee6138 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -247,6 +247,10 @@ circuit_receive_relay_cell(cell_t *cell, circuit_t *circ, if (recognized) { edge_connection_t *conn = NULL; + /* Recognized cell, the cell digest has been updated, we'll record it for + * the SENDME if need be. */ + sendme_record_received_cell_digest(circ, layer_hint); + if (circ->purpose == CIRCUIT_PURPOSE_PATH_BIAS_TESTING) { if (pathbias_check_probe_response(circ, cell) == -1) { pathbias_count_valid_cells(circ, cell); @@ -701,7 +705,7 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *circ, * we need to. This call needs to be after the circuit_package_relay_cell() * because the cell digest is set within that function. */ if (relay_command == RELAY_COMMAND_DATA) { - sendme_record_cell_digest(circ); + sendme_record_cell_digest_on_circ(circ, cpath_layer); } return 0; diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 0743c48c32..7d409a16ad 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -25,20 +25,6 @@ #include "lib/ctime/di_ops.h" #include "trunnel/sendme.h" -/* The maximum supported version. Above that value, the cell can't be - * recognized as a valid SENDME. */ -#define SENDME_MAX_SUPPORTED_VERSION 1 - -/* The cell version constants for when emitting a cell. */ -#define SENDME_EMIT_MIN_VERSION_DEFAULT 0 -#define SENDME_EMIT_MIN_VERSION_MIN 0 -#define SENDME_EMIT_MIN_VERSION_MAX UINT8_MAX - -/* The cell version constants for when accepting a cell. */ -#define SENDME_ACCEPT_MIN_VERSION_DEFAULT 0 -#define SENDME_ACCEPT_MIN_VERSION_MIN 0 -#define SENDME_ACCEPT_MIN_VERSION_MAX UINT8_MAX - /* Return the minimum version given by the consensus (if any) that should be * used when emitting a SENDME cell. */ STATIC int @@ -61,47 +47,53 @@ get_accept_min_version(void) SENDME_ACCEPT_MIN_VERSION_MAX); } +/* 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) +{ + uint8_t *circ_digest; + + tor_assert(circ); + + if (circ->sendme_last_digests == NULL || + smartlist_len(circ->sendme_last_digests) == 0) { + return NULL; + } + + /* More cell digest than the SENDME window is never suppose to happen. The + * cell should have been rejected before reaching this point due to its + * package_window down to 0 leading to a circuit close. Scream loudly but + * still pop the element so we don't memory leak. */ + tor_assert_nonfatal(smartlist_len(circ->sendme_last_digests) <= + CIRCWINDOW_START_MAX / CIRCWINDOW_INCREMENT); + + 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 circuit_t *circ, const uint8_t *cell_digest) +v1_digest_matches(const uint8_t *circ_digest, const uint8_t *cell_digest) { - bool ret = false; - uint8_t *circ_digest = NULL; - - tor_assert(circ); + tor_assert(circ_digest); 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; - } - - /* 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); - /* 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 @@ -111,42 +103,53 @@ 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 * accepted version from the consensus is known to us. */ STATIC bool -cell_version_is_valid(uint8_t cell_version) +cell_version_can_be_handled(uint8_t cell_version) { int accept_version = get_accept_min_version(); - /* Can we handle this version? */ + /* We will first check if the consensus minimum accepted version can be + * handled by us and if not, regardless of the cell version we got, we can't + * continue. */ if (accept_version > SENDME_MAX_SUPPORTED_VERSION) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "Unable to accept SENDME version %u (from consensus). " - "We only support <= %d. Probably your tor is too old?", - accept_version, cell_version); + "We only support <= %u. Probably your tor is too old?", + accept_version, SENDME_MAX_SUPPORTED_VERSION); goto invalid; } - /* We only accept a SENDME cell from what the consensus tells us. */ + /* Then, is this version below the accepted version from the consensus? If + * yes, we must not handle it. */ if (cell_version < accept_version) { - log_info(LD_PROTOCOL, "Unacceptable SENDME version %d. Only " + log_info(LD_PROTOCOL, "Unacceptable SENDME version %u. Only " "accepting %u (from consensus). Closing circuit.", cell_version, accept_version); goto invalid; } - return 1; + /* Is this cell version supported by us? */ + if (cell_version > SENDME_MAX_SUPPORTED_VERSION) { + log_info(LD_PROTOCOL, "SENDME cell version %u is not supported by us. " + "We only support <= %u", + cell_version, SENDME_MAX_SUPPORTED_VERSION); + goto invalid; + } + + return true; invalid: - return 0; + return false; } /* Return true iff the encoded SENDME cell in cell_payload of length @@ -163,6 +166,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); @@ -183,31 +187,50 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, } /* Validate that we can handle this cell version. */ - if (!cell_version_is_valid(cell_version)) { + if (!cell_version_can_be_handled(cell_version)) { + 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; case 0x00: - /* Fallthrough. Version 0, there is no work to be done on the payload so - * it is necessarily valid if we pass the version validation. */ + /* Version 0, there is no work to be done on the payload so it is + * necessarily valid if we pass the version validation. */ + break; default: - /* Unknown version means we can't handle it so fallback to version 0. */ + log_warn(LD_PROTOCOL, "Unknown SENDME cell version %d received.", + cell_version); + tor_assert_nonfatal_unreached(); break; } /* Valid cell. */ sendme_cell_free(cell); - return 1; + tor_free(circ_digest); + return true; invalid: sendme_cell_free(cell); - return 0; + tor_free(circ_digest); + return false; } /* Build and encode a version 1 SENDME cell into payload, which must be at @@ -274,6 +297,8 @@ send_circuit_level_sendme(circuit_t *circ, crypt_path_t *layer_hint, default: /* Unknown version, fallback to version 0 meaning no payload. */ payload_len = 0; + log_debug(LD_PROTOCOL, "Emitting SENDME version 0 cell. " + "Consensus emit version is %d", emit_version); break; } @@ -287,34 +312,54 @@ send_circuit_level_sendme(circuit_t *circ, crypt_path_t *layer_hint, return 0; } +/* Record the cell digest only if the next cell is expected to be a SENDME. */ +static void +record_cell_digest_on_circ(circuit_t *circ, const uint8_t *sendme_digest) +{ + tor_assert(circ); + tor_assert(sendme_digest); + + /* Add the digest to the last seen list in the circuit. */ + if (circ->sendme_last_digests == NULL) { + circ->sendme_last_digests = smartlist_new(); + } + smartlist_add(circ->sendme_last_digests, + tor_memdup(sendme_digest, DIGEST_LEN)); +} + /* * Public API */ -/** Keep the current inbound cell digest for the next SENDME digest. This part - * is only done by the client as the circuit came back from the Exit. */ -void -sendme_circuit_record_outbound_cell(or_circuit_t *or_circ) -{ - tor_assert(or_circ); - relay_crypto_record_sendme_digest(&or_circ->crypto); -} - /** Return true iff the next cell for the given cell window is expected to be * a SENDME. * * We are able to know that because the package or deliver window value minus * one cell (the possible SENDME cell) should be a multiple of the increment * window value. */ -bool -sendme_circuit_cell_is_next(int window) +static bool +circuit_sendme_cell_is_next(int window) { - /* Is this the last cell before a SENDME? The idea is that if the package or - * deliver window reaches a multiple of the increment, after this cell, we - * should expect a SENDME. */ + /* At the start of the window, no SENDME will be expected. */ + if (window == CIRCWINDOW_START) { + return false; + } + + /* Are we at the limit of the increment and if not, we don't expect next + * cell is a SENDME. + * + * We test against the window minus 1 because when we are looking if the + * next cell is a SENDME, the window (either package or deliver) hasn't been + * decremented just yet so when this is called, we are currently processing + * the "window - 1" cell. + * + * This function is used when recording a cell digest and this is done quite + * low in the stack when decrypting or encrypting a cell. The window is only + * updated once the cell is actually put in the outbuf. */ if (((window - 1) % CIRCWINDOW_INCREMENT) != 0) { return false; } + /* Next cell is expected to be a SENDME. */ return true; } @@ -372,6 +417,7 @@ sendme_connection_edge_consider_sending(edge_connection_t *conn) void sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint) { + bool sent_one_sendme = false; const uint8_t *digest; while ((layer_hint ? layer_hint->deliver_window : circ->deliver_window) <= @@ -387,6 +433,12 @@ sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint) if (send_circuit_level_sendme(circ, layer_hint, digest) < 0) { return; /* The circuit's closed, don't continue */ } + /* Current implementation is not suppose to send multiple SENDME at once + * because this means we would use the same relay crypto digest for each + * SENDME leading to a mismatch on the other side and the circuit to + * collapse. Scream loudly if it ever happens so we can address it. */ + tor_assert_nonfatal(!sent_one_sendme); + sent_one_sendme = true; } } @@ -409,6 +461,12 @@ sendme_process_circuit_level(crypt_path_t *layer_hint, tor_assert(circ); tor_assert(cell_payload); + /* 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)) { + return -END_CIRC_REASON_TORPROTOCOL; + } + /* If we are the origin of the circuit, we are the Client so we use the * layer hint (the Exit hop) for the package window tracking. */ if (CIRCUIT_IS_ORIGIN(circ)) { @@ -433,13 +491,6 @@ sendme_process_circuit_level(crypt_path_t *layer_hint, * are rate limited. */ circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ), cell_payload_len); } else { - /* Validate the SENDME cell. Depending on the version, different - * validation can be done. An invalid SENDME requires us to close the - * circuit. It is only done if we are the Exit of the circuit. */ - if (!sendme_is_valid(circ, cell_payload, cell_payload_len)) { - return -END_CIRC_REASON_TORPROTOCOL; - } - /* We aren't the origin of this circuit so we are the Exit and thus we * track the package window with the circuit object. */ if ((circ->package_window + CIRCWINDOW_INCREMENT) > @@ -568,34 +619,90 @@ int sendme_note_stream_data_packaged(edge_connection_t *conn) { tor_assert(conn); - return --conn->package_window; + log_debug(LD_APP, "Stream package_window now %d.", --conn->package_window); + return conn->package_window; } -/* Note the cell digest in the circuit sendme last digests FIFO if applicable. - * It is safe to pass a circuit that isn't meant to track those digests. */ +/* Record the cell digest into the circuit sendme digest list depending on + * which edge we are. The digest is recorded only if we expect the next cell + * that we will receive is a SENDME so we can match the digest. */ void -sendme_record_cell_digest(circuit_t *circ) +sendme_record_cell_digest_on_circ(circuit_t *circ, crypt_path_t *cpath) { - const uint8_t *digest; + int package_window; + uint8_t *sendme_digest; tor_assert(circ); - /* We only keep the cell digest if we are the Exit on that circuit and if - * this cell is the last one before the client should send a SENDME. */ - if (CIRCUIT_IS_ORIGIN(circ)) { - return; + package_window = circ->package_window; + if (cpath) { + package_window = cpath->package_window; } + /* Is this the last cell before a SENDME? The idea is that if the * package_window reaches a multiple of the increment, after this cell, we * should expect a SENDME. */ - if (!sendme_circuit_cell_is_next(circ->package_window)) { + if (!circuit_sendme_cell_is_next(package_window)) { return; } - /* Add the digest to the last seen list in the circuit. */ - digest = relay_crypto_get_sendme_digest(&TO_OR_CIRCUIT(circ)->crypto); - if (circ->sendme_last_digests == NULL) { - circ->sendme_last_digests = smartlist_new(); + /* Getting the digest is expensive so we only do it once we are certain to + * record it on the circuit. */ + if (cpath) { + sendme_digest = cpath_get_sendme_digest(cpath); + } else { + sendme_digest = + relay_crypto_get_sendme_digest(&TO_OR_CIRCUIT(circ)->crypto); } - smartlist_add(circ->sendme_last_digests, tor_memdup(digest, DIGEST_LEN)); + + record_cell_digest_on_circ(circ, sendme_digest); +} + +/* Called once we decrypted a cell and recognized it. Record the cell digest + * as the next sendme digest only if the next cell we'll send on the circuit + * is expected to be a SENDME. */ +void +sendme_record_received_cell_digest(circuit_t *circ, crypt_path_t *cpath) +{ + tor_assert(circ); + + /* Only record if the next cell is expected to be a SENDME. */ + if (!circuit_sendme_cell_is_next(cpath ? cpath->deliver_window : + circ->deliver_window)) { + return; + } + + if (cpath) { + /* Record incoming digest. */ + cpath_sendme_record_cell_digest(cpath, false); + } else { + /* Record foward digest. */ + relay_crypto_record_sendme_digest(&TO_OR_CIRCUIT(circ)->crypto, true); + } +} + +/* Called once we encrypted a cell. Record the cell digest as the next sendme + * digest only if the next cell we expect to receive is a SENDME so we can + * match the digests. */ +void +sendme_record_sending_cell_digest(circuit_t *circ, crypt_path_t *cpath) +{ + tor_assert(circ); + + /* Only record if the next cell is expected to be a SENDME. */ + if (!circuit_sendme_cell_is_next(cpath ? cpath->package_window : + circ->package_window)) { + goto end; + } + + if (cpath) { + /* Record the forward digest. */ + cpath_sendme_record_cell_digest(cpath, true); + } else { + /* Record the incoming digest. */ + relay_crypto_record_sendme_digest(&TO_OR_CIRCUIT(circ)->crypto, false); + } + + end: + return; } diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index ac18bbdd31..cdbdf55ac7 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -34,16 +34,29 @@ int sendme_note_circuit_data_packaged(circuit_t *circ, crypt_path_t *layer_hint); int sendme_note_stream_data_packaged(edge_connection_t *conn); -/* Track cell digest. */ -void sendme_record_cell_digest(circuit_t *circ); -void sendme_circuit_record_outbound_cell(or_circuit_t *or_circ); - -/* Circuit level information. */ -bool sendme_circuit_cell_is_next(int window); +/* Record cell digest on circuit. */ +void sendme_record_cell_digest_on_circ(circuit_t *circ, crypt_path_t *cpath); +/* Record cell digest as the SENDME digest. */ +void sendme_record_received_cell_digest(circuit_t *circ, crypt_path_t *cpath); +void sendme_record_sending_cell_digest(circuit_t *circ, crypt_path_t *cpath); /* Private section starts. */ #ifdef SENDME_PRIVATE +/* The maximum supported version. Above that value, the cell can't be + * recognized as a valid SENDME. */ +#define SENDME_MAX_SUPPORTED_VERSION 1 + +/* The cell version constants for when emitting a cell. */ +#define SENDME_EMIT_MIN_VERSION_DEFAULT 0 +#define SENDME_EMIT_MIN_VERSION_MIN 0 +#define SENDME_EMIT_MIN_VERSION_MAX UINT8_MAX + +/* The cell version constants for when accepting a cell. */ +#define SENDME_ACCEPT_MIN_VERSION_DEFAULT 0 +#define SENDME_ACCEPT_MIN_VERSION_MIN 0 +#define SENDME_ACCEPT_MIN_VERSION_MAX UINT8_MAX + /* * Unit tests declaractions. */ @@ -52,7 +65,7 @@ bool sendme_circuit_cell_is_next(int window); STATIC int get_emit_min_version(void); STATIC int get_accept_min_version(void); -STATIC bool cell_version_is_valid(uint8_t cell_version); +STATIC bool cell_version_can_be_handled(uint8_t cell_version); STATIC ssize_t build_cell_payload_v1(const uint8_t *cell_digest, uint8_t *payload); diff --git a/src/test/test_relaycell.c b/src/test/test_relaycell.c index 0623583511..c65279fb25 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" @@ -812,7 +813,11 @@ test_circbw_relay(void *arg) ASSERT_UNCOUNTED_BW(); /* Sendme on circuit with non-full window: counted */ - PACK_CELL(0, RELAY_COMMAND_SENDME, "Data1234"); + 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 d40fbaf862..fa5ae115ac 100644 --- a/src/test/test_sendme.c +++ b/src/test/test_sendme.c @@ -46,26 +46,12 @@ static void test_v1_record_digest(void *arg) { or_circuit_t *or_circ = NULL; - origin_circuit_t *orig_circ = NULL; circuit_t *circ = NULL; (void) arg; - /* Create our dummy circuits. */ - orig_circ = origin_circuit_new(); - tt_assert(orig_circ); + /* Create our dummy circuit. */ or_circ = or_circuit_new(1, NULL); - - /* Start by pointing to the origin circuit. */ - circ = TO_CIRCUIT(orig_circ); - circ->purpose = CIRCUIT_PURPOSE_S_REND_JOINED; - - /* We should never note SENDME digest on origin circuit. */ - sendme_record_cell_digest(circ); - tt_assert(!circ->sendme_last_digests); - /* We do not need the origin circuit for now. */ - orig_circ = NULL; - circuit_free_(circ); /* Points it to the OR circuit now. */ circ = TO_CIRCUIT(or_circ); @@ -73,23 +59,23 @@ test_v1_record_digest(void *arg) * in order to catched the CIRCWINDOW_INCREMENT-nth cell. Try something that * shouldn't be noted. */ circ->package_window = CIRCWINDOW_INCREMENT; - sendme_record_cell_digest(circ); + sendme_record_cell_digest_on_circ(circ, NULL); tt_assert(!circ->sendme_last_digests); /* This should work now. Package window at CIRCWINDOW_INCREMENT + 1. */ circ->package_window++; - sendme_record_cell_digest(circ); + sendme_record_cell_digest_on_circ(circ, NULL); tt_assert(circ->sendme_last_digests); tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 1); /* Next cell in the package window shouldn't do anything. */ circ->package_window++; - sendme_record_cell_digest(circ); + sendme_record_cell_digest_on_circ(circ, NULL); tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 1); /* The next CIRCWINDOW_INCREMENT should add one more digest. */ circ->package_window = (CIRCWINDOW_INCREMENT * 2) + 1; - sendme_record_cell_digest(circ); + sendme_record_cell_digest_on_circ(circ, NULL); tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 2); done: @@ -136,9 +122,9 @@ test_v1_consensus_params(void *arg) smartlist_add(current_md_consensus->net_params, (void *) "sendme_accept_min_version=1"); /* Minimum acceptable value is 1. */ - tt_int_op(cell_version_is_valid(1), OP_EQ, true); + tt_int_op(cell_version_can_be_handled(1), OP_EQ, true); /* Minimum acceptable value is 1 so a cell version of 0 is refused. */ - tt_int_op(cell_version_is_valid(0), OP_EQ, false); + tt_int_op(cell_version_can_be_handled(0), OP_EQ, false); done: free_mock_consensus(); @@ -157,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); @@ -171,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); @@ -188,7 +178,7 @@ test_v1_build_cell(void *arg) /* Note the wrong digest in the circuit, cell should fail validation. */ circ->package_window = CIRCWINDOW_INCREMENT + 1; - sendme_record_cell_digest(circ); + 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); @@ -200,7 +190,7 @@ test_v1_build_cell(void *arg) /* Record the cell digest into the circuit, cell should validate. */ memcpy(or_circ->crypto.sendme_digest, digest, sizeof(digest)); circ->package_window = CIRCWINDOW_INCREMENT + 1; - sendme_record_cell_digest(circ); + 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); /* After a validation, the last digests is always popped out. */ @@ -253,6 +243,33 @@ test_cell_payload_pad(void *arg) ; } +static void +test_cell_version_validation(void *arg) +{ + (void) arg; + + /* We currently only support up to SENDME_MAX_SUPPORTED_VERSION so we are + * going to test the boundaries there. */ + + tt_assert(cell_version_can_be_handled(SENDME_MAX_SUPPORTED_VERSION)); + + /* Version below our supported should pass. */ + tt_assert(cell_version_can_be_handled(SENDME_MAX_SUPPORTED_VERSION - 1)); + + /* Extra version from our supported should fail. */ + tt_assert(!cell_version_can_be_handled(SENDME_MAX_SUPPORTED_VERSION + 1)); + + /* Simple check for version 0. */ + tt_assert(cell_version_can_be_handled(0)); + + /* We MUST handle the default cell version that we emit or accept. */ + tt_assert(cell_version_can_be_handled(SENDME_EMIT_MIN_VERSION_DEFAULT)); + tt_assert(cell_version_can_be_handled(SENDME_ACCEPT_MIN_VERSION_DEFAULT)); + + done: + ; +} + struct testcase_t sendme_tests[] = { { "v1_record_digest", test_v1_record_digest, TT_FORK, NULL, NULL }, @@ -262,6 +279,8 @@ struct testcase_t sendme_tests[] = { NULL, NULL }, { "cell_payload_pad", test_cell_payload_pad, TT_FORK, NULL, NULL }, + { "cell_version_validation", test_cell_version_validation, TT_FORK, + NULL, NULL }, END_OF_TESTCASES };