From e5806dcea891cf9c6aa4d55c6d5deae9792792d7 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 8 Jan 2019 11:31:32 -0500 Subject: [PATCH 01/32] sendme: Move code to the new files sendme.{c|h} Take apart the SENDME cell specific code and put it in sendme.{c|h}. This is part of prop289 that implements authenticated SENDMEs. Creating those new files allow for the already huge relay.c to not grow in LOC and makes it easier to handle and test the SENDME cells in an isolated way. This commit only moves code. No behavior change. Signed-off-by: David Goulet --- src/core/include.am | 2 + src/core/or/connection_edge.c | 5 ++- src/core/or/relay.c | 72 ++------------------------------- src/core/or/sendme.c | 76 +++++++++++++++++++++++++++++++++++ src/core/or/sendme.h | 20 +++++++++ 5 files changed, 105 insertions(+), 70 deletions(-) create mode 100644 src/core/or/sendme.c create mode 100644 src/core/or/sendme.h diff --git a/src/core/include.am b/src/core/include.am index 9824601725..067b6857ae 100644 --- a/src/core/include.am +++ b/src/core/include.am @@ -52,6 +52,7 @@ LIBTOR_APP_A_SOURCES = \ src/core/or/scheduler.c \ src/core/or/scheduler_kist.c \ src/core/or/scheduler_vanilla.c \ + src/core/or/sendme.c \ src/core/or/status.c \ src/core/or/versions.c \ src/core/proto/proto_cell.c \ @@ -272,6 +273,7 @@ noinst_HEADERS += \ src/core/or/relay.h \ src/core/or/relay_crypto_st.h \ src/core/or/scheduler.h \ + src/core/or/sendme.h \ src/core/or/server_port_cfg_st.h \ src/core/or/socks_request_st.h \ src/core/or/status.h \ diff --git a/src/core/or/connection_edge.c b/src/core/or/connection_edge.c index 33ba723971..fa72fb7716 100644 --- a/src/core/or/connection_edge.c +++ b/src/core/or/connection_edge.c @@ -73,6 +73,7 @@ #include "core/or/policies.h" #include "core/or/reasons.h" #include "core/or/relay.h" +#include "core/or/sendme.h" #include "core/proto/proto_http.h" #include "core/proto/proto_socks.h" #include "feature/client/addressmap.h" @@ -767,7 +768,7 @@ connection_edge_flushed_some(edge_connection_t *conn) /* falls through. */ case EXIT_CONN_STATE_OPEN: - connection_edge_consider_sending_sendme(conn); + sendme_connection_edge_consider_sending(conn); break; } return 0; @@ -791,7 +792,7 @@ connection_edge_finished_flushing(edge_connection_t *conn) switch (conn->base_.state) { case AP_CONN_STATE_OPEN: case EXIT_CONN_STATE_OPEN: - connection_edge_consider_sending_sendme(conn); + sendme_connection_edge_consider_sending(conn); return 0; case AP_CONN_STATE_SOCKS_WAIT: case AP_CONN_STATE_NATD_WAIT: diff --git a/src/core/or/relay.c b/src/core/or/relay.c index a166904a5f..7cfacf761a 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -93,13 +93,12 @@ #include "core/or/origin_circuit_st.h" #include "feature/nodelist/routerinfo_st.h" #include "core/or/socks_request_st.h" +#include "core/or/sendme.h" static edge_connection_t *relay_lookup_conn(circuit_t *circ, cell_t *cell, cell_direction_t cell_direction, crypt_path_t *layer_hint); -static void circuit_consider_sending_sendme(circuit_t *circ, - crypt_path_t *layer_hint); static void circuit_resume_edge_reading(circuit_t *circ, crypt_path_t *layer_hint); static int circuit_resume_edge_reading_helper(edge_connection_t *conn, @@ -1564,7 +1563,7 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, log_debug(domain,"circ deliver_window now %d.", layer_hint ? layer_hint->deliver_window : circ->deliver_window); - circuit_consider_sending_sendme(circ, layer_hint); + sendme_circuit_consider_sending(circ, layer_hint); if (rh.stream_id == 0) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "Relay data cell with zero " @@ -1615,7 +1614,7 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, /* Only send a SENDME if we're not getting optimistic data; otherwise * a SENDME could arrive before the CONNECTED. */ - connection_edge_consider_sending_sendme(conn); + sendme_connection_edge_consider_sending(conn); } return 0; @@ -1869,7 +1868,7 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, /* Don't allow the other endpoint to request more than our maximum * (i.e. initial) stream SENDME window worth of data. Well-behaved * stock clients will not request more than this max (as per the check - * in the while loop of connection_edge_consider_sending_sendme()). + * in the while loop of sendme_connection_edge_consider_sending()). */ if (conn->package_window + STREAMWINDOW_INCREMENT > STREAMWINDOW_START_MAX) { @@ -2117,42 +2116,6 @@ connection_edge_package_raw_inbuf(edge_connection_t *conn, int package_partial, goto repeat_connection_edge_package_raw_inbuf; } -/** Called when we've just received a relay data cell, when - * we've just finished flushing all bytes to stream conn, - * or when we've flushed *some* bytes to the stream conn. - * - * If conn->outbuf is not too full, and our deliver window is - * low, send back a suitable number of stream-level sendme cells. - */ -void -connection_edge_consider_sending_sendme(edge_connection_t *conn) -{ - circuit_t *circ; - - if (connection_outbuf_too_full(TO_CONN(conn))) - return; - - circ = circuit_get_by_edge_conn(conn); - if (!circ) { - /* this can legitimately happen if the destroy has already - * arrived and torn down the circuit */ - log_info(LD_APP,"No circuit associated with conn. Skipping."); - return; - } - - while (conn->deliver_window <= STREAMWINDOW_START - STREAMWINDOW_INCREMENT) { - log_debug(conn->base_.type == CONN_TYPE_AP ?LD_APP:LD_EXIT, - "Outbuf %d, Queuing stream sendme.", - (int)conn->base_.outbuf_flushlen); - conn->deliver_window += STREAMWINDOW_INCREMENT; - if (connection_edge_send_command(conn, RELAY_COMMAND_SENDME, - NULL, 0) < 0) { - log_warn(LD_APP,"connection_edge_send_command failed. Skipping."); - return; /* the circuit's closed, don't continue */ - } - } -} - /** The circuit circ has received a circuit-level sendme * (on hop layer_hint, if we're the OP). Go through all the * attached streams and let them resume reading and packaging, if @@ -2369,33 +2332,6 @@ circuit_consider_stop_edge_reading(circuit_t *circ, crypt_path_t *layer_hint) return 0; } -/** Check if the deliver_window for circuit circ (at hop - * layer_hint if it's defined) is low enough that we should - * send a circuit-level sendme back down the circuit. If so, send - * enough sendmes that the window would be overfull if we sent any - * more. - */ -static void -circuit_consider_sending_sendme(circuit_t *circ, crypt_path_t *layer_hint) -{ -// log_fn(LOG_INFO,"Considering: layer_hint is %s", -// layer_hint ? "defined" : "null"); - while ((layer_hint ? layer_hint->deliver_window : circ->deliver_window) <= - CIRCWINDOW_START - CIRCWINDOW_INCREMENT) { - log_debug(LD_CIRC,"Queuing circuit sendme."); - if (layer_hint) - layer_hint->deliver_window += CIRCWINDOW_INCREMENT; - else - circ->deliver_window += CIRCWINDOW_INCREMENT; - if (relay_send_command_from_edge(0, circ, RELAY_COMMAND_SENDME, - NULL, 0, layer_hint) < 0) { - log_warn(LD_CIRC, - "relay_send_command_from_edge failed. Circuit's closed."); - return; /* the circuit's closed, don't continue */ - } - } -} - /** The total number of cells we have allocated. */ static size_t total_cells_allocated = 0; diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c new file mode 100644 index 0000000000..3e00721d26 --- /dev/null +++ b/src/core/or/sendme.c @@ -0,0 +1,76 @@ +/* Copyright (c) 2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * \file sendme.c + * \brief Code that is related to SENDME cells both in terms of + * creating/parsing cells and handling the content. + */ + +#include "core/or/or.h" + +#include "core/mainloop/connection.h" +#include "core/or/circuitlist.h" +#include "core/or/relay.h" +#include "core/or/sendme.h" + +/** Called when we've just received a relay data cell, when + * we've just finished flushing all bytes to stream conn, + * or when we've flushed *some* bytes to the stream conn. + * + * If conn->outbuf is not too full, and our deliver window is + * low, send back a suitable number of stream-level sendme cells. + */ +void +sendme_connection_edge_consider_sending(edge_connection_t *conn) +{ + circuit_t *circ; + + if (connection_outbuf_too_full(TO_CONN(conn))) + return; + + circ = circuit_get_by_edge_conn(conn); + if (!circ) { + /* this can legitimately happen if the destroy has already + * arrived and torn down the circuit */ + log_info(LD_APP,"No circuit associated with conn. Skipping."); + return; + } + + while (conn->deliver_window <= STREAMWINDOW_START - STREAMWINDOW_INCREMENT) { + log_debug(conn->base_.type == CONN_TYPE_AP ?LD_APP:LD_EXIT, + "Outbuf %d, Queuing stream sendme.", + (int)conn->base_.outbuf_flushlen); + conn->deliver_window += STREAMWINDOW_INCREMENT; + if (connection_edge_send_command(conn, RELAY_COMMAND_SENDME, + NULL, 0) < 0) { + log_warn(LD_APP,"connection_edge_send_command failed. Skipping."); + return; /* the circuit's closed, don't continue */ + } + } +} + +/** Check if the deliver_window for circuit circ (at hop + * layer_hint if it's defined) is low enough that we should + * send a circuit-level sendme back down the circuit. If so, send + * enough sendmes that the window would be overfull if we sent any + * more. + */ +void +sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint) +{ + while ((layer_hint ? layer_hint->deliver_window : circ->deliver_window) <= + CIRCWINDOW_START - CIRCWINDOW_INCREMENT) { + log_debug(LD_CIRC,"Queuing circuit sendme."); + if (layer_hint) + layer_hint->deliver_window += CIRCWINDOW_INCREMENT; + else + circ->deliver_window += CIRCWINDOW_INCREMENT; + if (relay_send_command_from_edge(0, circ, RELAY_COMMAND_SENDME, + NULL, 0, layer_hint) < 0) { + log_warn(LD_CIRC, + "relay_send_command_from_edge failed. Circuit's closed."); + return; /* the circuit's closed, don't continue */ + } + } +} diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h new file mode 100644 index 0000000000..3ff1d98e64 --- /dev/null +++ b/src/core/or/sendme.h @@ -0,0 +1,20 @@ +/* Copyright (c) 2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * \file sendme.h + * \brief Header file for sendme.c. + **/ + +#ifndef TOR_SENDME_H +#define TOR_SENDME_H + +#include "core/or/edge_connection_st.h" +#include "core/or/crypt_path_st.h" +#include "core/or/circuit_st.h" + +void sendme_connection_edge_consider_sending(edge_connection_t *edge_conn); +void sendme_circuit_consider_sending(circuit_t *circ, + crypt_path_t *layer_hint); + +#endif /* !defined(TOR_SENDME_H) */ From ed8593b9e0838f694eeb6315db38f6fadbc5ab71 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 8 Jan 2019 12:09:01 -0500 Subject: [PATCH 02/32] sendme: Modernize and cleanup old moved code Signed-off-by: David Goulet --- src/core/or/sendme.c | 48 ++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 3e00721d26..f3acf47147 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -14,40 +14,48 @@ #include "core/or/relay.h" #include "core/or/sendme.h" -/** Called when we've just received a relay data cell, when - * we've just finished flushing all bytes to stream conn, - * or when we've flushed *some* bytes to the stream conn. +/** Called when we've just received a relay data cell, when we've just + * finished flushing all bytes to stream conn, or when we've flushed + * *some* bytes to the stream conn. * - * If conn->outbuf is not too full, and our deliver window is - * low, send back a suitable number of stream-level sendme cells. + * If conn->outbuf is not too full, and our deliver window is low, send back a + * suitable number of stream-level sendme cells. */ void sendme_connection_edge_consider_sending(edge_connection_t *conn) { - circuit_t *circ; + tor_assert(conn); - if (connection_outbuf_too_full(TO_CONN(conn))) - return; + int log_domain = TO_CONN(conn)->type == CONN_TYPE_AP ? LD_APP : LD_EXIT; - circ = circuit_get_by_edge_conn(conn); - if (!circ) { - /* this can legitimately happen if the destroy has already - * arrived and torn down the circuit */ - log_info(LD_APP,"No circuit associated with conn. Skipping."); - return; + /* Don't send it if we still have data to deliver. */ + if (connection_outbuf_too_full(TO_CONN(conn))) { + goto end; } - while (conn->deliver_window <= STREAMWINDOW_START - STREAMWINDOW_INCREMENT) { - log_debug(conn->base_.type == CONN_TYPE_AP ?LD_APP:LD_EXIT, - "Outbuf %d, Queuing stream sendme.", - (int)conn->base_.outbuf_flushlen); + if (circuit_get_by_edge_conn(conn) == NULL) { + /* This can legitimately happen if the destroy has already arrived and + * torn down the circuit. */ + log_info(log_domain, "No circuit associated with edge connection. " + "Skipping sending SENDME."); + goto end; + } + + while (conn->deliver_window <= + (STREAMWINDOW_START - STREAMWINDOW_INCREMENT)) { + log_debug(log_domain, "Outbuf %" TOR_PRIuSZ ", queuing stream SENDME.", + TO_CONN(conn)->outbuf_flushlen); conn->deliver_window += STREAMWINDOW_INCREMENT; if (connection_edge_send_command(conn, RELAY_COMMAND_SENDME, NULL, 0) < 0) { - log_warn(LD_APP,"connection_edge_send_command failed. Skipping."); - return; /* the circuit's closed, don't continue */ + log_warn(LD_BUG, "connection_edge_send_command failed while sending " + "a SENDME. Circuit probably closed, skipping."); + goto end; /* The circuit's closed, don't continue */ } } + + end: + return; } /** Check if the deliver_window for circuit circ (at hop From 9c42cc1eb22da8125ec9596920dfb9113912eac0 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 8 Jan 2019 15:03:04 -0500 Subject: [PATCH 03/32] sendme: Refactor SENDME cell processing This is a bit of a complicated commit. It moves code but also refactors part of it. No behavior change, the idea is to split things up so we can better handle and understand how SENDME cells are processed where ultimately it will be easier to handle authenticated SENDMEs (prop289) using the intermediate functions added in this commit. The entry point for the cell arriving at the edge (Client or Exit), is connection_edge_process_relay_cell() for which we look if it is a circuit or stream level SENDME. This commit refactors that part where two new functions are introduced to process each of the SENDME types. The sendme_process_circuit_level() has basically two code paths. If we are a Client (the circuit is origin) or we are an Exit. Depending on which, the package window is updated accordingly. Then finally, we resume the reading on every edge streams on the circuit. The sendme_process_stream_level() applies on the edge connection which will update the package window if needed and then will try to empty the inbuf if need be because we can now deliver more cells. Again, no behavior change but in order to split that code properly into their own functions and outside the relay.c file, code modification was needed. Part of #26840. Signed-off-by: David Goulet --- src/core/or/relay.c | 88 ++++++++++++--------------------------- src/core/or/sendme.c | 98 ++++++++++++++++++++++++++++++++++++++++++++ src/core/or/sendme.h | 5 +++ 3 files changed, 130 insertions(+), 61 deletions(-) diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 7cfacf761a..132da21466 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -1807,87 +1807,52 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, (unsigned)circ->n_circ_id, rh.stream_id); return 0; case RELAY_COMMAND_SENDME: + { + int ret; + if (!rh.stream_id) { - if (layer_hint) { - if (layer_hint->package_window + CIRCWINDOW_INCREMENT > - CIRCWINDOW_START_MAX) { - static struct ratelim_t exit_warn_ratelim = RATELIM_INIT(600); - log_fn_ratelim(&exit_warn_ratelim, LOG_WARN, LD_PROTOCOL, - "Unexpected sendme cell from exit relay. " - "Closing circ."); - return -END_CIRC_REASON_TORPROTOCOL; - } - layer_hint->package_window += CIRCWINDOW_INCREMENT; - log_debug(LD_APP,"circ-level sendme at origin, packagewindow %d.", - layer_hint->package_window); - circuit_resume_edge_reading(circ, layer_hint); - - /* We count circuit-level sendme's as valid delivered data because - * they are rate limited. - */ - if (CIRCUIT_IS_ORIGIN(circ)) { - circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ), - rh.length); - } - - } else { - if (circ->package_window + CIRCWINDOW_INCREMENT > - CIRCWINDOW_START_MAX) { - static struct ratelim_t client_warn_ratelim = RATELIM_INIT(600); - log_fn_ratelim(&client_warn_ratelim,LOG_PROTOCOL_WARN, LD_PROTOCOL, - "Unexpected sendme cell from client. " - "Closing circ (window %d).", - circ->package_window); - return -END_CIRC_REASON_TORPROTOCOL; - } - circ->package_window += CIRCWINDOW_INCREMENT; - log_debug(LD_APP, - "circ-level sendme at non-origin, packagewindow %d.", - circ->package_window); - circuit_resume_edge_reading(circ, layer_hint); + /* Circuit level SENDME cell. */ + ret = sendme_process_circuit_level(layer_hint, circ, rh.length); + if (ret < 0) { + return ret; } + /* Resume reading on any streams now that we've processed a valid + * SENDME cell that updated our package window. */ + circuit_resume_edge_reading(circ, layer_hint); + /* We are done, the rest of the code is for the stream level. */ return 0; } + + /* No connection, might be half edge state. We are done if so. */ if (!conn) { if (CIRCUIT_IS_ORIGIN(circ)) { origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); if (connection_half_edge_is_valid_sendme(ocirc->half_streams, rh.stream_id)) { circuit_read_valid_data(ocirc, rh.length); - log_info(domain, - "sendme cell on circ %u valid on half-closed " - "stream id %d", ocirc->global_identifier, rh.stream_id); + log_info(domain, "Sendme cell on circ %u valid on half-closed " + "stream id %d", + ocirc->global_identifier, rh.stream_id); } } - log_info(domain,"sendme cell dropped, unknown stream (streamid %d).", + log_info(domain, "SENDME cell dropped, unknown stream (streamid %d).", rh.stream_id); return 0; } - /* Don't allow the other endpoint to request more than our maximum - * (i.e. initial) stream SENDME window worth of data. Well-behaved - * stock clients will not request more than this max (as per the check - * in the while loop of sendme_connection_edge_consider_sending()). - */ - if (conn->package_window + STREAMWINDOW_INCREMENT > - STREAMWINDOW_START_MAX) { - static struct ratelim_t stream_warn_ratelim = RATELIM_INIT(600); - log_fn_ratelim(&stream_warn_ratelim, LOG_PROTOCOL_WARN, LD_PROTOCOL, - "Unexpected stream sendme cell. Closing circ (window %d).", - conn->package_window); - return -END_CIRC_REASON_TORPROTOCOL; + /* Stream level SENDME cell. */ + ret = sendme_process_stream_level(conn, circ, rh.length); + if (ret < 0) { + /* Means we need to close the circuit with reason ret. */ + return ret; } - /* At this point, the stream sendme is valid */ - if (CIRCUIT_IS_ORIGIN(circ)) { - circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ), - rh.length); - } + /* We've now processed properly a SENDME cell, all windows have been + * properly updated, we'll read on the edge connection to see if we can + * get data out towards the end point (Exit or client) since we are now + * allowed to deliver more cells. */ - conn->package_window += STREAMWINDOW_INCREMENT; - log_debug(domain,"stream-level sendme, packagewindow now %d.", - conn->package_window); if (circuit_queue_streams_are_blocked(circ)) { /* Still waiting for queue to flush; don't touch conn */ return 0; @@ -1900,6 +1865,7 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, return 0; } return 0; + } case RELAY_COMMAND_RESOLVE: if (layer_hint) { log_fn(LOG_PROTOCOL_WARN, LD_APP, diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index f3acf47147..df9fc57bd6 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -9,8 +9,10 @@ #include "core/or/or.h" +#include "app/config/config.h" #include "core/mainloop/connection.h" #include "core/or/circuitlist.h" +#include "core/or/circuituse.h" #include "core/or/relay.h" #include "core/or/sendme.h" @@ -82,3 +84,99 @@ sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint) } } } + +/* Process a circuit-level SENDME cell that we just received. The layer_hint, + * if not NULL, is the Exit hop of the connection which means that we are a + * client. In that case, circ must be an origin circuit. The cell_body_len is + * the length of the SENDME cell payload (excluding the header). + * + * Return 0 on success that is the SENDME is valid and the package window has + * been updated properly. + * + * On error, a negative value is returned which indicate that the circuit must + * be closed using the value as the reason for it. */ +int +sendme_process_circuit_level(crypt_path_t *layer_hint, + circuit_t *circ, uint16_t cell_body_len) +{ + tor_assert(circ); + + /* 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)) { + if ((layer_hint->package_window + CIRCWINDOW_INCREMENT) > + CIRCWINDOW_START_MAX) { + static struct ratelim_t exit_warn_ratelim = RATELIM_INIT(600); + log_fn_ratelim(&exit_warn_ratelim, LOG_WARN, LD_PROTOCOL, + "Unexpected sendme cell from exit relay. " + "Closing circ."); + return -END_CIRC_REASON_TORPROTOCOL; + } + layer_hint->package_window += CIRCWINDOW_INCREMENT; + log_debug(LD_APP, "circ-level sendme at origin, packagewindow %d.", + layer_hint->package_window); + + /* We count circuit-level sendme's as valid delivered data because they + * are rate limited. */ + circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ), cell_body_len); + } else { + /* 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) > + CIRCWINDOW_START_MAX) { + static struct ratelim_t client_warn_ratelim = RATELIM_INIT(600); + log_fn_ratelim(&client_warn_ratelim, LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Unexpected sendme cell from client. " + "Closing circ (window %d).", circ->package_window); + return -END_CIRC_REASON_TORPROTOCOL; + } + circ->package_window += CIRCWINDOW_INCREMENT; + log_debug(LD_EXIT, "circ-level sendme at non-origin, packagewindow %d.", + circ->package_window); + } + + return 0; +} + +/* Process a stream-level SENDME cell that we just received. The conn is the + * edge connection (stream) that the circuit circ is associated with. The + * cell_body_len is the length of the payload (excluding the header). + * + * Return 0 on success that is the SENDME is valid and the package window has + * been updated properly. + * + * On error, a negative value is returned which indicate that the circuit must + * be closed using the value as the reason for it. */ +int +sendme_process_stream_level(edge_connection_t *conn, circuit_t *circ, + uint16_t cell_body_len) +{ + tor_assert(conn); + tor_assert(circ); + + /* Don't allow the other endpoint to request more than our maximum (i.e. + * initial) stream SENDME window worth of data. Well-behaved stock clients + * will not request more than this max (as per the check in the while loop + * of sendme_connection_edge_consider_sending()). */ + if ((conn->package_window + STREAMWINDOW_INCREMENT) > + STREAMWINDOW_START_MAX) { + static struct ratelim_t stream_warn_ratelim = RATELIM_INIT(600); + log_fn_ratelim(&stream_warn_ratelim, LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Unexpected stream sendme cell. Closing circ (window %d).", + conn->package_window); + return -END_CIRC_REASON_TORPROTOCOL; + } + /* At this point, the stream sendme is valid */ + conn->package_window += STREAMWINDOW_INCREMENT; + + /* We count circuit-level sendme's as valid delivered data because they are + * rate limited. */ + if (CIRCUIT_IS_ORIGIN(circ)) { + circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ), cell_body_len); + } + + log_debug(CIRCUIT_IS_ORIGIN(circ) ? LD_APP : LD_EXIT, + "stream-level sendme, package_window now %d.", + conn->package_window); + return 0; +} diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index 3ff1d98e64..97748cf65e 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -17,4 +17,9 @@ void sendme_connection_edge_consider_sending(edge_connection_t *edge_conn); void sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint); +int sendme_process_circuit_level(crypt_path_t *layer_hint, + circuit_t *circ, uint16_t cell_body_len); +int sendme_process_stream_level(edge_connection_t *conn, circuit_t *circ, + uint16_t cell_body_len); + #endif /* !defined(TOR_SENDME_H) */ From 2d3c600915c22f1e9a7e9dcbba8358556ef64505 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 9 Jan 2019 10:39:58 -0500 Subject: [PATCH 04/32] sendme: Add helper functions for DATA cell delivery When we get a relay DATA cell delivered, we have to decrement the deliver window on both the circuit and stream level. This commit adds helper functions to handle the deliver window decrement. Part of #26840 Signed-off-by: David Goulet --- src/core/or/relay.c | 16 +++++++++++----- src/core/or/sendme.c | 33 +++++++++++++++++++++++++++++++++ src/core/or/sendme.h | 6 ++++++ 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 132da21466..0eb2ba3f67 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -1548,8 +1548,11 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, return connection_exit_begin_conn(cell, circ); case RELAY_COMMAND_DATA: ++stats_n_data_cells_received; - if (( layer_hint && --layer_hint->deliver_window < 0) || - (!layer_hint && --circ->deliver_window < 0)) { + + /* Update our circuit-level deliver window that we received a DATA cell. + * If the deliver window goes below 0, we end the connection due to a + * protocol failure. */ + if (sendme_circuit_data_received(circ, layer_hint) < 0) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "(relay data) circ deliver_window below 0. Killing."); if (conn) { @@ -1560,9 +1563,8 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, } return -END_CIRC_REASON_TORPROTOCOL; } - log_debug(domain,"circ deliver_window now %d.", layer_hint ? - layer_hint->deliver_window : circ->deliver_window); + /* Consider sending a circuit-level SENDME cell. */ sendme_circuit_consider_sending(circ, layer_hint); if (rh.stream_id == 0) { @@ -1586,7 +1588,11 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, return 0; } - if (--conn->deliver_window < 0) { /* is it below 0 after decrement? */ + /* Update our stream-level deliver window that we just received a DATA + * cell. Going below 0 means we have a protocol level error so the + * circuit is closed. */ + + if (sendme_stream_data_received(conn) < 0) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "(relay data) conn deliver_window below 0. Killing."); return -END_CIRC_REASON_TORPROTOCOL; diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index df9fc57bd6..6d9f6f7d05 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -180,3 +180,36 @@ sendme_process_stream_level(edge_connection_t *conn, circuit_t *circ, conn->package_window); return 0; } + +/* Called when a relay DATA cell is received on the given circuit. If + * layer_hint is NULL, this means we are the Exit end point else we are the + * Client. Update the deliver window and return its new value. */ +int +sendme_circuit_data_received(circuit_t *circ, crypt_path_t *layer_hint) +{ + int deliver_window, domain; + + if (CIRCUIT_IS_ORIGIN(circ)) { + tor_assert(layer_hint); + --layer_hint->deliver_window; + deliver_window = layer_hint->deliver_window; + domain = LD_APP; + } else { + tor_assert(!layer_hint); + --circ->deliver_window; + deliver_window = circ->deliver_window; + domain = LD_EXIT; + } + + log_debug(domain, "Circuit deliver_window now %d.", deliver_window); + return deliver_window; +} + +/* Called when a relay DATA cell is received for the given edge connection + * conn. Update the deliver window and return its new value. */ +int +sendme_stream_data_received(edge_connection_t *conn) +{ + tor_assert(conn); + return --conn->deliver_window; +} diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index 97748cf65e..6313e91216 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -13,13 +13,19 @@ #include "core/or/crypt_path_st.h" #include "core/or/circuit_st.h" +/* Sending SENDME cell. */ void sendme_connection_edge_consider_sending(edge_connection_t *edge_conn); void sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint); +/* Processing SENDME cell. */ int sendme_process_circuit_level(crypt_path_t *layer_hint, circuit_t *circ, uint16_t cell_body_len); int sendme_process_stream_level(edge_connection_t *conn, circuit_t *circ, uint16_t cell_body_len); +/* Update deliver window functions. */ +int sendme_stream_data_received(edge_connection_t *conn); +int sendme_circuit_data_received(circuit_t *circ, crypt_path_t *layer_hint); + #endif /* !defined(TOR_SENDME_H) */ From 8e38791baf48ca2a4c865f3b7fc264392e63f426 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 11 Jan 2019 11:36:08 -0500 Subject: [PATCH 05/32] sendme: Add helper functions for DATA cell packaging When we are about to send a DATA cell, we have to decrement the package window for both the circuit and stream level. This commit adds helper functions to handle the package window decrement. Part of #26288 Signed-off-by: David Goulet --- src/core/or/relay.c | 16 +++++++++------- src/core/or/sendme.c | 37 +++++++++++++++++++++++++++++++++++++ src/core/or/sendme.h | 4 ++++ 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 0eb2ba3f67..06e201f20d 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -2062,15 +2062,17 @@ connection_edge_package_raw_inbuf(edge_connection_t *conn, int package_partial, return 0; } - if (!cpath_layer) { /* non-rendezvous exit */ - tor_assert(circ->package_window > 0); - circ->package_window--; - } else { /* we're an AP, or an exit on a rendezvous circ */ - tor_assert(cpath_layer->package_window > 0); - cpath_layer->package_window--; + /* Handle the circuit-level SENDME package window. */ + if (sendme_circuit_data_packaged(circ, cpath_layer) < 0) { + /* Package window has gone under 0. Protocol issue. */ + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Circuit package window is below 0. Closing circuit."); + conn->end_reason = END_STREAM_REASON_TORPROTOCOL; + return -1; } - if (--conn->package_window <= 0) { /* is it 0 after decrement? */ + /* Handle the stream-level SENDME package window. */ + if (sendme_stream_data_packaged(conn) < 0) { connection_stop_reading(TO_CONN(conn)); log_debug(domain,"conn->package_window reached 0."); circuit_consider_stop_edge_reading(circ, cpath_layer); diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 6d9f6f7d05..d7feb6bfc6 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -213,3 +213,40 @@ sendme_stream_data_received(edge_connection_t *conn) tor_assert(conn); return --conn->deliver_window; } + +/* Called when a relay DATA cell is packaged on the given circuit. If + * layer_hint is NULL, this means we are the Exit end point else we are the + * Client. Update the package window and return its new value. */ +int +sendme_circuit_data_packaged(circuit_t *circ, crypt_path_t *layer_hint) +{ + int package_window, domain; + + tor_assert(circ); + + if (CIRCUIT_IS_ORIGIN(circ)) { + /* Client side. */ + tor_assert(layer_hint); + --layer_hint->package_window; + package_window = layer_hint->package_window; + domain = LD_APP; + } else { + /* Exit side. */ + tor_assert(!layer_hint); + --circ->package_window; + package_window = circ->package_window; + domain = LD_EXIT; + } + + log_debug(domain, "Circuit package_window now %d.", package_window); + return package_window; +} + +/* Called when a relay DATA cell is packaged for the given edge connection + * conn. Update the package window and return its new value. */ +int +sendme_stream_data_packaged(edge_connection_t *conn) +{ + tor_assert(conn); + return --conn->package_window; +} diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index 6313e91216..f50ccfbfe2 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -28,4 +28,8 @@ int sendme_process_stream_level(edge_connection_t *conn, circuit_t *circ, int sendme_stream_data_received(edge_connection_t *conn); int sendme_circuit_data_received(circuit_t *circ, crypt_path_t *layer_hint); +/* Update package window functions. */ +int sendme_circuit_data_packaged(circuit_t *circ, crypt_path_t *layer_hint); +int sendme_stream_data_packaged(edge_connection_t *conn); + #endif /* !defined(TOR_SENDME_H) */ From 0e6e800c89c7cfef255491179473e13de5f72d03 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 9 Jan 2019 11:03:49 -0500 Subject: [PATCH 06/32] sendme: Always close stream if deliver window is negative Previously, we would only close the stream when our deliver window was negative at the circuit-level but _not_ at the stream-level when receiving a DATA cell. This commit adds an helper function connection_edge_end_close() which sends an END and then mark the stream for close for a given reason. That function is now used both in case the deliver window goes below zero for both circuit and stream level. Part of #26840 Signed-off-by: David Goulet --- src/core/or/connection_edge.c | 19 +++++++++++++++++++ src/core/or/connection_edge.h | 1 + src/core/or/relay.c | 14 +++++--------- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/core/or/connection_edge.c b/src/core/or/connection_edge.c index fa72fb7716..8ed4034560 100644 --- a/src/core/or/connection_edge.c +++ b/src/core/or/connection_edge.c @@ -4565,6 +4565,25 @@ circuit_clear_isolation(origin_circuit_t *circ) circ->socks_username_len = circ->socks_password_len = 0; } +/** Send an END and mark for close the given edge connection conn using the + * given reason that has to be a stream reason. + * + * Note: We don't unattached the AP connection (if applicable) because we + * don't want to flush the remaining data. This function aims at ending + * everything quickly regardless of the connection state. + * + * This function can't fail and does nothing if conn is NULL. */ +void +connection_edge_end_close(edge_connection_t *conn, uint8_t reason) +{ + if (!conn) { + return; + } + + connection_edge_end(conn, reason); + connection_mark_for_close(TO_CONN(conn)); +} + /** Free all storage held in module-scoped variables for connection_edge.c */ void connection_edge_free_all(void) diff --git a/src/core/or/connection_edge.h b/src/core/or/connection_edge.h index 68d8b19a11..e82b6bd765 100644 --- a/src/core/or/connection_edge.h +++ b/src/core/or/connection_edge.h @@ -80,6 +80,7 @@ int connection_edge_process_inbuf(edge_connection_t *conn, int connection_edge_destroy(circid_t circ_id, edge_connection_t *conn); int connection_edge_end(edge_connection_t *conn, uint8_t reason); int connection_edge_end_errno(edge_connection_t *conn); +void connection_edge_end_close(edge_connection_t *conn, uint8_t reason); int connection_edge_flushed_some(edge_connection_t *conn); int connection_edge_finished_flushing(edge_connection_t *conn); int connection_edge_finished_connecting(edge_connection_t *conn); diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 06e201f20d..6ff053d8a0 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -1550,17 +1550,12 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, ++stats_n_data_cells_received; /* Update our circuit-level deliver window that we received a DATA cell. - * If the deliver window goes below 0, we end the connection due to a - * protocol failure. */ + * If the deliver window goes below 0, we end the circuit and stream due + * to a protocol failure. */ if (sendme_circuit_data_received(circ, layer_hint) < 0) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "(relay data) circ deliver_window below 0. Killing."); - if (conn) { - /* XXXX Do we actually need to do this? Will killing the circuit - * not send an END and mark the stream for close as appropriate? */ - connection_edge_end(conn, END_STREAM_REASON_TORPROTOCOL); - connection_mark_for_close(TO_CONN(conn)); - } + connection_edge_end_close(conn, END_STREAM_REASON_TORPROTOCOL); return -END_CIRC_REASON_TORPROTOCOL; } @@ -1590,11 +1585,12 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, /* Update our stream-level deliver window that we just received a DATA * cell. Going below 0 means we have a protocol level error so the - * circuit is closed. */ + * stream and circuit are closed. */ if (sendme_stream_data_received(conn) < 0) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "(relay data) conn deliver_window below 0. Killing."); + connection_edge_end_close(conn, END_STREAM_REASON_TORPROTOCOL); return -END_CIRC_REASON_TORPROTOCOL; } /* Total all valid application bytes delivered */ From c38d46bf4adf4107d39a2ce46aeacb630f3f112a Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 9 Jan 2019 12:02:01 -0500 Subject: [PATCH 07/32] prop289: Add two consensus parameters In order to be able to deploy the authenticated SENDMEs, these two consensus parameters are needed to control the minimum version that we can emit and accept. See section 4 in prop289 for more details. Note that at this commit, the functions that return the values aren't used so compilation fails if warnings are set to errors. Closes #26842 Signed-off-by: David Goulet --- src/core/or/sendme.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index d7feb6bfc6..bba760ae98 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -15,6 +15,39 @@ #include "core/or/circuituse.h" #include "core/or/relay.h" #include "core/or/sendme.h" +#include "feature/nodelist/networkstatus.h" + +/* 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 +get_emit_min_version(void) +{ + return networkstatus_get_param(NULL, "sendme_emit_min_version", + SENDME_EMIT_MIN_VERSION_DEFAULT, + SENDME_EMIT_MIN_VERSION_MIN, + SENDME_EMIT_MIN_VERSION_MAX); +} + +/* Return the minimum version given by the consensus (if any) that should be + * accepted when receiving a SENDME cell. */ +static int +get_accept_min_version(void) +{ + return networkstatus_get_param(NULL, "sendme_accept_min_version", + SENDME_ACCEPT_MIN_VERSION_DEFAULT, + SENDME_ACCEPT_MIN_VERSION_MIN, + SENDME_ACCEPT_MIN_VERSION_MAX); +} /** Called when we've just received a relay data cell, when we've just * finished flushing all bytes to stream conn, or when we've flushed From eef78ac0b07096a6925ae42e8b5d526304fa54a8 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 8 Jan 2019 11:13:37 -0500 Subject: [PATCH 08/32] prop289: Add SENDME trunnel declaration Signed-off-by: David Goulet --- src/trunnel/include.am | 3 + src/trunnel/sendme.c | 492 +++++++++++++++++++++++++++++++++++++ src/trunnel/sendme.h | 170 +++++++++++++ src/trunnel/sendme.trunnel | 18 ++ 4 files changed, 683 insertions(+) create mode 100644 src/trunnel/sendme.c create mode 100644 src/trunnel/sendme.h create mode 100644 src/trunnel/sendme.trunnel diff --git a/src/trunnel/include.am b/src/trunnel/include.am index 4f4f1d3624..82e7a66959 100644 --- a/src/trunnel/include.am +++ b/src/trunnel/include.am @@ -11,6 +11,7 @@ TRUNNELINPUTS = \ src/trunnel/link_handshake.trunnel \ src/trunnel/pwbox.trunnel \ src/trunnel/channelpadding_negotiation.trunnel \ + src/trunnel/sendme.trunnel \ src/trunnel/socks5.trunnel \ src/trunnel/circpad_negotiation.trunnel @@ -24,6 +25,7 @@ TRUNNELSOURCES = \ src/trunnel/hs/cell_introduce1.c \ src/trunnel/hs/cell_rendezvous.c \ src/trunnel/channelpadding_negotiation.c \ + src/trunnel/sendme.c \ src/trunnel/socks5.c \ src/trunnel/netinfo.c \ src/trunnel/circpad_negotiation.c @@ -40,6 +42,7 @@ TRUNNELHEADERS = \ src/trunnel/hs/cell_introduce1.h \ src/trunnel/hs/cell_rendezvous.h \ src/trunnel/channelpadding_negotiation.h \ + src/trunnel/sendme.h \ src/trunnel/socks5.h \ src/trunnel/netinfo.h \ src/trunnel/circpad_negotiation.h diff --git a/src/trunnel/sendme.c b/src/trunnel/sendme.c new file mode 100644 index 0000000000..08f9ed5e91 --- /dev/null +++ b/src/trunnel/sendme.c @@ -0,0 +1,492 @@ +/* sendme.c -- generated by Trunnel v1.5.2. + * https://gitweb.torproject.org/trunnel.git + * You probably shouldn't edit this file. + */ +#include +#include "trunnel-impl.h" + +#include "sendme.h" + +#define TRUNNEL_SET_ERROR_CODE(obj) \ + do { \ + (obj)->trunnel_error_code_ = 1; \ + } while (0) + +#if defined(__COVERITY__) || defined(__clang_analyzer__) +/* If we're running a static analysis tool, we don't want it to complain + * that some of our remaining-bytes checks are dead-code. */ +int sendme_deadcode_dummy__ = 0; +#define OR_DEADCODE_DUMMY || sendme_deadcode_dummy__ +#else +#define OR_DEADCODE_DUMMY +#endif + +#define CHECK_REMAINING(nbytes, label) \ + do { \ + if (remaining < (nbytes) OR_DEADCODE_DUMMY) { \ + goto label; \ + } \ + } while (0) + +sendme_cell_t * +sendme_cell_new(void) +{ + sendme_cell_t *val = trunnel_calloc(1, sizeof(sendme_cell_t)); + if (NULL == val) + return NULL; + return val; +} + +/** Release all storage held inside 'obj', but do not free 'obj'. + */ +static void +sendme_cell_clear(sendme_cell_t *obj) +{ + (void) obj; + TRUNNEL_DYNARRAY_WIPE(&obj->data); + TRUNNEL_DYNARRAY_CLEAR(&obj->data); +} + +void +sendme_cell_free(sendme_cell_t *obj) +{ + if (obj == NULL) + return; + sendme_cell_clear(obj); + trunnel_memwipe(obj, sizeof(sendme_cell_t)); + trunnel_free_(obj); +} + +uint8_t +sendme_cell_get_version(const sendme_cell_t *inp) +{ + return inp->version; +} +int +sendme_cell_set_version(sendme_cell_t *inp, uint8_t val) +{ + if (! ((val == 0 || val == 1))) { + TRUNNEL_SET_ERROR_CODE(inp); + return -1; + } + inp->version = val; + return 0; +} +uint16_t +sendme_cell_get_data_len(const sendme_cell_t *inp) +{ + return inp->data_len; +} +int +sendme_cell_set_data_len(sendme_cell_t *inp, uint16_t val) +{ + inp->data_len = val; + return 0; +} +size_t +sendme_cell_getlen_data(const sendme_cell_t *inp) +{ + return TRUNNEL_DYNARRAY_LEN(&inp->data); +} + +uint8_t +sendme_cell_get_data(sendme_cell_t *inp, size_t idx) +{ + return TRUNNEL_DYNARRAY_GET(&inp->data, idx); +} + +uint8_t +sendme_cell_getconst_data(const sendme_cell_t *inp, size_t idx) +{ + return sendme_cell_get_data((sendme_cell_t*)inp, idx); +} +int +sendme_cell_set_data(sendme_cell_t *inp, size_t idx, uint8_t elt) +{ + TRUNNEL_DYNARRAY_SET(&inp->data, idx, elt); + return 0; +} +int +sendme_cell_add_data(sendme_cell_t *inp, uint8_t elt) +{ +#if SIZE_MAX >= UINT16_MAX + if (inp->data.n_ == UINT16_MAX) + goto trunnel_alloc_failed; +#endif + TRUNNEL_DYNARRAY_ADD(uint8_t, &inp->data, elt, {}); + return 0; + trunnel_alloc_failed: + TRUNNEL_SET_ERROR_CODE(inp); + return -1; +} + +uint8_t * +sendme_cell_getarray_data(sendme_cell_t *inp) +{ + return inp->data.elts_; +} +const uint8_t * +sendme_cell_getconstarray_data(const sendme_cell_t *inp) +{ + return (const uint8_t *)sendme_cell_getarray_data((sendme_cell_t*)inp); +} +int +sendme_cell_setlen_data(sendme_cell_t *inp, size_t newlen) +{ + uint8_t *newptr; +#if UINT16_MAX < SIZE_MAX + if (newlen > UINT16_MAX) + goto trunnel_alloc_failed; +#endif + newptr = trunnel_dynarray_setlen(&inp->data.allocated_, + &inp->data.n_, inp->data.elts_, newlen, + sizeof(inp->data.elts_[0]), (trunnel_free_fn_t) NULL, + &inp->trunnel_error_code_); + if (newlen != 0 && newptr == NULL) + goto trunnel_alloc_failed; + inp->data.elts_ = newptr; + return 0; + trunnel_alloc_failed: + TRUNNEL_SET_ERROR_CODE(inp); + return -1; +} +const char * +sendme_cell_check(const sendme_cell_t *obj) +{ + if (obj == NULL) + return "Object was NULL"; + if (obj->trunnel_error_code_) + return "A set function failed on this object"; + if (! (obj->version == 0 || obj->version == 1)) + return "Integer out of bounds"; + if (TRUNNEL_DYNARRAY_LEN(&obj->data) != obj->data_len) + return "Length mismatch for data"; + return NULL; +} + +ssize_t +sendme_cell_encoded_len(const sendme_cell_t *obj) +{ + ssize_t result = 0; + + if (NULL != sendme_cell_check(obj)) + return -1; + + + /* Length of u8 version IN [0, 1] */ + result += 1; + + /* Length of u16 data_len */ + result += 2; + + /* Length of u8 data[data_len] */ + result += TRUNNEL_DYNARRAY_LEN(&obj->data); + return result; +} +int +sendme_cell_clear_errors(sendme_cell_t *obj) +{ + int r = obj->trunnel_error_code_; + obj->trunnel_error_code_ = 0; + return r; +} +ssize_t +sendme_cell_encode(uint8_t *output, const size_t avail, const sendme_cell_t *obj) +{ + ssize_t result = 0; + size_t written = 0; + uint8_t *ptr = output; + const char *msg; +#ifdef TRUNNEL_CHECK_ENCODED_LEN + const ssize_t encoded_len = sendme_cell_encoded_len(obj); +#endif + + if (NULL != (msg = sendme_cell_check(obj))) + goto check_failed; + +#ifdef TRUNNEL_CHECK_ENCODED_LEN + trunnel_assert(encoded_len >= 0); +#endif + + /* Encode u8 version IN [0, 1] */ + trunnel_assert(written <= avail); + if (avail - written < 1) + goto truncated; + trunnel_set_uint8(ptr, (obj->version)); + written += 1; ptr += 1; + + /* Encode u16 data_len */ + trunnel_assert(written <= avail); + if (avail - written < 2) + goto truncated; + trunnel_set_uint16(ptr, trunnel_htons(obj->data_len)); + written += 2; ptr += 2; + + /* Encode u8 data[data_len] */ + { + size_t elt_len = TRUNNEL_DYNARRAY_LEN(&obj->data); + trunnel_assert(obj->data_len == elt_len); + trunnel_assert(written <= avail); + if (avail - written < elt_len) + goto truncated; + if (elt_len) + memcpy(ptr, obj->data.elts_, elt_len); + written += elt_len; ptr += elt_len; + } + + + trunnel_assert(ptr == output + written); +#ifdef TRUNNEL_CHECK_ENCODED_LEN + { + trunnel_assert(encoded_len >= 0); + trunnel_assert((size_t)encoded_len == written); + } + +#endif + + return written; + + truncated: + result = -2; + goto fail; + check_failed: + (void)msg; + result = -1; + goto fail; + fail: + trunnel_assert(result < 0); + return result; +} + +/** As sendme_cell_parse(), but do not allocate the output object. + */ +static ssize_t +sendme_cell_parse_into(sendme_cell_t *obj, const uint8_t *input, const size_t len_in) +{ + const uint8_t *ptr = input; + size_t remaining = len_in; + ssize_t result = 0; + (void)result; + + /* Parse u8 version IN [0, 1] */ + CHECK_REMAINING(1, truncated); + obj->version = (trunnel_get_uint8(ptr)); + remaining -= 1; ptr += 1; + if (! (obj->version == 0 || obj->version == 1)) + goto fail; + + /* Parse u16 data_len */ + CHECK_REMAINING(2, truncated); + obj->data_len = trunnel_ntohs(trunnel_get_uint16(ptr)); + remaining -= 2; ptr += 2; + + /* Parse u8 data[data_len] */ + CHECK_REMAINING(obj->data_len, truncated); + TRUNNEL_DYNARRAY_EXPAND(uint8_t, &obj->data, obj->data_len, {}); + obj->data.n_ = obj->data_len; + if (obj->data_len) + memcpy(obj->data.elts_, ptr, obj->data_len); + ptr += obj->data_len; remaining -= obj->data_len; + trunnel_assert(ptr + remaining == input + len_in); + return len_in - remaining; + + truncated: + return -2; + trunnel_alloc_failed: + return -1; + fail: + result = -1; + return result; +} + +ssize_t +sendme_cell_parse(sendme_cell_t **output, const uint8_t *input, const size_t len_in) +{ + ssize_t result; + *output = sendme_cell_new(); + if (NULL == *output) + return -1; + result = sendme_cell_parse_into(*output, input, len_in); + if (result < 0) { + sendme_cell_free(*output); + *output = NULL; + } + return result; +} +sendme_data_v1_t * +sendme_data_v1_new(void) +{ + sendme_data_v1_t *val = trunnel_calloc(1, sizeof(sendme_data_v1_t)); + if (NULL == val) + return NULL; + return val; +} + +/** Release all storage held inside 'obj', but do not free 'obj'. + */ +static void +sendme_data_v1_clear(sendme_data_v1_t *obj) +{ + (void) obj; +} + +void +sendme_data_v1_free(sendme_data_v1_t *obj) +{ + if (obj == NULL) + return; + sendme_data_v1_clear(obj); + trunnel_memwipe(obj, sizeof(sendme_data_v1_t)); + trunnel_free_(obj); +} + +size_t +sendme_data_v1_getlen_digest(const sendme_data_v1_t *inp) +{ + (void)inp; return 4; +} + +uint8_t +sendme_data_v1_get_digest(sendme_data_v1_t *inp, size_t idx) +{ + trunnel_assert(idx < 4); + return inp->digest[idx]; +} + +uint8_t +sendme_data_v1_getconst_digest(const sendme_data_v1_t *inp, size_t idx) +{ + return sendme_data_v1_get_digest((sendme_data_v1_t*)inp, idx); +} +int +sendme_data_v1_set_digest(sendme_data_v1_t *inp, size_t idx, uint8_t elt) +{ + trunnel_assert(idx < 4); + inp->digest[idx] = elt; + return 0; +} + +uint8_t * +sendme_data_v1_getarray_digest(sendme_data_v1_t *inp) +{ + return inp->digest; +} +const uint8_t * +sendme_data_v1_getconstarray_digest(const sendme_data_v1_t *inp) +{ + return (const uint8_t *)sendme_data_v1_getarray_digest((sendme_data_v1_t*)inp); +} +const char * +sendme_data_v1_check(const sendme_data_v1_t *obj) +{ + if (obj == NULL) + return "Object was NULL"; + if (obj->trunnel_error_code_) + return "A set function failed on this object"; + return NULL; +} + +ssize_t +sendme_data_v1_encoded_len(const sendme_data_v1_t *obj) +{ + ssize_t result = 0; + + if (NULL != sendme_data_v1_check(obj)) + return -1; + + + /* Length of u8 digest[4] */ + result += 4; + return result; +} +int +sendme_data_v1_clear_errors(sendme_data_v1_t *obj) +{ + int r = obj->trunnel_error_code_; + obj->trunnel_error_code_ = 0; + return r; +} +ssize_t +sendme_data_v1_encode(uint8_t *output, const size_t avail, const sendme_data_v1_t *obj) +{ + ssize_t result = 0; + size_t written = 0; + uint8_t *ptr = output; + const char *msg; +#ifdef TRUNNEL_CHECK_ENCODED_LEN + const ssize_t encoded_len = sendme_data_v1_encoded_len(obj); +#endif + + if (NULL != (msg = sendme_data_v1_check(obj))) + goto check_failed; + +#ifdef TRUNNEL_CHECK_ENCODED_LEN + trunnel_assert(encoded_len >= 0); +#endif + + /* Encode u8 digest[4] */ + trunnel_assert(written <= avail); + if (avail - written < 4) + goto truncated; + memcpy(ptr, obj->digest, 4); + written += 4; ptr += 4; + + + trunnel_assert(ptr == output + written); +#ifdef TRUNNEL_CHECK_ENCODED_LEN + { + trunnel_assert(encoded_len >= 0); + trunnel_assert((size_t)encoded_len == written); + } + +#endif + + return written; + + truncated: + result = -2; + goto fail; + check_failed: + (void)msg; + result = -1; + goto fail; + fail: + trunnel_assert(result < 0); + return result; +} + +/** As sendme_data_v1_parse(), but do not allocate the output object. + */ +static ssize_t +sendme_data_v1_parse_into(sendme_data_v1_t *obj, const uint8_t *input, const size_t len_in) +{ + const uint8_t *ptr = input; + size_t remaining = len_in; + ssize_t result = 0; + (void)result; + + /* Parse u8 digest[4] */ + CHECK_REMAINING(4, truncated); + memcpy(obj->digest, ptr, 4); + remaining -= 4; ptr += 4; + trunnel_assert(ptr + remaining == input + len_in); + return len_in - remaining; + + truncated: + return -2; +} + +ssize_t +sendme_data_v1_parse(sendme_data_v1_t **output, const uint8_t *input, const size_t len_in) +{ + ssize_t result; + *output = sendme_data_v1_new(); + if (NULL == *output) + return -1; + result = sendme_data_v1_parse_into(*output, input, len_in); + if (result < 0) { + sendme_data_v1_free(*output); + *output = NULL; + } + return result; +} diff --git a/src/trunnel/sendme.h b/src/trunnel/sendme.h new file mode 100644 index 0000000000..8207cb56f7 --- /dev/null +++ b/src/trunnel/sendme.h @@ -0,0 +1,170 @@ +/* sendme.h -- generated by Trunnel v1.5.2. + * https://gitweb.torproject.org/trunnel.git + * You probably shouldn't edit this file. + */ +#ifndef TRUNNEL_SENDME_H +#define TRUNNEL_SENDME_H + +#include +#include "trunnel.h" + +#if !defined(TRUNNEL_OPAQUE) && !defined(TRUNNEL_OPAQUE_SENDME_CELL) +struct sendme_cell_st { + uint8_t version; + uint16_t data_len; + TRUNNEL_DYNARRAY_HEAD(, uint8_t) data; + uint8_t trunnel_error_code_; +}; +#endif +typedef struct sendme_cell_st sendme_cell_t; +#if !defined(TRUNNEL_OPAQUE) && !defined(TRUNNEL_OPAQUE_SENDME_DATA_V1) +struct sendme_data_v1_st { + uint8_t digest[4]; + uint8_t trunnel_error_code_; +}; +#endif +typedef struct sendme_data_v1_st sendme_data_v1_t; +/** Return a newly allocated sendme_cell with all elements set to + * zero. + */ +sendme_cell_t *sendme_cell_new(void); +/** Release all storage held by the sendme_cell in 'victim'. (Do + * nothing if 'victim' is NULL.) + */ +void sendme_cell_free(sendme_cell_t *victim); +/** Try to parse a sendme_cell from the buffer in 'input', using up to + * 'len_in' bytes from the input buffer. On success, return the number + * of bytes consumed and set *output to the newly allocated + * sendme_cell_t. On failure, return -2 if the input appears + * truncated, and -1 if the input is otherwise invalid. + */ +ssize_t sendme_cell_parse(sendme_cell_t **output, const uint8_t *input, const size_t len_in); +/** Return the number of bytes we expect to need to encode the + * sendme_cell in 'obj'. On failure, return a negative value. Note + * that this value may be an overestimate, and can even be an + * underestimate for certain unencodeable objects. + */ +ssize_t sendme_cell_encoded_len(const sendme_cell_t *obj); +/** Try to encode the sendme_cell from 'input' into the buffer at + * 'output', using up to 'avail' bytes of the output buffer. On + * success, return the number of bytes used. On failure, return -2 if + * the buffer was not long enough, and -1 if the input was invalid. + */ +ssize_t sendme_cell_encode(uint8_t *output, size_t avail, const sendme_cell_t *input); +/** Check whether the internal state of the sendme_cell in 'obj' is + * consistent. Return NULL if it is, and a short message if it is not. + */ +const char *sendme_cell_check(const sendme_cell_t *obj); +/** Clear any errors that were set on the object 'obj' by its setter + * functions. Return true iff errors were cleared. + */ +int sendme_cell_clear_errors(sendme_cell_t *obj); +/** Return the value of the version field of the sendme_cell_t in + * 'inp' + */ +uint8_t sendme_cell_get_version(const sendme_cell_t *inp); +/** Set the value of the version field of the sendme_cell_t in 'inp' + * to 'val'. Return 0 on success; return -1 and set the error code on + * 'inp' on failure. + */ +int sendme_cell_set_version(sendme_cell_t *inp, uint8_t val); +/** Return the value of the data_len field of the sendme_cell_t in + * 'inp' + */ +uint16_t sendme_cell_get_data_len(const sendme_cell_t *inp); +/** Set the value of the data_len field of the sendme_cell_t in 'inp' + * to 'val'. Return 0 on success; return -1 and set the error code on + * 'inp' on failure. + */ +int sendme_cell_set_data_len(sendme_cell_t *inp, uint16_t val); +/** Return the length of the dynamic array holding the data field of + * the sendme_cell_t in 'inp'. + */ +size_t sendme_cell_getlen_data(const sendme_cell_t *inp); +/** Return the element at position 'idx' of the dynamic array field + * data of the sendme_cell_t in 'inp'. + */ +uint8_t sendme_cell_get_data(sendme_cell_t *inp, size_t idx); +/** As sendme_cell_get_data, but take and return a const pointer + */ +uint8_t sendme_cell_getconst_data(const sendme_cell_t *inp, size_t idx); +/** Change the element at position 'idx' of the dynamic array field + * data of the sendme_cell_t in 'inp', so that it will hold the value + * 'elt'. + */ +int sendme_cell_set_data(sendme_cell_t *inp, size_t idx, uint8_t elt); +/** Append a new element 'elt' to the dynamic array field data of the + * sendme_cell_t in 'inp'. + */ +int sendme_cell_add_data(sendme_cell_t *inp, uint8_t elt); +/** Return a pointer to the variable-length array field data of 'inp'. + */ +uint8_t * sendme_cell_getarray_data(sendme_cell_t *inp); +/** As sendme_cell_get_data, but take and return a const pointer + */ +const uint8_t * sendme_cell_getconstarray_data(const sendme_cell_t *inp); +/** Change the length of the variable-length array field data of 'inp' + * to 'newlen'.Fill extra elements with 0. Return 0 on success; return + * -1 and set the error code on 'inp' on failure. + */ +int sendme_cell_setlen_data(sendme_cell_t *inp, size_t newlen); +/** Return a newly allocated sendme_data_v1 with all elements set to + * zero. + */ +sendme_data_v1_t *sendme_data_v1_new(void); +/** Release all storage held by the sendme_data_v1 in 'victim'. (Do + * nothing if 'victim' is NULL.) + */ +void sendme_data_v1_free(sendme_data_v1_t *victim); +/** Try to parse a sendme_data_v1 from the buffer in 'input', using up + * to 'len_in' bytes from the input buffer. On success, return the + * number of bytes consumed and set *output to the newly allocated + * sendme_data_v1_t. On failure, return -2 if the input appears + * truncated, and -1 if the input is otherwise invalid. + */ +ssize_t sendme_data_v1_parse(sendme_data_v1_t **output, const uint8_t *input, const size_t len_in); +/** Return the number of bytes we expect to need to encode the + * sendme_data_v1 in 'obj'. On failure, return a negative value. Note + * that this value may be an overestimate, and can even be an + * underestimate for certain unencodeable objects. + */ +ssize_t sendme_data_v1_encoded_len(const sendme_data_v1_t *obj); +/** Try to encode the sendme_data_v1 from 'input' into the buffer at + * 'output', using up to 'avail' bytes of the output buffer. On + * success, return the number of bytes used. On failure, return -2 if + * the buffer was not long enough, and -1 if the input was invalid. + */ +ssize_t sendme_data_v1_encode(uint8_t *output, size_t avail, const sendme_data_v1_t *input); +/** Check whether the internal state of the sendme_data_v1 in 'obj' is + * consistent. Return NULL if it is, and a short message if it is not. + */ +const char *sendme_data_v1_check(const sendme_data_v1_t *obj); +/** Clear any errors that were set on the object 'obj' by its setter + * functions. Return true iff errors were cleared. + */ +int sendme_data_v1_clear_errors(sendme_data_v1_t *obj); +/** Return the (constant) length of the array holding the digest field + * of the sendme_data_v1_t in 'inp'. + */ +size_t sendme_data_v1_getlen_digest(const sendme_data_v1_t *inp); +/** Return the element at position 'idx' of the fixed array field + * digest of the sendme_data_v1_t in 'inp'. + */ +uint8_t sendme_data_v1_get_digest(sendme_data_v1_t *inp, size_t idx); +/** As sendme_data_v1_get_digest, but take and return a const pointer + */ +uint8_t sendme_data_v1_getconst_digest(const sendme_data_v1_t *inp, size_t idx); +/** Change the element at position 'idx' of the fixed array field + * digest of the sendme_data_v1_t in 'inp', so that it will hold the + * value 'elt'. + */ +int sendme_data_v1_set_digest(sendme_data_v1_t *inp, size_t idx, uint8_t elt); +/** Return a pointer to the 4-element array field digest of 'inp'. + */ +uint8_t * sendme_data_v1_getarray_digest(sendme_data_v1_t *inp); +/** As sendme_data_v1_get_digest, but take and return a const pointer + */ +const uint8_t * sendme_data_v1_getconstarray_digest(const sendme_data_v1_t *inp); + + +#endif diff --git a/src/trunnel/sendme.trunnel b/src/trunnel/sendme.trunnel new file mode 100644 index 0000000000..7294a09b47 --- /dev/null +++ b/src/trunnel/sendme.trunnel @@ -0,0 +1,18 @@ +/* This file contains the SENDME cell definition. */ + +struct sendme_cell { + /* Version field. */ + u8 version IN [0x00, 0x01]; + + /* The data content depends on the version. */ + u16 data_len; + u8 data[data_len]; +} + +/* SENDME version 0. No data. */ + +/* SENDME version 1. Authenticated with digest. */ +struct sendme_data_v1 { + /* A 4 bytes digest. */ + u8 digest[4]; +} From 023a70da841182fbbbe11389929c8fbbc7490365 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 9 Jan 2019 12:22:35 -0500 Subject: [PATCH 09/32] prop289: Support sending SENDME version 1 This code will obey the consensus parameter "sendme_emit_min_version" to know which SENDME version it should send. For now, the default is 0 and the parameter is not yet used in the consensus. This commit adds the support to send version 1 SENDMEs but aren't sent on the wire at this commit. Closes #26840 Signed-off-by: David Goulet --- src/core/or/relay.c | 2 +- src/core/or/sendme.c | 106 ++++++++++++++++++++++++++++++++++++++++--- src/core/or/sendme.h | 3 +- 3 files changed, 103 insertions(+), 8 deletions(-) diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 6ff053d8a0..6f69ed999b 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -1560,7 +1560,7 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, } /* Consider sending a circuit-level SENDME cell. */ - sendme_circuit_consider_sending(circ, layer_hint); + sendme_circuit_consider_sending(circ, layer_hint, NULL); if (rh.stream_id == 0) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "Relay data cell with zero " diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index bba760ae98..f22e7027db 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -16,6 +16,7 @@ #include "core/or/relay.h" #include "core/or/sendme.h" #include "feature/nodelist/networkstatus.h" +#include "trunnel/sendme.h" /* The cell version constants for when emitting a cell. */ #define SENDME_EMIT_MIN_VERSION_DEFAULT 0 @@ -38,6 +39,7 @@ get_emit_min_version(void) SENDME_EMIT_MIN_VERSION_MAX); } +#if 0 /* Return the minimum version given by the consensus (if any) that should be * accepted when receiving a SENDME cell. */ static int @@ -48,6 +50,98 @@ get_accept_min_version(void) SENDME_ACCEPT_MIN_VERSION_MIN, SENDME_ACCEPT_MIN_VERSION_MAX); } +#endif + +/* Build and encode a version 1 SENDME cell into payload, which must be at + * least of RELAY_PAYLOAD_SIZE bytes, using the digest for the cell data. + * + * Return the size in bytes of the encoded cell in payload. A negative value + * is returned on encoding failure. */ +static ssize_t +build_cell_payload_v1(crypto_digest_t *cell_digest, uint8_t *payload) +{ + ssize_t len = -1; + sendme_cell_t *cell = NULL; + sendme_data_v1_t *data = NULL; + + tor_assert(cell_digest); + tor_assert(payload); + + cell = sendme_cell_new(); + data = sendme_data_v1_new(); + + /* Building a payload for version 1. */ + sendme_cell_set_version(cell, 0x01); + + /* Copy the digest into the data payload. */ + crypto_digest_get_digest(cell_digest, + (char *) sendme_data_v1_getarray_digest(data), + sendme_data_v1_getlen_digest(data)); + + /* Set the length of the data in the cell payload. It is the encoded length + * of the v1 data object. */ + sendme_cell_setlen_data(cell, sendme_data_v1_encoded_len(data)); + /* Encode into the cell's data field using its current length just set. */ + if (sendme_data_v1_encode(sendme_cell_getarray_data(cell), + sendme_cell_getlen_data(cell), data) < 0) { + goto end; + } + /* Set the DATA_LEN field to what we've just encoded. */ + sendme_cell_set_data_len(cell, sendme_cell_getlen_data(cell)); + + /* Finally, encode the cell into the payload. */ + len = sendme_cell_encode(payload, RELAY_PAYLOAD_SIZE, cell); + + end: + sendme_cell_free(cell); + sendme_data_v1_free(data); + return len; +} + +/* Send a circuit-level SENDME on the given circuit using the layer_hint if + * not NULL. The digest is only used for version 1. + * + * Return 0 on success else a negative value and the circuit will be closed + * because we failed to send the cell on it. */ +static int +send_circuit_level_sendme(circuit_t *circ, crypt_path_t *layer_hint, + crypto_digest_t *cell_digest) +{ + uint8_t emit_version; + uint8_t payload[RELAY_PAYLOAD_SIZE]; + ssize_t payload_len; + + tor_assert(circ); + tor_assert(cell_digest); + + emit_version = get_emit_min_version(); + switch (emit_version) { + case 0x01: + payload_len = build_cell_payload_v1(cell_digest, payload); + if (BUG(payload_len < 0)) { + /* Unable to encode the cell, abort. We can recover from this by closing + * the circuit but in theory it should never happen. */ + return -1; + } + log_debug(LD_PROTOCOL, "Emitting SENDME version 1 cell."); + break; + case 0x00: + /* Fallthrough because default is to use v0. */ + default: + /* Unknown version, fallback to version 0 meaning no payload. */ + payload_len = 0; + break; + } + + if (relay_send_command_from_edge(0, circ, RELAY_COMMAND_SENDME, + (char *) payload, payload_len, + layer_hint) < 0) { + log_warn(LD_CIRC, + "SENDME relay_send_command_from_edge failed. Circuit's closed."); + return -1; /* the circuit's closed, don't continue */ + } + return 0; +} /** Called when we've just received a relay data cell, when we've just * finished flushing all bytes to stream conn, or when we've flushed @@ -100,8 +194,11 @@ sendme_connection_edge_consider_sending(edge_connection_t *conn) * more. */ void -sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint) +sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint, + crypto_digest_t *digest) { + tor_assert(digest); + while ((layer_hint ? layer_hint->deliver_window : circ->deliver_window) <= CIRCWINDOW_START - CIRCWINDOW_INCREMENT) { log_debug(LD_CIRC,"Queuing circuit sendme."); @@ -109,11 +206,8 @@ sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint) layer_hint->deliver_window += CIRCWINDOW_INCREMENT; else circ->deliver_window += CIRCWINDOW_INCREMENT; - if (relay_send_command_from_edge(0, circ, RELAY_COMMAND_SENDME, - NULL, 0, layer_hint) < 0) { - log_warn(LD_CIRC, - "relay_send_command_from_edge failed. Circuit's closed."); - return; /* the circuit's closed, don't continue */ + if (send_circuit_level_sendme(circ, layer_hint, digest) < 0) { + return; /* The circuit's closed, don't continue */ } } } diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index f50ccfbfe2..ba01ecfaf8 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -16,7 +16,8 @@ /* Sending SENDME cell. */ void sendme_connection_edge_consider_sending(edge_connection_t *edge_conn); void sendme_circuit_consider_sending(circuit_t *circ, - crypt_path_t *layer_hint); + crypt_path_t *layer_hint, + crypto_digest_t *digest); /* Processing SENDME cell. */ int sendme_process_circuit_level(crypt_path_t *layer_hint, From 81706d84279f0a2870f8b1789403188fd933b32a Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 9 Jan 2019 14:03:32 -0500 Subject: [PATCH 10/32] prop289: Support SENDME v1 cell parsing This commit makes tor able to parse and handle a SENDME version 1. It will look at the consensus parameter "sendme_accept_min_version" to know what is the minimum version it should look at. IMPORTANT: At this commit, the validation of the cell is not fully implemented. For this, we need #26839 to be completed that is to match the SENDME digest with the last cell digest. Closes #26841 Signed-off-by: David Goulet --- src/core/or/relay.c | 4 +- src/core/or/sendme.c | 140 +++++++++++++++++++++++++++++++++++++++++-- src/core/or/sendme.h | 3 +- 3 files changed, 140 insertions(+), 7 deletions(-) diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 6f69ed999b..76f2203a9a 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -1814,7 +1814,9 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, if (!rh.stream_id) { /* Circuit level SENDME cell. */ - ret = sendme_process_circuit_level(layer_hint, circ, rh.length); + ret = sendme_process_circuit_level(layer_hint, circ, + cell->payload + RELAY_HEADER_SIZE, + rh.length); if (ret < 0) { return ret; } diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index f22e7027db..64497055e1 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -18,6 +18,10 @@ #include "feature/nodelist/networkstatus.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 @@ -39,7 +43,6 @@ get_emit_min_version(void) SENDME_EMIT_MIN_VERSION_MAX); } -#if 0 /* Return the minimum version given by the consensus (if any) that should be * accepted when receiving a SENDME cell. */ static int @@ -50,7 +53,124 @@ get_accept_min_version(void) SENDME_ACCEPT_MIN_VERSION_MIN, SENDME_ACCEPT_MIN_VERSION_MAX); } -#endif + +/* Return true iff the given decoded SENDME version 1 cell is valid. + * + * Validation is done by comparing the digest in the cell from the previous + * 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) +{ + sendme_data_v1_t *data = NULL; + + tor_assert(cell); + + if (sendme_data_v1_parse(&data, sendme_cell_getconstarray_data(cell), + sendme_cell_getlen_data(cell)) < 0) { + goto invalid; + } + + /* XXX: Match the digest in the cell to the previous cell. Needs to be + * implemented that is passed to this function and compared. For this, we + * need #26839 that is making tor remember the last digest(s). */ + + /* Validated SENDME v1 cell. */ + sendme_data_v1_free(data); + return 1; + invalid: + sendme_data_v1_free(data); + return 0; +} + +/* 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) +{ + int accept_version = get_accept_min_version(); + + /* Can we handle this version? */ + if (accept_version > SENDME_MAX_SUPPORTED_VERSION) { + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Unable to handle SENDME version %u. We only support <= %d " + "(from consensus). Probably your tor is too old?", + accept_version, cell_version); + goto invalid; + } + + /* We only accept a SENDME cell from what the consensus tells us. */ + if (cell_version < accept_version) { + log_info(LD_PROTOCOL, "Unacceptable SENDME version %d. Only " + "accepting %u (taken from the consensus). " + "Closing circuit.", + cell_version, accept_version); + goto invalid; + } + + return 1; + invalid: + return 0; +} + +/* Return true iff the encoded SENDME cell in cell_payload of length + * cell_payload_len is valid. For each version: + * + * 0: No validation + * 1: Authenticated with last cell digest. + * + * 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 mark for close. */ +static bool +sendme_is_valid(const uint8_t *cell_payload, size_t cell_payload_len) +{ + uint8_t cell_version; + sendme_cell_t *cell = NULL; + + tor_assert(cell_payload); + + /* An empty payload means version 0 so skip trunnel parsing. We won't be + * able to parse a 0 length buffer into a valid SENDME cell. */ + if (cell_payload_len == 0) { + cell_version = 0; + } else { + /* First we'll decode the cell so we can get the version. */ + if (sendme_cell_parse(&cell, cell_payload, cell_payload_len) < 0) { + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Unparseable SENDME cell received. Closing circuit."); + goto invalid; + } + cell_version = sendme_cell_get_version(cell); + } + + /* Validate that we can handle this cell version. */ + if (!cell_version_is_valid(cell_version)) { + goto invalid; + } + + /* Validate depending on the version now. */ + switch (cell_version) { + case 0x01: + if (!cell_v1_is_valid(cell)) { + 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. */ + default: + /* Unknown version means we can't handle it so fallback to version 0. */ + break; + } + + /* Valid cell. */ + sendme_cell_free(cell); + return 1; + invalid: + sendme_cell_free(cell); + return 0; +} /* Build and encode a version 1 SENDME cell into payload, which must be at * least of RELAY_PAYLOAD_SIZE bytes, using the digest for the cell data. @@ -215,7 +335,8 @@ sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint, /* Process a circuit-level SENDME cell that we just received. The layer_hint, * if not NULL, is the Exit hop of the connection which means that we are a * client. In that case, circ must be an origin circuit. The cell_body_len is - * the length of the SENDME cell payload (excluding the header). + * the length of the SENDME cell payload (excluding the header). The + * cell_payload is the payload. * * Return 0 on success that is the SENDME is valid and the package window has * been updated properly. @@ -224,9 +345,11 @@ sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint, * be closed using the value as the reason for it. */ int sendme_process_circuit_level(crypt_path_t *layer_hint, - circuit_t *circ, uint16_t cell_body_len) + circuit_t *circ, const uint8_t *cell_payload, + uint16_t cell_payload_len) { tor_assert(circ); + tor_assert(cell_payload); /* 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. */ @@ -245,8 +368,15 @@ sendme_process_circuit_level(crypt_path_t *layer_hint, /* We count circuit-level sendme's as valid delivered data because they * are rate limited. */ - circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ), cell_body_len); + 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(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) > diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index ba01ecfaf8..033bc6ff75 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -21,7 +21,8 @@ void sendme_circuit_consider_sending(circuit_t *circ, /* Processing SENDME cell. */ int sendme_process_circuit_level(crypt_path_t *layer_hint, - circuit_t *circ, uint16_t cell_body_len); + circuit_t *circ, const uint8_t *cell_payload, + uint16_t cell_payload_len); int sendme_process_stream_level(edge_connection_t *conn, circuit_t *circ, uint16_t cell_body_len); From 93f9fbbd34f03aca68c8c64a7e39b64548462eeb Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 9 Jan 2019 15:08:14 -0500 Subject: [PATCH 11/32] prop289: Keep track of the last seen cell digests This makes tor remember the last seen digest of a cell if that cell is the last one before a SENDME on the Exit side. Closes #26839 Signed-off-by: David Goulet --- src/core/or/circuit_st.h | 6 ++++++ src/core/or/circuitlist.c | 6 ++++++ src/core/or/relay.c | 8 ++++++++ src/core/or/sendme.c | 29 +++++++++++++++++++++++++++++ src/core/or/sendme.h | 3 +++ 5 files changed, 52 insertions(+) diff --git a/src/core/or/circuit_st.h b/src/core/or/circuit_st.h index cc21cf62f7..5adb158935 100644 --- a/src/core/or/circuit_st.h +++ b/src/core/or/circuit_st.h @@ -104,6 +104,12 @@ struct circuit_t { * circuit-level sendme cells to indicate that we're willing to accept * more. */ int deliver_window; + /** FIFO containing the digest of the cells that are just before a SENDME is + * sent by the client. It is done at the last cell before our package_window + * goes down to 0 which is when we expect a SENDME. The protocol doesn't + * allow more than 10 outstanding SENDMEs worth of data meaning this list + * should only contain at most 10 digests of 4 bytes each. */ + smartlist_t *sendme_last_digests; /** Temporary field used during circuits_handle_oom. */ uint32_t age_tmp; diff --git a/src/core/or/circuitlist.c b/src/core/or/circuitlist.c index afbde06434..6428cdb8a7 100644 --- a/src/core/or/circuitlist.c +++ b/src/core/or/circuitlist.c @@ -1227,6 +1227,12 @@ circuit_free_(circuit_t *circ) * "active" checks will be violated. */ cell_queue_clear(&circ->n_chan_cells); + /* Cleanup possible SENDME state. */ + if (circ->sendme_last_digests) { + SMARTLIST_FOREACH(circ->sendme_last_digests, uint8_t *, d, tor_free(d)); + smartlist_free(circ->sendme_last_digests); + } + log_info(LD_CIRC, "Circuit %u (id: %" PRIu32 ") has been freed.", n_circ_id, CIRCUIT_IS_ORIGIN(circ) ? diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 76f2203a9a..b26360b245 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -639,6 +639,14 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *circ, circuit_mark_for_close(circ, END_CIRC_REASON_INTERNAL); return -1; } + + /* If applicable, note the cell digest for the SENDME version 1 purpose if + * 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_note_cell_digest(circ); + } + return 0; } diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 64497055e1..69bcac4680 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -11,6 +11,7 @@ #include "app/config/config.h" #include "core/mainloop/connection.h" +#include "core/or/cell_st.h" #include "core/or/circuitlist.h" #include "core/or/circuituse.h" #include "core/or/relay.h" @@ -507,3 +508,31 @@ sendme_stream_data_packaged(edge_connection_t *conn) tor_assert(conn); 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. */ +void +sendme_note_cell_digest(circuit_t *circ) +{ + uint8_t *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; + } + /* 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 (((circ->package_window - 1) % CIRCWINDOW_INCREMENT) != 0) { + return; + } + + digest = tor_malloc_zero(4); + if (circ->sendme_last_digests == NULL) { + circ->sendme_last_digests = smartlist_new(); + } + smartlist_add(circ->sendme_last_digests, digest); +} diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index 033bc6ff75..300fb25d96 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -34,4 +34,7 @@ int sendme_circuit_data_received(circuit_t *circ, crypt_path_t *layer_hint); int sendme_circuit_data_packaged(circuit_t *circ, crypt_path_t *layer_hint); int sendme_stream_data_packaged(edge_connection_t *conn); +/* Track cell digest. */ +void sendme_note_cell_digest(circuit_t *circ); + #endif /* !defined(TOR_SENDME_H) */ From bb473a807ae94a1e6c45a069db6ddf213413940a Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 9 Jan 2019 15:27:51 -0500 Subject: [PATCH 12/32] prop289: Match the SENDME digest Now that we keep the last seen cell digests on the Exit side on the circuit object, use that to match the SENDME v1 transforming this whole process into a real authenticated SENDME mechanism. Part of #26841 Signed-off-by: David Goulet --- src/core/or/sendme.c | 41 ++++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 69bcac4680..afade43f74 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -17,6 +17,7 @@ #include "core/or/relay.h" #include "core/or/sendme.h" #include "feature/nodelist/networkstatus.h" +#include "lib/ctime/di_ops.h" #include "trunnel/sendme.h" /* The maximum supported version. Above that value, the cell can't be @@ -61,7 +62,7 @@ get_accept_min_version(void) * 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) +cell_v1_is_valid(const sendme_cell_t *cell, const circuit_t *circ) { sendme_data_v1_t *data = NULL; @@ -72,9 +73,33 @@ cell_v1_is_valid(const sendme_cell_t *cell) goto invalid; } - /* XXX: Match the digest in the cell to the previous cell. Needs to be - * implemented that is passed to this function and compared. For this, we - * need #26839 that is making tor remember the last digest(s). */ + /* We shouldn't have received this 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 invalid; + } + + /* Pop the first element that was added (FIFO) and compare it. */ + { + uint8_t *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_memcmp(digest, sendme_data_v1_getconstarray_digest(data), + sendme_data_v1_getlen_digest(data))) { + tor_free(digest); + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "SENDME v1 cell digest do not match."); + goto invalid; + } + tor_free(digest); + } /* Validated SENDME v1 cell. */ sendme_data_v1_free(data); @@ -124,11 +149,13 @@ cell_version_is_valid(uint8_t cell_version) * send/recv cells on a circuit. If the SENDME is invalid, the circuit should * be mark for close. */ static bool -sendme_is_valid(const uint8_t *cell_payload, size_t cell_payload_len) +sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, + size_t cell_payload_len) { uint8_t cell_version; sendme_cell_t *cell = NULL; + tor_assert(circ); tor_assert(cell_payload); /* An empty payload means version 0 so skip trunnel parsing. We won't be @@ -153,7 +180,7 @@ sendme_is_valid(const uint8_t *cell_payload, size_t cell_payload_len) /* Validate depending on the version now. */ switch (cell_version) { case 0x01: - if (!cell_v1_is_valid(cell)) { + if (!cell_v1_is_valid(cell, circ)) { goto invalid; } break; @@ -374,7 +401,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. It is only done if we are the Exit of the circuit. */ - if (!sendme_is_valid(cell_payload, cell_payload_len)) { + if (!sendme_is_valid(circ, cell_payload, cell_payload_len)) { return -END_CIRC_REASON_TORPROTOCOL; } From 402f0a4f5d70bee128130f4dbd0ea18de1747410 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 23 Jan 2019 14:39:04 -0500 Subject: [PATCH 13/32] prop289: Remember the last cell digest for v1 SENDMEs In order to do so, depending on where the cell is going, we'll keep the last cell digest that is either received inbound or sent outbound. Then it can be used for validation. Part of #26288 Signed-off-by: David Goulet --- src/core/crypto/relay_crypto.c | 13 +++++++++++++ src/core/or/relay.c | 2 +- src/core/or/relay_crypto_st.h | 2 ++ src/core/or/sendme.c | 28 +++++++++++++++++++--------- src/core/or/sendme.h | 3 +-- 5 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/core/crypto/relay_crypto.c b/src/core/crypto/relay_crypto.c index 0b83b2d0a5..d4116d47ab 100644 --- a/src/core/crypto/relay_crypto.c +++ b/src/core/crypto/relay_crypto.c @@ -142,6 +142,13 @@ relay_decrypt_cell(circuit_t *circ, cell_t *cell, if (relay_digest_matches(thishop->crypto.b_digest, cell)) { *recognized = 1; *layer_hint = thishop; + /* Keep current digest of this cell for the possible SENDME. */ + if (thishop->crypto.sendme_digest) { + crypto_digest_free(thishop->crypto.sendme_digest); + } + thishop->crypto.sendme_digest = + crypto_digest_dup(thishop->crypto.b_digest); + return 0; } } @@ -212,6 +219,11 @@ relay_encrypt_cell_inbound(cell_t *cell, or_circuit_t *or_circ) { relay_set_digest(or_circ->crypto.b_digest, cell); + /* Keep a record of this cell, we might use it for validating the SENDME. */ + if (or_circ->crypto.sendme_digest) { + crypto_digest_free(or_circ->crypto.sendme_digest); + } + or_circ->crypto.sendme_digest = crypto_digest_dup(or_circ->crypto.b_digest); /* encrypt one layer */ relay_crypt_one_payload(or_circ->crypto.b_crypto, cell->payload); } @@ -229,6 +241,7 @@ relay_crypto_clear(relay_crypto_t *crypto) crypto_cipher_free(crypto->b_crypto); crypto_digest_free(crypto->f_digest); crypto_digest_free(crypto->b_digest); + crypto_digest_free(crypto->sendme_digest); } /** Initialize crypto from the key material in key_data. diff --git a/src/core/or/relay.c b/src/core/or/relay.c index b26360b245..47275a811e 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -1568,7 +1568,7 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, } /* Consider sending a circuit-level SENDME cell. */ - sendme_circuit_consider_sending(circ, layer_hint, NULL); + sendme_circuit_consider_sending(circ, layer_hint); if (rh.stream_id == 0) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "Relay data cell with zero " diff --git a/src/core/or/relay_crypto_st.h b/src/core/or/relay_crypto_st.h index dafce257c7..dbdf1599dc 100644 --- a/src/core/or/relay_crypto_st.h +++ b/src/core/or/relay_crypto_st.h @@ -25,6 +25,8 @@ struct relay_crypto_t { /** Digest state for cells heading away from the OR at this step. */ struct crypto_digest_t *b_digest; + /** Digest used for the next SENDME cell if any. */ + struct crypto_digest_t *sendme_digest; }; #undef crypto_cipher_t diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index afade43f74..76f551a929 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -14,6 +14,7 @@ #include "core/or/cell_st.h" #include "core/or/circuitlist.h" #include "core/or/circuituse.h" +#include "core/or/or_circuit_st.h" #include "core/or/relay.h" #include "core/or/sendme.h" #include "feature/nodelist/networkstatus.h" @@ -342,18 +343,20 @@ sendme_connection_edge_consider_sending(edge_connection_t *conn) * more. */ void -sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint, - crypto_digest_t *digest) +sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint) { - tor_assert(digest); + crypto_digest_t *digest; while ((layer_hint ? layer_hint->deliver_window : circ->deliver_window) <= CIRCWINDOW_START - CIRCWINDOW_INCREMENT) { log_debug(LD_CIRC,"Queuing circuit sendme."); - if (layer_hint) + if (layer_hint) { layer_hint->deliver_window += CIRCWINDOW_INCREMENT; - else + digest = layer_hint->crypto.sendme_digest; + } else { circ->deliver_window += CIRCWINDOW_INCREMENT; + digest = TO_OR_CIRCUIT(circ)->crypto.sendme_digest; + } if (send_circuit_level_sendme(circ, layer_hint, digest) < 0) { return; /* The circuit's closed, don't continue */ } @@ -557,9 +560,16 @@ sendme_note_cell_digest(circuit_t *circ) return; } - digest = tor_malloc_zero(4); - if (circ->sendme_last_digests == NULL) { - circ->sendme_last_digests = smartlist_new(); + /* Only note the digest if we actually have the digest of the previous cell + * recorded. It should never happen in theory as we always record the last + * digest for the v1 SENDME. */ + if (TO_OR_CIRCUIT(circ)->crypto.sendme_digest) { + digest = tor_malloc_zero(4); + crypto_digest_get_digest(TO_OR_CIRCUIT(circ)->crypto.sendme_digest, + (char *) digest, 4); + if (circ->sendme_last_digests == NULL) { + circ->sendme_last_digests = smartlist_new(); + } + smartlist_add(circ->sendme_last_digests, digest); } - smartlist_add(circ->sendme_last_digests, digest); } diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index 300fb25d96..e7cf718bb1 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -16,8 +16,7 @@ /* Sending SENDME cell. */ void sendme_connection_edge_consider_sending(edge_connection_t *edge_conn); void sendme_circuit_consider_sending(circuit_t *circ, - crypt_path_t *layer_hint, - crypto_digest_t *digest); + crypt_path_t *layer_hint); /* Processing SENDME cell. */ int sendme_process_circuit_level(crypt_path_t *layer_hint, From a6e012508e5b0d676cdf204fcbd7942e3cc21419 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 19 Feb 2019 15:02:11 -0500 Subject: [PATCH 14/32] prop289: Add random bytes to the unused portion of the cell Signed-off-by: David Goulet --- src/core/or/relay.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 47275a811e..63c406d8af 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -572,6 +572,14 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *circ, if (payload_len) memcpy(cell.payload+RELAY_HEADER_SIZE, payload, payload_len); + /* Add random bytes to the unused portion of the payload, to foil attacks + * where the other side can predict all of the bytes in the payload and thus + * compute authenticated sendme cells without seeing the traffic. See + * proposal 289. */ + crypto_fast_rng_getbytes(get_thread_fast_rng(), + cell.payload + RELAY_HEADER_SIZE + payload_len, + RELAY_PAYLOAD_SIZE - payload_len); + log_debug(LD_OR,"delivering %d cell %s.", relay_command, cell_direction == CELL_DIRECTION_OUT ? "forward" : "backward"); From cede93b2d83fb810ec8b2152882732ed0a7481dc Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 19 Feb 2019 14:49:38 -0500 Subject: [PATCH 15/32] tests: Implement unit tests for SENDME v1 Part of #26288 Signed-off-by: David Goulet --- src/core/or/sendme.c | 12 +- src/core/or/sendme.h | 23 ++++ src/test/include.am | 1 + src/test/test.c | 1 + src/test/test.h | 1 + src/test/test_relaycell.c | 1 + src/test/test_sendme.c | 223 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 257 insertions(+), 5 deletions(-) create mode 100644 src/test/test_sendme.c diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 76f551a929..980684c827 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -7,6 +7,8 @@ * creating/parsing cells and handling the content. */ +#define SENDME_PRIVATE + #include "core/or/or.h" #include "app/config/config.h" @@ -37,7 +39,7 @@ /* Return the minimum version given by the consensus (if any) that should be * used when emitting a SENDME cell. */ -static int +STATIC int get_emit_min_version(void) { return networkstatus_get_param(NULL, "sendme_emit_min_version", @@ -48,7 +50,7 @@ get_emit_min_version(void) /* Return the minimum version given by the consensus (if any) that should be * accepted when receiving a SENDME cell. */ -static int +STATIC int get_accept_min_version(void) { return networkstatus_get_param(NULL, "sendme_accept_min_version", @@ -112,7 +114,7 @@ cell_v1_is_valid(const sendme_cell_t *cell, const circuit_t *circ) /* 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 +STATIC bool cell_version_is_valid(uint8_t cell_version) { int accept_version = get_accept_min_version(); @@ -149,7 +151,7 @@ cell_version_is_valid(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 mark for close. */ -static bool +STATIC bool sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, size_t cell_payload_len) { @@ -206,7 +208,7 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, * * Return the size in bytes of the encoded cell in payload. A negative value * is returned on encoding failure. */ -static ssize_t +STATIC ssize_t build_cell_payload_v1(crypto_digest_t *cell_digest, uint8_t *payload) { ssize_t len = -1; diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index e7cf718bb1..c2e2518da8 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -36,4 +36,27 @@ int sendme_stream_data_packaged(edge_connection_t *conn); /* Track cell digest. */ void sendme_note_cell_digest(circuit_t *circ); +/* Private section starts. */ +#ifdef SENDME_PRIVATE + +/* + * Unit tests declaractions. + */ +#ifdef TOR_UNIT_TESTS + +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 ssize_t build_cell_payload_v1(crypto_digest_t *cell_digest, + uint8_t *payload); +STATIC bool sendme_is_valid(const circuit_t *circ, + const uint8_t *cell_payload, + size_t cell_payload_len); + +#endif /* TOR_UNIT_TESTS */ + +#endif /* SENDME_PRIVATE */ + #endif /* !defined(TOR_SENDME_H) */ diff --git a/src/test/include.am b/src/test/include.am index 497aa320a4..5f6b05fa95 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -179,6 +179,7 @@ src_test_test_SOURCES += \ src/test/test_routerlist.c \ src/test/test_routerset.c \ src/test/test_scheduler.c \ + src/test/test_sendme.c \ src/test/test_shared_random.c \ src/test/test_socks.c \ src/test/test_status.c \ diff --git a/src/test/test.c b/src/test/test.c index fbc30fb64e..17159b71c5 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -923,6 +923,7 @@ struct testgroup_t testgroups[] = { { "routerlist/", routerlist_tests }, { "routerset/" , routerset_tests }, { "scheduler/", scheduler_tests }, + { "sendme/", sendme_tests }, { "shared-random/", sr_tests }, { "socks/", socks_tests }, { "status/" , status_tests }, diff --git a/src/test/test.h b/src/test/test.h index 7d19af9b20..167fd090ac 100644 --- a/src/test/test.h +++ b/src/test/test.h @@ -266,6 +266,7 @@ extern struct testcase_t routerkeys_tests[]; extern struct testcase_t routerlist_tests[]; extern struct testcase_t routerset_tests[]; extern struct testcase_t scheduler_tests[]; +extern struct testcase_t sendme_tests[]; extern struct testcase_t socks_tests[]; extern struct testcase_t sr_tests[]; extern struct testcase_t status_tests[]; diff --git a/src/test/test_relaycell.c b/src/test/test_relaycell.c index 0623583511..c4ed215c7c 100644 --- a/src/test/test_relaycell.c +++ b/src/test/test_relaycell.c @@ -705,6 +705,7 @@ test_circbw_relay(void *arg) circ = helper_create_origin_circuit(CIRCUIT_PURPOSE_C_GENERAL, 0); circ->cpath->state = CPATH_STATE_AWAITING_KEYS; circ->cpath->deliver_window = CIRCWINDOW_START; + circ->cpath->crypto.sendme_digest = crypto_digest_new(); entryconn1 = fake_entry_conn(circ, 1); edgeconn = ENTRY_TO_EDGE_CONN(entryconn1); diff --git a/src/test/test_sendme.c b/src/test/test_sendme.c new file mode 100644 index 0000000000..ad6aac6c0c --- /dev/null +++ b/src/test/test_sendme.c @@ -0,0 +1,223 @@ +/* Copyright (c) 2014-2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/* Unit tests for handling different kinds of relay cell */ + +#define CIRCUITLIST_PRIVATE +#define NETWORKSTATUS_PRIVATE +#define SENDME_PRIVATE + +#include "core/or/circuit_st.h" +#include "core/or/or_circuit_st.h" +#include "core/or/origin_circuit_st.h" +#include "core/or/circuitlist.h" +#include "core/or/sendme.h" + +#include "feature/nodelist/networkstatus.h" +#include "feature/nodelist/networkstatus_st.h" + +#include "test/test.h" +#include "test/log_test_helpers.h" + +static void +setup_mock_consensus(void) +{ + current_md_consensus = current_ns_consensus = + tor_malloc_zero(sizeof(networkstatus_t)); + current_md_consensus->net_params = smartlist_new(); + current_md_consensus->routerstatus_list = smartlist_new(); +} + +static void +free_mock_consensus(void) +{ + SMARTLIST_FOREACH(current_md_consensus->routerstatus_list, void *, r, + tor_free(r)); + smartlist_free(current_md_consensus->routerstatus_list); + smartlist_free(current_ns_consensus->net_params); + tor_free(current_ns_consensus); +} + +static void +test_v1_note_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); + 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_note_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); + or_circ->crypto.sendme_digest = crypto_digest_new(); + + /* The package window has to be a multiple of CIRCWINDOW_INCREMENT minus 1 + * in order to catched the CIRCWINDOW_INCREMENT-nth cell. Try something that + * shouldn't be noted. */ + circ->package_window = CIRCWINDOW_INCREMENT; + sendme_note_cell_digest(circ); + tt_assert(!circ->sendme_last_digests); + + /* This should work now. Package window at CIRCWINDOW_INCREMENT + 1. */ + circ->package_window++; + sendme_note_cell_digest(circ); + 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_note_cell_digest(circ); + 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_note_cell_digest(circ); + tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 2); + + done: + circuit_free_(circ); +} + +static void +test_v1_consensus_params(void *arg) +{ + (void) arg; + + setup_mock_consensus(); + tt_assert(current_md_consensus); + + /* Both zeroes. */ + smartlist_add(current_md_consensus->net_params, + (void *) "sendme_emit_min_version=0"); + smartlist_add(current_md_consensus->net_params, + (void *) "sendme_accept_min_version=0"); + tt_int_op(get_emit_min_version(), OP_EQ, 0); + tt_int_op(get_accept_min_version(), OP_EQ, 0); + smartlist_clear(current_md_consensus->net_params); + + /* Both ones. */ + smartlist_add(current_md_consensus->net_params, + (void *) "sendme_emit_min_version=1"); + smartlist_add(current_md_consensus->net_params, + (void *) "sendme_accept_min_version=1"); + tt_int_op(get_emit_min_version(), OP_EQ, 1); + tt_int_op(get_accept_min_version(), OP_EQ, 1); + smartlist_clear(current_md_consensus->net_params); + + /* Different values from each other. */ + smartlist_add(current_md_consensus->net_params, + (void *) "sendme_emit_min_version=1"); + smartlist_add(current_md_consensus->net_params, + (void *) "sendme_accept_min_version=0"); + tt_int_op(get_emit_min_version(), OP_EQ, 1); + tt_int_op(get_accept_min_version(), OP_EQ, 0); + smartlist_clear(current_md_consensus->net_params); + + /* Validate is the cell version is coherent with our internal default value + * and the one in the consensus. */ + 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); + /* Minimum acceptable value is 1 so a cell version of 0 is refused. */ + tt_int_op(cell_version_is_valid(0), OP_EQ, false); + + done: + free_mock_consensus(); +} + +static void +test_v1_build_cell(void *arg) +{ + uint8_t payload[RELAY_PAYLOAD_SIZE]; + ssize_t ret; + crypto_digest_t *cell_digest = NULL; + or_circuit_t *or_circ = NULL; + circuit_t *circ = NULL; + + (void) arg; + + or_circ = or_circuit_new(1, NULL); + circ = TO_CIRCUIT(or_circ); + + cell_digest = crypto_digest_new(); + crypto_digest_add_bytes(cell_digest, "AAAA", 4); + tt_assert(cell_digest); + + /* SENDME v1 payload is 7 bytes. See spec. */ + ret = build_cell_payload_v1(cell_digest, payload); + tt_int_op(ret, OP_EQ, 7); + + /* Validation. */ + + /* An empty payload means SENDME version 0 thus valid. */ + tt_int_op(sendme_is_valid(circ, payload, 0), OP_EQ, true); + + /* 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); + 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); + expect_log_msg_containing("We received a SENDME but we have no cell digests " + "to match. Closing circuit."); + teardown_capture_of_logs(); + + /* Note the wrong digest in the circuit, cell should fail validation. */ + or_circ->crypto.sendme_digest = crypto_digest_new(); + circ->package_window = CIRCWINDOW_INCREMENT + 1; + sendme_note_cell_digest(circ); + 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); + /* 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."); + teardown_capture_of_logs(); + + /* Cleanup */ + crypto_digest_free(or_circ->crypto.sendme_digest); + + /* Record the cell digest into the circuit, cell should validate. */ + or_circ->crypto.sendme_digest = crypto_digest_dup(cell_digest); + circ->package_window = CIRCWINDOW_INCREMENT + 1; + sendme_note_cell_digest(circ); + 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. */ + tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 0); + + done: + crypto_digest_free(cell_digest); + circuit_free_(circ); +} + +struct testcase_t sendme_tests[] = { + { "v1_note_digest", test_v1_note_digest, TT_FORK, + NULL, NULL }, + { "v1_consensus_params", test_v1_consensus_params, TT_FORK, + NULL, NULL }, + { "v1_build_cell", test_v1_build_cell, TT_FORK, + NULL, NULL }, + + END_OF_TESTCASES +}; From 504e05b02999afb6a58ebe4af5770ca8dc136233 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 7 Mar 2019 11:20:23 -0500 Subject: [PATCH 16/32] prop289: Use a 20 bytes digest instead of 4 To achieve such, this commit also changes the trunnel declaration to use a union instead of a seperate object for the v1 data. A constant is added for the digest length so we can use it within the SENDME code giving us a single reference. Part of #26288 Signed-off-by: David Goulet --- src/core/or/sendme.c | 41 ++--- src/test/test_sendme.c | 6 +- src/trunnel/sendme.c | 347 +++++++++++-------------------------- src/trunnel/sendme.h | 107 ++---------- src/trunnel/sendme.trunnel | 19 +- 5 files changed, 145 insertions(+), 375 deletions(-) diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 980684c827..a333b02b60 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -59,7 +59,8 @@ get_accept_min_version(void) SENDME_ACCEPT_MIN_VERSION_MAX); } -/* Return true iff the given decoded SENDME version 1 cell is valid. +/* Return true iff the given decoded SENDME version 1 cell is valid and + * matches the expected digest on the circuit. * * Validation is done by comparing the digest in the cell from the previous * cell we saw which tells us that the other side has in fact seen that cell. @@ -67,14 +68,12 @@ get_accept_min_version(void) static bool cell_v1_is_valid(const sendme_cell_t *cell, const circuit_t *circ) { - sendme_data_v1_t *data = NULL; + const uint8_t *cell_digest = NULL; tor_assert(cell); + tor_assert(circ); - if (sendme_data_v1_parse(&data, sendme_cell_getconstarray_data(cell), - sendme_cell_getlen_data(cell)) < 0) { - goto invalid; - } + cell_digest = sendme_cell_getconstarray_data_v1_digest(cell); /* We shouldn't have received this SENDME if we have no digests. Log at * protocol warning because it can be tricked by sending many SENDMEs @@ -94,8 +93,7 @@ cell_v1_is_valid(const sendme_cell_t *cell, const circuit_t *circ) /* Compare the digest with the one in the SENDME. This cell is invalid * without a perfect match. */ - if (tor_memcmp(digest, sendme_data_v1_getconstarray_digest(data), - sendme_data_v1_getlen_digest(data))) { + if (tor_memcmp(digest, cell_digest, TRUNNEL_SENDME_V1_DIGEST_LEN)) { tor_free(digest); log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "SENDME v1 cell digest do not match."); @@ -105,10 +103,8 @@ cell_v1_is_valid(const sendme_cell_t *cell, const circuit_t *circ) } /* Validated SENDME v1 cell. */ - sendme_data_v1_free(data); return 1; invalid: - sendme_data_v1_free(data); return 0; } @@ -213,39 +209,26 @@ build_cell_payload_v1(crypto_digest_t *cell_digest, uint8_t *payload) { ssize_t len = -1; sendme_cell_t *cell = NULL; - sendme_data_v1_t *data = NULL; tor_assert(cell_digest); tor_assert(payload); cell = sendme_cell_new(); - data = sendme_data_v1_new(); /* Building a payload for version 1. */ sendme_cell_set_version(cell, 0x01); + /* Set the data length field for v1. */ + sendme_cell_set_data_len(cell, TRUNNEL_SENDME_V1_DIGEST_LEN); /* Copy the digest into the data payload. */ crypto_digest_get_digest(cell_digest, - (char *) sendme_data_v1_getarray_digest(data), - sendme_data_v1_getlen_digest(data)); - - /* Set the length of the data in the cell payload. It is the encoded length - * of the v1 data object. */ - sendme_cell_setlen_data(cell, sendme_data_v1_encoded_len(data)); - /* Encode into the cell's data field using its current length just set. */ - if (sendme_data_v1_encode(sendme_cell_getarray_data(cell), - sendme_cell_getlen_data(cell), data) < 0) { - goto end; - } - /* Set the DATA_LEN field to what we've just encoded. */ - sendme_cell_set_data_len(cell, sendme_cell_getlen_data(cell)); + (char *) sendme_cell_getarray_data_v1_digest(cell), + sendme_cell_get_data_len(cell)); /* Finally, encode the cell into the payload. */ len = sendme_cell_encode(payload, RELAY_PAYLOAD_SIZE, cell); - end: sendme_cell_free(cell); - sendme_data_v1_free(data); return len; } @@ -566,9 +549,9 @@ sendme_note_cell_digest(circuit_t *circ) * recorded. It should never happen in theory as we always record the last * digest for the v1 SENDME. */ if (TO_OR_CIRCUIT(circ)->crypto.sendme_digest) { - digest = tor_malloc_zero(4); + digest = tor_malloc_zero(TRUNNEL_SENDME_V1_DIGEST_LEN); crypto_digest_get_digest(TO_OR_CIRCUIT(circ)->crypto.sendme_digest, - (char *) digest, 4); + (char *) digest, TRUNNEL_SENDME_V1_DIGEST_LEN); if (circ->sendme_last_digests == NULL) { circ->sendme_last_digests = smartlist_new(); } diff --git a/src/test/test_sendme.c b/src/test/test_sendme.c index ad6aac6c0c..92f478df0e 100644 --- a/src/test/test_sendme.c +++ b/src/test/test_sendme.c @@ -156,12 +156,12 @@ test_v1_build_cell(void *arg) circ = TO_CIRCUIT(or_circ); cell_digest = crypto_digest_new(); - crypto_digest_add_bytes(cell_digest, "AAAA", 4); + crypto_digest_add_bytes(cell_digest, "AAAAAAAAAAAAAAAAAAAA", 20); tt_assert(cell_digest); - /* SENDME v1 payload is 7 bytes. See spec. */ + /* SENDME v1 payload is 3 bytes + 20 bytes digest. See spec. */ ret = build_cell_payload_v1(cell_digest, payload); - tt_int_op(ret, OP_EQ, 7); + tt_int_op(ret, OP_EQ, 23); /* Validation. */ diff --git a/src/trunnel/sendme.c b/src/trunnel/sendme.c index 08f9ed5e91..262b915234 100644 --- a/src/trunnel/sendme.c +++ b/src/trunnel/sendme.c @@ -43,8 +43,6 @@ static void sendme_cell_clear(sendme_cell_t *obj) { (void) obj; - TRUNNEL_DYNARRAY_WIPE(&obj->data); - TRUNNEL_DYNARRAY_CLEAR(&obj->data); } void @@ -84,71 +82,40 @@ sendme_cell_set_data_len(sendme_cell_t *inp, uint16_t val) return 0; } size_t -sendme_cell_getlen_data(const sendme_cell_t *inp) +sendme_cell_getlen_data_v1_digest(const sendme_cell_t *inp) { - return TRUNNEL_DYNARRAY_LEN(&inp->data); + (void)inp; return TRUNNEL_SENDME_V1_DIGEST_LEN; } uint8_t -sendme_cell_get_data(sendme_cell_t *inp, size_t idx) +sendme_cell_get_data_v1_digest(sendme_cell_t *inp, size_t idx) { - return TRUNNEL_DYNARRAY_GET(&inp->data, idx); + trunnel_assert(idx < TRUNNEL_SENDME_V1_DIGEST_LEN); + return inp->data_v1_digest[idx]; } uint8_t -sendme_cell_getconst_data(const sendme_cell_t *inp, size_t idx) +sendme_cell_getconst_data_v1_digest(const sendme_cell_t *inp, size_t idx) { - return sendme_cell_get_data((sendme_cell_t*)inp, idx); + return sendme_cell_get_data_v1_digest((sendme_cell_t*)inp, idx); } int -sendme_cell_set_data(sendme_cell_t *inp, size_t idx, uint8_t elt) +sendme_cell_set_data_v1_digest(sendme_cell_t *inp, size_t idx, uint8_t elt) { - TRUNNEL_DYNARRAY_SET(&inp->data, idx, elt); + trunnel_assert(idx < TRUNNEL_SENDME_V1_DIGEST_LEN); + inp->data_v1_digest[idx] = elt; return 0; } -int -sendme_cell_add_data(sendme_cell_t *inp, uint8_t elt) -{ -#if SIZE_MAX >= UINT16_MAX - if (inp->data.n_ == UINT16_MAX) - goto trunnel_alloc_failed; -#endif - TRUNNEL_DYNARRAY_ADD(uint8_t, &inp->data, elt, {}); - return 0; - trunnel_alloc_failed: - TRUNNEL_SET_ERROR_CODE(inp); - return -1; -} uint8_t * -sendme_cell_getarray_data(sendme_cell_t *inp) +sendme_cell_getarray_data_v1_digest(sendme_cell_t *inp) { - return inp->data.elts_; + return inp->data_v1_digest; } const uint8_t * -sendme_cell_getconstarray_data(const sendme_cell_t *inp) +sendme_cell_getconstarray_data_v1_digest(const sendme_cell_t *inp) { - return (const uint8_t *)sendme_cell_getarray_data((sendme_cell_t*)inp); -} -int -sendme_cell_setlen_data(sendme_cell_t *inp, size_t newlen) -{ - uint8_t *newptr; -#if UINT16_MAX < SIZE_MAX - if (newlen > UINT16_MAX) - goto trunnel_alloc_failed; -#endif - newptr = trunnel_dynarray_setlen(&inp->data.allocated_, - &inp->data.n_, inp->data.elts_, newlen, - sizeof(inp->data.elts_[0]), (trunnel_free_fn_t) NULL, - &inp->trunnel_error_code_); - if (newlen != 0 && newptr == NULL) - goto trunnel_alloc_failed; - inp->data.elts_ = newptr; - return 0; - trunnel_alloc_failed: - TRUNNEL_SET_ERROR_CODE(inp); - return -1; + return (const uint8_t *)sendme_cell_getarray_data_v1_digest((sendme_cell_t*)inp); } const char * sendme_cell_check(const sendme_cell_t *obj) @@ -159,8 +126,18 @@ sendme_cell_check(const sendme_cell_t *obj) return "A set function failed on this object"; if (! (obj->version == 0 || obj->version == 1)) return "Integer out of bounds"; - if (TRUNNEL_DYNARRAY_LEN(&obj->data) != obj->data_len) - return "Length mismatch for data"; + switch (obj->version) { + + case 0: + break; + + case 1: + break; + + default: + return "Bad tag for union"; + break; + } return NULL; } @@ -178,9 +155,21 @@ sendme_cell_encoded_len(const sendme_cell_t *obj) /* Length of u16 data_len */ result += 2; + switch (obj->version) { - /* Length of u8 data[data_len] */ - result += TRUNNEL_DYNARRAY_LEN(&obj->data); + case 0: + break; + + case 1: + + /* Length of u8 data_v1_digest[TRUNNEL_SENDME_V1_DIGEST_LEN] */ + result += TRUNNEL_SENDME_V1_DIGEST_LEN; + break; + + default: + trunnel_assert(0); + break; + } return result; } int @@ -201,6 +190,8 @@ sendme_cell_encode(uint8_t *output, const size_t avail, const sendme_cell_t *obj const ssize_t encoded_len = sendme_cell_encoded_len(obj); #endif + uint8_t *backptr_data_len = NULL; + if (NULL != (msg = sendme_cell_check(obj))) goto check_failed; @@ -216,22 +207,43 @@ sendme_cell_encode(uint8_t *output, const size_t avail, const sendme_cell_t *obj written += 1; ptr += 1; /* Encode u16 data_len */ + backptr_data_len = ptr; trunnel_assert(written <= avail); if (avail - written < 2) goto truncated; trunnel_set_uint16(ptr, trunnel_htons(obj->data_len)); written += 2; ptr += 2; - - /* Encode u8 data[data_len] */ { - size_t elt_len = TRUNNEL_DYNARRAY_LEN(&obj->data); - trunnel_assert(obj->data_len == elt_len); + size_t written_before_union = written; + + /* Encode union data[version] */ trunnel_assert(written <= avail); - if (avail - written < elt_len) - goto truncated; - if (elt_len) - memcpy(ptr, obj->data.elts_, elt_len); - written += elt_len; ptr += elt_len; + switch (obj->version) { + + case 0: + break; + + case 1: + + /* Encode u8 data_v1_digest[TRUNNEL_SENDME_V1_DIGEST_LEN] */ + trunnel_assert(written <= avail); + if (avail - written < TRUNNEL_SENDME_V1_DIGEST_LEN) + goto truncated; + memcpy(ptr, obj->data_v1_digest, TRUNNEL_SENDME_V1_DIGEST_LEN); + written += TRUNNEL_SENDME_V1_DIGEST_LEN; ptr += TRUNNEL_SENDME_V1_DIGEST_LEN; + break; + + default: + trunnel_assert(0); + break; + } + /* Write the length field back to data_len */ + trunnel_assert(written >= written_before_union); +#if UINT16_MAX < SIZE_MAX + if (written - written_before_union > UINT16_MAX) + goto check_failed; +#endif + trunnel_set_uint16(backptr_data_len, trunnel_htons(written - written_before_union)); } @@ -279,21 +291,41 @@ sendme_cell_parse_into(sendme_cell_t *obj, const uint8_t *input, const size_t le CHECK_REMAINING(2, truncated); obj->data_len = trunnel_ntohs(trunnel_get_uint16(ptr)); remaining -= 2; ptr += 2; + { + size_t remaining_after; + CHECK_REMAINING(obj->data_len, truncated); + remaining_after = remaining - obj->data_len; + remaining = obj->data_len; - /* Parse u8 data[data_len] */ - CHECK_REMAINING(obj->data_len, truncated); - TRUNNEL_DYNARRAY_EXPAND(uint8_t, &obj->data, obj->data_len, {}); - obj->data.n_ = obj->data_len; - if (obj->data_len) - memcpy(obj->data.elts_, ptr, obj->data_len); - ptr += obj->data_len; remaining -= obj->data_len; + /* Parse union data[version] */ + switch (obj->version) { + + case 0: + /* Skip to end of union */ + ptr += remaining; remaining = 0; + break; + + case 1: + + /* Parse u8 data_v1_digest[TRUNNEL_SENDME_V1_DIGEST_LEN] */ + CHECK_REMAINING(TRUNNEL_SENDME_V1_DIGEST_LEN, fail); + memcpy(obj->data_v1_digest, ptr, TRUNNEL_SENDME_V1_DIGEST_LEN); + remaining -= TRUNNEL_SENDME_V1_DIGEST_LEN; ptr += TRUNNEL_SENDME_V1_DIGEST_LEN; + break; + + default: + goto fail; + break; + } + if (remaining != 0) + goto fail; + remaining = remaining_after; + } trunnel_assert(ptr + remaining == input + len_in); return len_in - remaining; truncated: return -2; - trunnel_alloc_failed: - return -1; fail: result = -1; return result; @@ -313,180 +345,3 @@ sendme_cell_parse(sendme_cell_t **output, const uint8_t *input, const size_t len } return result; } -sendme_data_v1_t * -sendme_data_v1_new(void) -{ - sendme_data_v1_t *val = trunnel_calloc(1, sizeof(sendme_data_v1_t)); - if (NULL == val) - return NULL; - return val; -} - -/** Release all storage held inside 'obj', but do not free 'obj'. - */ -static void -sendme_data_v1_clear(sendme_data_v1_t *obj) -{ - (void) obj; -} - -void -sendme_data_v1_free(sendme_data_v1_t *obj) -{ - if (obj == NULL) - return; - sendme_data_v1_clear(obj); - trunnel_memwipe(obj, sizeof(sendme_data_v1_t)); - trunnel_free_(obj); -} - -size_t -sendme_data_v1_getlen_digest(const sendme_data_v1_t *inp) -{ - (void)inp; return 4; -} - -uint8_t -sendme_data_v1_get_digest(sendme_data_v1_t *inp, size_t idx) -{ - trunnel_assert(idx < 4); - return inp->digest[idx]; -} - -uint8_t -sendme_data_v1_getconst_digest(const sendme_data_v1_t *inp, size_t idx) -{ - return sendme_data_v1_get_digest((sendme_data_v1_t*)inp, idx); -} -int -sendme_data_v1_set_digest(sendme_data_v1_t *inp, size_t idx, uint8_t elt) -{ - trunnel_assert(idx < 4); - inp->digest[idx] = elt; - return 0; -} - -uint8_t * -sendme_data_v1_getarray_digest(sendme_data_v1_t *inp) -{ - return inp->digest; -} -const uint8_t * -sendme_data_v1_getconstarray_digest(const sendme_data_v1_t *inp) -{ - return (const uint8_t *)sendme_data_v1_getarray_digest((sendme_data_v1_t*)inp); -} -const char * -sendme_data_v1_check(const sendme_data_v1_t *obj) -{ - if (obj == NULL) - return "Object was NULL"; - if (obj->trunnel_error_code_) - return "A set function failed on this object"; - return NULL; -} - -ssize_t -sendme_data_v1_encoded_len(const sendme_data_v1_t *obj) -{ - ssize_t result = 0; - - if (NULL != sendme_data_v1_check(obj)) - return -1; - - - /* Length of u8 digest[4] */ - result += 4; - return result; -} -int -sendme_data_v1_clear_errors(sendme_data_v1_t *obj) -{ - int r = obj->trunnel_error_code_; - obj->trunnel_error_code_ = 0; - return r; -} -ssize_t -sendme_data_v1_encode(uint8_t *output, const size_t avail, const sendme_data_v1_t *obj) -{ - ssize_t result = 0; - size_t written = 0; - uint8_t *ptr = output; - const char *msg; -#ifdef TRUNNEL_CHECK_ENCODED_LEN - const ssize_t encoded_len = sendme_data_v1_encoded_len(obj); -#endif - - if (NULL != (msg = sendme_data_v1_check(obj))) - goto check_failed; - -#ifdef TRUNNEL_CHECK_ENCODED_LEN - trunnel_assert(encoded_len >= 0); -#endif - - /* Encode u8 digest[4] */ - trunnel_assert(written <= avail); - if (avail - written < 4) - goto truncated; - memcpy(ptr, obj->digest, 4); - written += 4; ptr += 4; - - - trunnel_assert(ptr == output + written); -#ifdef TRUNNEL_CHECK_ENCODED_LEN - { - trunnel_assert(encoded_len >= 0); - trunnel_assert((size_t)encoded_len == written); - } - -#endif - - return written; - - truncated: - result = -2; - goto fail; - check_failed: - (void)msg; - result = -1; - goto fail; - fail: - trunnel_assert(result < 0); - return result; -} - -/** As sendme_data_v1_parse(), but do not allocate the output object. - */ -static ssize_t -sendme_data_v1_parse_into(sendme_data_v1_t *obj, const uint8_t *input, const size_t len_in) -{ - const uint8_t *ptr = input; - size_t remaining = len_in; - ssize_t result = 0; - (void)result; - - /* Parse u8 digest[4] */ - CHECK_REMAINING(4, truncated); - memcpy(obj->digest, ptr, 4); - remaining -= 4; ptr += 4; - trunnel_assert(ptr + remaining == input + len_in); - return len_in - remaining; - - truncated: - return -2; -} - -ssize_t -sendme_data_v1_parse(sendme_data_v1_t **output, const uint8_t *input, const size_t len_in) -{ - ssize_t result; - *output = sendme_data_v1_new(); - if (NULL == *output) - return -1; - result = sendme_data_v1_parse_into(*output, input, len_in); - if (result < 0) { - sendme_data_v1_free(*output); - *output = NULL; - } - return result; -} diff --git a/src/trunnel/sendme.h b/src/trunnel/sendme.h index 8207cb56f7..f3c3dd78c4 100644 --- a/src/trunnel/sendme.h +++ b/src/trunnel/sendme.h @@ -8,22 +8,16 @@ #include #include "trunnel.h" +#define TRUNNEL_SENDME_V1_DIGEST_LEN 20 #if !defined(TRUNNEL_OPAQUE) && !defined(TRUNNEL_OPAQUE_SENDME_CELL) struct sendme_cell_st { uint8_t version; uint16_t data_len; - TRUNNEL_DYNARRAY_HEAD(, uint8_t) data; + uint8_t data_v1_digest[TRUNNEL_SENDME_V1_DIGEST_LEN]; uint8_t trunnel_error_code_; }; #endif typedef struct sendme_cell_st sendme_cell_t; -#if !defined(TRUNNEL_OPAQUE) && !defined(TRUNNEL_OPAQUE_SENDME_DATA_V1) -struct sendme_data_v1_st { - uint8_t digest[4]; - uint8_t trunnel_error_code_; -}; -#endif -typedef struct sendme_data_v1_st sendme_data_v1_t; /** Return a newly allocated sendme_cell with all elements set to * zero. */ @@ -77,94 +71,31 @@ uint16_t sendme_cell_get_data_len(const sendme_cell_t *inp); * 'inp' on failure. */ int sendme_cell_set_data_len(sendme_cell_t *inp, uint16_t val); -/** Return the length of the dynamic array holding the data field of - * the sendme_cell_t in 'inp'. +/** Return the (constant) length of the array holding the + * data_v1_digest field of the sendme_cell_t in 'inp'. */ -size_t sendme_cell_getlen_data(const sendme_cell_t *inp); -/** Return the element at position 'idx' of the dynamic array field - * data of the sendme_cell_t in 'inp'. - */ -uint8_t sendme_cell_get_data(sendme_cell_t *inp, size_t idx); -/** As sendme_cell_get_data, but take and return a const pointer - */ -uint8_t sendme_cell_getconst_data(const sendme_cell_t *inp, size_t idx); -/** Change the element at position 'idx' of the dynamic array field - * data of the sendme_cell_t in 'inp', so that it will hold the value - * 'elt'. - */ -int sendme_cell_set_data(sendme_cell_t *inp, size_t idx, uint8_t elt); -/** Append a new element 'elt' to the dynamic array field data of the - * sendme_cell_t in 'inp'. - */ -int sendme_cell_add_data(sendme_cell_t *inp, uint8_t elt); -/** Return a pointer to the variable-length array field data of 'inp'. - */ -uint8_t * sendme_cell_getarray_data(sendme_cell_t *inp); -/** As sendme_cell_get_data, but take and return a const pointer - */ -const uint8_t * sendme_cell_getconstarray_data(const sendme_cell_t *inp); -/** Change the length of the variable-length array field data of 'inp' - * to 'newlen'.Fill extra elements with 0. Return 0 on success; return - * -1 and set the error code on 'inp' on failure. - */ -int sendme_cell_setlen_data(sendme_cell_t *inp, size_t newlen); -/** Return a newly allocated sendme_data_v1 with all elements set to - * zero. - */ -sendme_data_v1_t *sendme_data_v1_new(void); -/** Release all storage held by the sendme_data_v1 in 'victim'. (Do - * nothing if 'victim' is NULL.) - */ -void sendme_data_v1_free(sendme_data_v1_t *victim); -/** Try to parse a sendme_data_v1 from the buffer in 'input', using up - * to 'len_in' bytes from the input buffer. On success, return the - * number of bytes consumed and set *output to the newly allocated - * sendme_data_v1_t. On failure, return -2 if the input appears - * truncated, and -1 if the input is otherwise invalid. - */ -ssize_t sendme_data_v1_parse(sendme_data_v1_t **output, const uint8_t *input, const size_t len_in); -/** Return the number of bytes we expect to need to encode the - * sendme_data_v1 in 'obj'. On failure, return a negative value. Note - * that this value may be an overestimate, and can even be an - * underestimate for certain unencodeable objects. - */ -ssize_t sendme_data_v1_encoded_len(const sendme_data_v1_t *obj); -/** Try to encode the sendme_data_v1 from 'input' into the buffer at - * 'output', using up to 'avail' bytes of the output buffer. On - * success, return the number of bytes used. On failure, return -2 if - * the buffer was not long enough, and -1 if the input was invalid. - */ -ssize_t sendme_data_v1_encode(uint8_t *output, size_t avail, const sendme_data_v1_t *input); -/** Check whether the internal state of the sendme_data_v1 in 'obj' is - * consistent. Return NULL if it is, and a short message if it is not. - */ -const char *sendme_data_v1_check(const sendme_data_v1_t *obj); -/** Clear any errors that were set on the object 'obj' by its setter - * functions. Return true iff errors were cleared. - */ -int sendme_data_v1_clear_errors(sendme_data_v1_t *obj); -/** Return the (constant) length of the array holding the digest field - * of the sendme_data_v1_t in 'inp'. - */ -size_t sendme_data_v1_getlen_digest(const sendme_data_v1_t *inp); +size_t sendme_cell_getlen_data_v1_digest(const sendme_cell_t *inp); /** Return the element at position 'idx' of the fixed array field - * digest of the sendme_data_v1_t in 'inp'. + * data_v1_digest of the sendme_cell_t in 'inp'. */ -uint8_t sendme_data_v1_get_digest(sendme_data_v1_t *inp, size_t idx); -/** As sendme_data_v1_get_digest, but take and return a const pointer +uint8_t sendme_cell_get_data_v1_digest(sendme_cell_t *inp, size_t idx); +/** As sendme_cell_get_data_v1_digest, but take and return a const + * pointer */ -uint8_t sendme_data_v1_getconst_digest(const sendme_data_v1_t *inp, size_t idx); +uint8_t sendme_cell_getconst_data_v1_digest(const sendme_cell_t *inp, size_t idx); /** Change the element at position 'idx' of the fixed array field - * digest of the sendme_data_v1_t in 'inp', so that it will hold the - * value 'elt'. + * data_v1_digest of the sendme_cell_t in 'inp', so that it will hold + * the value 'elt'. */ -int sendme_data_v1_set_digest(sendme_data_v1_t *inp, size_t idx, uint8_t elt); -/** Return a pointer to the 4-element array field digest of 'inp'. +int sendme_cell_set_data_v1_digest(sendme_cell_t *inp, size_t idx, uint8_t elt); +/** Return a pointer to the TRUNNEL_SENDME_V1_DIGEST_LEN-element array + * field data_v1_digest of 'inp'. */ -uint8_t * sendme_data_v1_getarray_digest(sendme_data_v1_t *inp); -/** As sendme_data_v1_get_digest, but take and return a const pointer +uint8_t * sendme_cell_getarray_data_v1_digest(sendme_cell_t *inp); +/** As sendme_cell_get_data_v1_digest, but take and return a const + * pointer */ -const uint8_t * sendme_data_v1_getconstarray_digest(const sendme_data_v1_t *inp); +const uint8_t * sendme_cell_getconstarray_data_v1_digest(const sendme_cell_t *inp); #endif diff --git a/src/trunnel/sendme.trunnel b/src/trunnel/sendme.trunnel index 7294a09b47..300963e679 100644 --- a/src/trunnel/sendme.trunnel +++ b/src/trunnel/sendme.trunnel @@ -1,18 +1,19 @@ /* This file contains the SENDME cell definition. */ +/* v1 digest length in bytes. */ +const TRUNNEL_SENDME_V1_DIGEST_LEN = 20; + +/* SENDME cell declaration. */ struct sendme_cell { /* Version field. */ u8 version IN [0x00, 0x01]; - /* The data content depends on the version. */ + /* Length of data contained in this cell. */ u16 data_len; - u8 data[data_len]; -} -/* SENDME version 0. No data. */ - -/* SENDME version 1. Authenticated with digest. */ -struct sendme_data_v1 { - /* A 4 bytes digest. */ - u8 digest[4]; + /* The data content depends on the version. */ + union data[version] with length data_len { + 0x00: ignore; + 0x01: u8 v1_digest[TRUNNEL_SENDME_V1_DIGEST_LEN]; + }; } From 2ec25e847eb2d9af0a1a1c552ffa8dbd87cf6023 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 7 Mar 2019 11:35:52 -0500 Subject: [PATCH 17/32] prop289: Move SENDME cell processing in a separate function No behavior change. Only moving code and fixing part of it in order to use the parameters passed as pointers. Part of #26288 Signed-off-by: David Goulet --- src/core/or/relay.c | 137 ++++++++++++++++++++++++-------------------- 1 file changed, 76 insertions(+), 61 deletions(-) diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 63c406d8af..6bf7ac1a7a 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -1444,6 +1444,81 @@ connection_edge_process_relay_cell_not_open( // return -1; } +/** Process a SENDME cell that arrived on circ. If it is a stream level + * cell, it is destined for the given conn. If it is a circuit level + * cell, it is destined for the layer_hint. The domain is the + * logging domain that should be used. + * + * Return 0 if everything went well or a negative value representing a circuit + * end reason on error for which the caller is responsible for closing it. */ +static int +process_sendme_cell(const relay_header_t *rh, const cell_t *cell, + circuit_t *circ, edge_connection_t *conn, + crypt_path_t *layer_hint, int domain) +{ + int ret; + + tor_assert(rh); + + if (!rh->stream_id) { + /* Circuit level SENDME cell. */ + ret = sendme_process_circuit_level(layer_hint, circ, + cell->payload + RELAY_HEADER_SIZE, + rh->length); + if (ret < 0) { + return ret; + } + /* Resume reading on any streams now that we've processed a valid + * SENDME cell that updated our package window. */ + circuit_resume_edge_reading(circ, layer_hint); + /* We are done, the rest of the code is for the stream level. */ + return 0; + } + + /* No connection, might be half edge state. We are done if so. */ + if (!conn) { + if (CIRCUIT_IS_ORIGIN(circ)) { + origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); + if (connection_half_edge_is_valid_sendme(ocirc->half_streams, + rh->stream_id)) { + circuit_read_valid_data(ocirc, rh->length); + log_info(domain, "Sendme cell on circ %u valid on half-closed " + "stream id %d", + ocirc->global_identifier, rh->stream_id); + } + } + + log_info(domain, "SENDME cell dropped, unknown stream (streamid %d).", + rh->stream_id); + return 0; + } + + /* Stream level SENDME cell. */ + ret = sendme_process_stream_level(conn, circ, rh->length); + if (ret < 0) { + /* Means we need to close the circuit with reason ret. */ + return ret; + } + + /* We've now processed properly a SENDME cell, all windows have been + * properly updated, we'll read on the edge connection to see if we can + * get data out towards the end point (Exit or client) since we are now + * allowed to deliver more cells. */ + + if (circuit_queue_streams_are_blocked(circ)) { + /* Still waiting for queue to flush; don't touch conn */ + return 0; + } + connection_start_reading(TO_CONN(conn)); + /* handle whatever might still be on the inbuf */ + if (connection_edge_package_raw_inbuf(conn, 1, NULL) < 0) { + /* (We already sent an end cell if possible) */ + connection_mark_for_close(TO_CONN(conn)); + return 0; + } + return 0; +} + /** An incoming relay cell has arrived on circuit circ. If * conn is NULL this is a control cell, else cell is * destined for conn. @@ -1825,67 +1900,7 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, (unsigned)circ->n_circ_id, rh.stream_id); return 0; case RELAY_COMMAND_SENDME: - { - int ret; - - if (!rh.stream_id) { - /* Circuit level SENDME cell. */ - ret = sendme_process_circuit_level(layer_hint, circ, - cell->payload + RELAY_HEADER_SIZE, - rh.length); - if (ret < 0) { - return ret; - } - /* Resume reading on any streams now that we've processed a valid - * SENDME cell that updated our package window. */ - circuit_resume_edge_reading(circ, layer_hint); - /* We are done, the rest of the code is for the stream level. */ - return 0; - } - - /* No connection, might be half edge state. We are done if so. */ - if (!conn) { - if (CIRCUIT_IS_ORIGIN(circ)) { - origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); - if (connection_half_edge_is_valid_sendme(ocirc->half_streams, - rh.stream_id)) { - circuit_read_valid_data(ocirc, rh.length); - log_info(domain, "Sendme cell on circ %u valid on half-closed " - "stream id %d", - ocirc->global_identifier, rh.stream_id); - } - } - - log_info(domain, "SENDME cell dropped, unknown stream (streamid %d).", - rh.stream_id); - return 0; - } - - /* Stream level SENDME cell. */ - ret = sendme_process_stream_level(conn, circ, rh.length); - if (ret < 0) { - /* Means we need to close the circuit with reason ret. */ - return ret; - } - - /* We've now processed properly a SENDME cell, all windows have been - * properly updated, we'll read on the edge connection to see if we can - * get data out towards the end point (Exit or client) since we are now - * allowed to deliver more cells. */ - - if (circuit_queue_streams_are_blocked(circ)) { - /* Still waiting for queue to flush; don't touch conn */ - return 0; - } - connection_start_reading(TO_CONN(conn)); - /* handle whatever might still be on the inbuf */ - if (connection_edge_package_raw_inbuf(conn, 1, NULL) < 0) { - /* (We already sent an end cell if possible) */ - connection_mark_for_close(TO_CONN(conn)); - return 0; - } - return 0; - } + return process_sendme_cell(&rh, cell, circ, conn, layer_hint, domain); case RELAY_COMMAND_RESOLVE: if (layer_hint) { log_fn(LOG_PROTOCOL_WARN, LD_APP, From 217b55319336227f9e397db526cea551dbd796e4 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 7 Mar 2019 11:45:38 -0500 Subject: [PATCH 18/32] prop289: Rename packaged functions with better name The circuit and stream level functions that update the package window have been renamed to have a "_note_" in them to make their purpose more clear. Part of #26288 Signed-off-by: David Goulet --- src/core/or/relay.c | 4 ++-- src/core/or/sendme.c | 4 ++-- src/core/or/sendme.h | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 6bf7ac1a7a..fa008120b3 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -2092,7 +2092,7 @@ connection_edge_package_raw_inbuf(edge_connection_t *conn, int package_partial, } /* Handle the circuit-level SENDME package window. */ - if (sendme_circuit_data_packaged(circ, cpath_layer) < 0) { + if (sendme_note_circuit_data_packaged(circ, cpath_layer) < 0) { /* Package window has gone under 0. Protocol issue. */ log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "Circuit package window is below 0. Closing circuit."); @@ -2101,7 +2101,7 @@ connection_edge_package_raw_inbuf(edge_connection_t *conn, int package_partial, } /* Handle the stream-level SENDME package window. */ - if (sendme_stream_data_packaged(conn) < 0) { + if (sendme_note_stream_data_packaged(conn) < 0) { connection_stop_reading(TO_CONN(conn)); log_debug(domain,"conn->package_window reached 0."); circuit_consider_stop_edge_reading(circ, cpath_layer); diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index a333b02b60..16ff5bcb8f 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -491,7 +491,7 @@ sendme_stream_data_received(edge_connection_t *conn) * layer_hint is NULL, this means we are the Exit end point else we are the * Client. Update the package window and return its new value. */ int -sendme_circuit_data_packaged(circuit_t *circ, crypt_path_t *layer_hint) +sendme_note_circuit_data_packaged(circuit_t *circ, crypt_path_t *layer_hint) { int package_window, domain; @@ -518,7 +518,7 @@ sendme_circuit_data_packaged(circuit_t *circ, crypt_path_t *layer_hint) /* Called when a relay DATA cell is packaged for the given edge connection * conn. Update the package window and return its new value. */ int -sendme_stream_data_packaged(edge_connection_t *conn) +sendme_note_stream_data_packaged(edge_connection_t *conn) { tor_assert(conn); return --conn->package_window; diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index c2e2518da8..2154a29f4a 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -30,8 +30,9 @@ int sendme_stream_data_received(edge_connection_t *conn); int sendme_circuit_data_received(circuit_t *circ, crypt_path_t *layer_hint); /* Update package window functions. */ -int sendme_circuit_data_packaged(circuit_t *circ, crypt_path_t *layer_hint); -int sendme_stream_data_packaged(edge_connection_t *conn); +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_note_cell_digest(circuit_t *circ); From 4efe9d653aa1d375d77d6dca83ca63787d6599d7 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 7 Mar 2019 12:01:58 -0500 Subject: [PATCH 19/32] prop289: Move digest matching in its own function No behavior change but code had to be refactored a bit. Also, the tor_memcmp() was changed to tor_memneq(). Part of #26288 Signed-off-by: David Goulet --- src/core/or/sendme.c | 80 +++++++++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 35 deletions(-) diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 16ff5bcb8f..0a7b1cbc02 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -59,6 +59,49 @@ 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) +{ + bool ret = false; + uint8_t *circ_digest = NULL; + + 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; + } + + /* 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; + } + /* 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; +} + /* Return true iff the given decoded SENDME version 1 cell is valid and * matches the expected digest on the circuit. * @@ -68,44 +111,11 @@ get_accept_min_version(void) static bool cell_v1_is_valid(const sendme_cell_t *cell, const circuit_t *circ) { - const uint8_t *cell_digest = NULL; - tor_assert(cell); tor_assert(circ); - cell_digest = sendme_cell_getconstarray_data_v1_digest(cell); - - /* We shouldn't have received this 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 invalid; - } - - /* Pop the first element that was added (FIFO) and compare it. */ - { - uint8_t *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_memcmp(digest, cell_digest, TRUNNEL_SENDME_V1_DIGEST_LEN)) { - tor_free(digest); - log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, - "SENDME v1 cell digest do not match."); - goto invalid; - } - tor_free(digest); - } - - /* Validated SENDME v1 cell. */ - return 1; - invalid: - return 0; + const uint8_t *cell_digest = sendme_cell_getconstarray_data_v1_digest(cell); + return v1_digest_matches(circ, cell_digest); } /* Return true iff the given cell version can be handled or if the minimum From 77d560af64226eaa0fde157d7a6607791975a7a9 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 7 Mar 2019 12:30:13 -0500 Subject: [PATCH 20/32] prop289: Keep the digest bytes, not the object The digest object is as large as the entire internal digest object's state, which is often much larger than the actual set of bytes you're transmitting. This commit makes it that we keep the digest itself which is 20 bytes. Part of #26288 Signed-off-by: David Goulet --- src/core/crypto/relay_crypto.c | 17 ++++++----------- src/core/or/relay_crypto_st.h | 2 +- src/core/or/sendme.c | 29 +++++++++++------------------ src/core/or/sendme.h | 2 +- src/test/test_relaycell.c | 1 - src/test/test_sendme.c | 16 +++++++--------- 6 files changed, 26 insertions(+), 41 deletions(-) diff --git a/src/core/crypto/relay_crypto.c b/src/core/crypto/relay_crypto.c index d4116d47ab..94e9060651 100644 --- a/src/core/crypto/relay_crypto.c +++ b/src/core/crypto/relay_crypto.c @@ -143,12 +143,9 @@ relay_decrypt_cell(circuit_t *circ, cell_t *cell, *recognized = 1; *layer_hint = thishop; /* Keep current digest of this cell for the possible SENDME. */ - if (thishop->crypto.sendme_digest) { - crypto_digest_free(thishop->crypto.sendme_digest); - } - thishop->crypto.sendme_digest = - crypto_digest_dup(thishop->crypto.b_digest); - + crypto_digest_get_digest(thishop->crypto.b_digest, + (char *) thishop->crypto.sendme_digest, + sizeof(thishop->crypto.sendme_digest)); return 0; } } @@ -220,10 +217,9 @@ relay_encrypt_cell_inbound(cell_t *cell, { relay_set_digest(or_circ->crypto.b_digest, cell); /* Keep a record of this cell, we might use it for validating the SENDME. */ - if (or_circ->crypto.sendme_digest) { - crypto_digest_free(or_circ->crypto.sendme_digest); - } - or_circ->crypto.sendme_digest = crypto_digest_dup(or_circ->crypto.b_digest); + crypto_digest_get_digest(or_circ->crypto.b_digest, + (char *) or_circ->crypto.sendme_digest, + sizeof(or_circ->crypto.sendme_digest)); /* encrypt one layer */ relay_crypt_one_payload(or_circ->crypto.b_crypto, cell->payload); } @@ -241,7 +237,6 @@ relay_crypto_clear(relay_crypto_t *crypto) crypto_cipher_free(crypto->b_crypto); crypto_digest_free(crypto->f_digest); crypto_digest_free(crypto->b_digest); - crypto_digest_free(crypto->sendme_digest); } /** Initialize crypto from the key material in key_data. diff --git a/src/core/or/relay_crypto_st.h b/src/core/or/relay_crypto_st.h index dbdf1599dc..1f243ccdc8 100644 --- a/src/core/or/relay_crypto_st.h +++ b/src/core/or/relay_crypto_st.h @@ -26,7 +26,7 @@ struct relay_crypto_t { struct crypto_digest_t *b_digest; /** Digest used for the next SENDME cell if any. */ - struct crypto_digest_t *sendme_digest; + uint8_t sendme_digest[DIGEST_LEN]; }; #undef crypto_cipher_t diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 0a7b1cbc02..3dcd9df08e 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -215,7 +215,7 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, * Return the size in bytes of the encoded cell in payload. A negative value * is returned on encoding failure. */ STATIC ssize_t -build_cell_payload_v1(crypto_digest_t *cell_digest, uint8_t *payload) +build_cell_payload_v1(const uint8_t *cell_digest, uint8_t *payload) { ssize_t len = -1; sendme_cell_t *cell = NULL; @@ -231,9 +231,8 @@ build_cell_payload_v1(crypto_digest_t *cell_digest, uint8_t *payload) sendme_cell_set_data_len(cell, TRUNNEL_SENDME_V1_DIGEST_LEN); /* Copy the digest into the data payload. */ - crypto_digest_get_digest(cell_digest, - (char *) sendme_cell_getarray_data_v1_digest(cell), - sendme_cell_get_data_len(cell)); + memcpy(sendme_cell_getarray_data_v1_digest(cell), cell_digest, + sendme_cell_get_data_len(cell)); /* Finally, encode the cell into the payload. */ len = sendme_cell_encode(payload, RELAY_PAYLOAD_SIZE, cell); @@ -249,7 +248,7 @@ build_cell_payload_v1(crypto_digest_t *cell_digest, uint8_t *payload) * because we failed to send the cell on it. */ static int send_circuit_level_sendme(circuit_t *circ, crypt_path_t *layer_hint, - crypto_digest_t *cell_digest) + const uint8_t *cell_digest) { uint8_t emit_version; uint8_t payload[RELAY_PAYLOAD_SIZE]; @@ -340,7 +339,7 @@ sendme_connection_edge_consider_sending(edge_connection_t *conn) void sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint) { - crypto_digest_t *digest; + const uint8_t *digest; while ((layer_hint ? layer_hint->deliver_window : circ->deliver_window) <= CIRCWINDOW_START - CIRCWINDOW_INCREMENT) { @@ -539,7 +538,7 @@ sendme_note_stream_data_packaged(edge_connection_t *conn) void sendme_note_cell_digest(circuit_t *circ) { - uint8_t *digest; + const uint8_t *digest; tor_assert(circ); @@ -555,16 +554,10 @@ sendme_note_cell_digest(circuit_t *circ) return; } - /* Only note the digest if we actually have the digest of the previous cell - * recorded. It should never happen in theory as we always record the last - * digest for the v1 SENDME. */ - if (TO_OR_CIRCUIT(circ)->crypto.sendme_digest) { - digest = tor_malloc_zero(TRUNNEL_SENDME_V1_DIGEST_LEN); - crypto_digest_get_digest(TO_OR_CIRCUIT(circ)->crypto.sendme_digest, - (char *) digest, TRUNNEL_SENDME_V1_DIGEST_LEN); - if (circ->sendme_last_digests == NULL) { - circ->sendme_last_digests = smartlist_new(); - } - smartlist_add(circ->sendme_last_digests, digest); + /* Add the digest to the last seen list in the circuit. */ + digest = TO_OR_CIRCUIT(circ)->crypto.sendme_digest; + if (circ->sendme_last_digests == NULL) { + circ->sendme_last_digests = smartlist_new(); } + smartlist_add(circ->sendme_last_digests, tor_memdup(digest, DIGEST_LEN)); } diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index 2154a29f4a..71df9b6f07 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -50,7 +50,7 @@ STATIC int get_accept_min_version(void); STATIC bool cell_version_is_valid(uint8_t cell_version); -STATIC ssize_t build_cell_payload_v1(crypto_digest_t *cell_digest, +STATIC ssize_t build_cell_payload_v1(const uint8_t *cell_digest, uint8_t *payload); STATIC bool sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload, diff --git a/src/test/test_relaycell.c b/src/test/test_relaycell.c index c4ed215c7c..0623583511 100644 --- a/src/test/test_relaycell.c +++ b/src/test/test_relaycell.c @@ -705,7 +705,6 @@ test_circbw_relay(void *arg) circ = helper_create_origin_circuit(CIRCUIT_PURPOSE_C_GENERAL, 0); circ->cpath->state = CPATH_STATE_AWAITING_KEYS; circ->cpath->deliver_window = CIRCWINDOW_START; - circ->cpath->crypto.sendme_digest = crypto_digest_new(); entryconn1 = fake_entry_conn(circ, 1); edgeconn = ENTRY_TO_EDGE_CONN(entryconn1); diff --git a/src/test/test_sendme.c b/src/test/test_sendme.c index 92f478df0e..d6410a7488 100644 --- a/src/test/test_sendme.c +++ b/src/test/test_sendme.c @@ -16,6 +16,8 @@ #include "feature/nodelist/networkstatus.h" #include "feature/nodelist/networkstatus_st.h" +#include "lib/crypt_ops/crypto_digest.h" + #include "test/test.h" #include "test/log_test_helpers.h" @@ -64,7 +66,6 @@ test_v1_note_digest(void *arg) circuit_free_(circ); /* Points it to the OR circuit now. */ circ = TO_CIRCUIT(or_circ); - or_circ->crypto.sendme_digest = crypto_digest_new(); /* The package window has to be a multiple of CIRCWINDOW_INCREMENT minus 1 * in order to catched the CIRCWINDOW_INCREMENT-nth cell. Try something that @@ -144,7 +145,7 @@ test_v1_consensus_params(void *arg) static void test_v1_build_cell(void *arg) { - uint8_t payload[RELAY_PAYLOAD_SIZE]; + uint8_t payload[RELAY_PAYLOAD_SIZE], digest[DIGEST_LEN]; ssize_t ret; crypto_digest_t *cell_digest = NULL; or_circuit_t *or_circ = NULL; @@ -156,11 +157,12 @@ test_v1_build_cell(void *arg) circ = TO_CIRCUIT(or_circ); cell_digest = crypto_digest_new(); - crypto_digest_add_bytes(cell_digest, "AAAAAAAAAAAAAAAAAAAA", 20); tt_assert(cell_digest); + crypto_digest_add_bytes(cell_digest, "AAAAAAAAAAAAAAAAAAAA", 20); + crypto_digest_get_digest(cell_digest, (char *) digest, sizeof(digest)); /* SENDME v1 payload is 3 bytes + 20 bytes digest. See spec. */ - ret = build_cell_payload_v1(cell_digest, payload); + ret = build_cell_payload_v1(digest, payload); tt_int_op(ret, OP_EQ, 23); /* Validation. */ @@ -183,7 +185,6 @@ test_v1_build_cell(void *arg) teardown_capture_of_logs(); /* Note the wrong digest in the circuit, cell should fail validation. */ - or_circ->crypto.sendme_digest = crypto_digest_new(); circ->package_window = CIRCWINDOW_INCREMENT + 1; sendme_note_cell_digest(circ); tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 1); @@ -194,11 +195,8 @@ test_v1_build_cell(void *arg) expect_log_msg_containing("SENDME v1 cell digest do not match."); teardown_capture_of_logs(); - /* Cleanup */ - crypto_digest_free(or_circ->crypto.sendme_digest); - /* Record the cell digest into the circuit, cell should validate. */ - or_circ->crypto.sendme_digest = crypto_digest_dup(cell_digest); + memcpy(or_circ->crypto.sendme_digest, digest, sizeof(digest)); circ->package_window = CIRCWINDOW_INCREMENT + 1; sendme_note_cell_digest(circ); tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 1); From 44750b0de60fe80ea81f7fc83f8713f814caf5a6 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 7 Mar 2019 12:45:16 -0500 Subject: [PATCH 21/32] prop289: Skip the first 4 unused bytes in a cell When adding random to a cell, skip the first 4 bytes and leave them zeroed. It has been very useful in the past for us to keep bytes like this. Some code trickery was added to make sure we have enough room for this 4 bytes offset when adding random. Part of #26288 Signed-off-by: David Goulet --- src/core/or/relay.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/core/or/relay.c b/src/core/or/relay.c index fa008120b3..504f391d9a 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -547,6 +547,8 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *circ, cell_t cell; relay_header_t rh; cell_direction_t cell_direction; + int random_bytes_len; + size_t random_bytes_offset = 0; /* XXXX NM Split this function into a separate versions per circuit type? */ tor_assert(circ); @@ -574,11 +576,20 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *circ, /* Add random bytes to the unused portion of the payload, to foil attacks * where the other side can predict all of the bytes in the payload and thus - * compute authenticated sendme cells without seeing the traffic. See - * proposal 289. */ + * compute authenticated sendme cells without seeing the traffic. See + * proposal 289. + * + * We'll skip the first 4 bytes of unused data because having some unused + * zero bytes has saved us a lot of times in the past. */ + random_bytes_len = RELAY_PAYLOAD_SIZE - + (RELAY_HEADER_SIZE + payload_len + 4); + if (random_bytes_len < 0) { + random_bytes_len = 0; + } + random_bytes_offset = RELAY_PAYLOAD_SIZE - random_bytes_len; crypto_fast_rng_getbytes(get_thread_fast_rng(), - cell.payload + RELAY_HEADER_SIZE + payload_len, - RELAY_PAYLOAD_SIZE - payload_len); + cell.payload + random_bytes_offset, + random_bytes_len); log_debug(LD_OR,"delivering %d cell %s.", relay_command, cell_direction == CELL_DIRECTION_OUT ? "forward" : "backward"); From aef7095c3e52e2f98850e72c68b00f54a39608a6 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 7 Mar 2019 12:57:15 -0500 Subject: [PATCH 22/32] prop289: Add documentation for the circuit FIFO list Part of #26288 Signed-off-by: David Goulet --- src/core/or/circuit_st.h | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/core/or/circuit_st.h b/src/core/or/circuit_st.h index 5adb158935..a68547ecb1 100644 --- a/src/core/or/circuit_st.h +++ b/src/core/or/circuit_st.h @@ -106,9 +106,23 @@ struct circuit_t { int deliver_window; /** FIFO containing the digest of the cells that are just before a SENDME is * sent by the client. It is done at the last cell before our package_window - * goes down to 0 which is when we expect a SENDME. The protocol doesn't - * allow more than 10 outstanding SENDMEs worth of data meaning this list - * should only contain at most 10 digests of 4 bytes each. */ + * goes down to 0 which is when we expect a SENDME. + * + * Our current circuit package window is capped to 1000 + * (CIRCWINDOW_START_MAX) which is also the start value. The increment is + * set to 100 (CIRCWINDOW_INCREMENT) which means we don't allow more than + * 1000/100 = 10 outstanding SENDME cells worth of data. Meaning that this + * 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. + * + * For example, position 2 (starting at 0) means that we've received 299 + * cells and the 299th cell digest is kept at index 2. + * + * At maximum, this list contains 200 bytes plus the smartlist overhead. */ smartlist_t *sendme_last_digests; /** Temporary field used during circuits_handle_oom. */ From 7c8e519b3452ce3eb3d3c854d80be5b7e49164b4 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 24 Apr 2019 10:25:29 -0400 Subject: [PATCH 23/32] sendme: Helper to know if next cell is a SENDME We'll use it this in order to know when to hash the cell for the SENDME instead of doing it at every cell. Part of #26288 Signed-off-by: David Goulet --- src/core/or/sendme.c | 25 ++++++++++++++++++++++++- src/core/or/sendme.h | 3 +++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 3dcd9df08e..c66e947bc4 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -286,6 +286,29 @@ send_circuit_level_sendme(circuit_t *circ, crypt_path_t *layer_hint, return 0; } +/* + * Public API + */ + +/** 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_is_next_cell(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. */ + if (((window - 1) % CIRCWINDOW_INCREMENT) != 0) { + return false; + } + /* Next cell is expected to be a SENDME. */ + return true; +} + /** Called when we've just received a relay data cell, when we've just * finished flushing all bytes to stream conn, or when we've flushed * *some* bytes to the stream conn. @@ -550,7 +573,7 @@ sendme_note_cell_digest(circuit_t *circ) /* 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 (((circ->package_window - 1) % CIRCWINDOW_INCREMENT) != 0) { + if (!sendme_circuit_is_next_cell(circ->package_window)) { return; } diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index 71df9b6f07..0965b5b22a 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -37,6 +37,9 @@ int sendme_note_stream_data_packaged(edge_connection_t *conn); /* Track cell digest. */ void sendme_note_cell_digest(circuit_t *circ); +/* Circuit level information. */ +bool sendme_circuit_is_next_cell(int window); + /* Private section starts. */ #ifdef SENDME_PRIVATE From 805c81efed9bc2c474d3f10675846ee445a908d5 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 24 Apr 2019 11:57:20 -0400 Subject: [PATCH 24/32] sendme: Add helper to note the cell digest Signed-off-by: David Goulet --- src/core/or/sendme.c | 27 +++++++++++++++++++++++++++ src/core/or/sendme.h | 2 ++ 2 files changed, 29 insertions(+) diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index c66e947bc4..b384a19164 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -286,10 +286,37 @@ send_circuit_level_sendme(circuit_t *circ, crypt_path_t *layer_hint, return 0; } +/* Put the crypto.b_digest in the sendme_digest. */ +static void +note_cell_digest(const relay_crypto_t *crypto) +{ + tor_assert(crypto); + crypto_digest_get_digest(crypto->b_digest, (char *) crypto->sendme_digest, + sizeof(crypto->sendme_digest)); +} + /* * 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_note_outbound_cell(or_circuit_t *or_circ) +{ + tor_assert(or_circ); + note_cell_digest(&or_circ->crypto); +} + +/** 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_note_inbound_cell(crypt_path_t *cpath) +{ + tor_assert(cpath); + note_cell_digest(&cpath->crypto); +} + /** Return true iff the next cell for the given cell window is expected to be * a SENDME. * diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index 0965b5b22a..5889b41e9c 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -36,6 +36,8 @@ int sendme_note_stream_data_packaged(edge_connection_t *conn); /* Track cell digest. */ void sendme_note_cell_digest(circuit_t *circ); +void sendme_circuit_note_inbound_cell(crypt_path_t *cpath); +void sendme_circuit_note_outbound_cell(or_circuit_t *or_circ); /* Circuit level information. */ bool sendme_circuit_is_next_cell(int window); From c7385b5b14b30774c1768798c4495465da4d995d Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 24 Apr 2019 13:38:47 -0400 Subject: [PATCH 25/32] sendme: Keep cell digest only if a SENDME is next This way, we reduce the load by only hashing when we absolutely must. Part of #26288 Signed-off-by: David Goulet --- src/core/crypto/relay_crypto.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/core/crypto/relay_crypto.c b/src/core/crypto/relay_crypto.c index 94e9060651..eddc4298e2 100644 --- a/src/core/crypto/relay_crypto.c +++ b/src/core/crypto/relay_crypto.c @@ -12,6 +12,7 @@ #include "core/crypto/hs_ntor.h" // for HS_NTOR_KEY_EXPANSION_KDF_OUT_LEN #include "core/or/relay.h" #include "core/crypto/relay_crypto.h" +#include "core/or/sendme.h" #include "core/or/cell_st.h" #include "core/or/or_circuit_st.h" @@ -142,10 +143,11 @@ relay_decrypt_cell(circuit_t *circ, cell_t *cell, if (relay_digest_matches(thishop->crypto.b_digest, cell)) { *recognized = 1; *layer_hint = thishop; - /* Keep current digest of this cell for the possible SENDME. */ - crypto_digest_get_digest(thishop->crypto.b_digest, - (char *) thishop->crypto.sendme_digest, - sizeof(thishop->crypto.sendme_digest)); + /* This cell is for us. Keep a record of this cell because we will + * use it in the next SENDME cell. */ + if (sendme_circuit_is_next_cell(thishop->deliver_window)) { + sendme_circuit_note_inbound_cell(thishop); + } return 0; } } @@ -216,10 +218,13 @@ relay_encrypt_cell_inbound(cell_t *cell, or_circuit_t *or_circ) { relay_set_digest(or_circ->crypto.b_digest, cell); - /* Keep a record of this cell, we might use it for validating the SENDME. */ - crypto_digest_get_digest(or_circ->crypto.b_digest, - (char *) or_circ->crypto.sendme_digest, - sizeof(or_circ->crypto.sendme_digest)); + + /* 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_is_next_cell(TO_CIRCUIT(or_circ)->package_window)) { + sendme_circuit_note_outbound_cell(or_circ); + } + /* encrypt one layer */ relay_crypt_one_payload(or_circ->crypto.b_crypto, cell->payload); } From d084f9115d7d46ad5e029b9c75cea716fa7d65a5 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 24 Apr 2019 15:39:10 -0400 Subject: [PATCH 26/32] sendme: Better handle the random padding We add random padding to every cell if there is room. This commit not only fixes how we compute that random padding length/offset but also improves its safety with helper functions and a unit test. Part of #26288 Signed-off-by: David Goulet --- src/core/or/relay.c | 74 ++++++++++++++++++++++++++++++++---------- src/core/or/relay.h | 1 + src/test/test_sendme.c | 46 ++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 18 deletions(-) diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 504f391d9a..d273facd55 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -529,6 +529,60 @@ relay_command_to_string(uint8_t command) } } +/** Return the offset where the padding should start. The data_len is + * the relay payload length expected to be put in the cell. It can not be + * bigger than RELAY_PAYLOAD_SIZE else this function assert(). + * + * Value will always be smaller than CELL_PAYLOAD_SIZE because this offset is + * for the entire cell length not just the data payload length. Zero is + * returned if there is no room for padding. + * + * This function always skips the first 4 bytes after the payload because + * having some unused zero bytes has saved us a lot of times in the past. */ + +STATIC size_t +get_pad_cell_offset(size_t data_len) +{ + /* This is never suppose to happen but in case it does, stop right away + * because if tor is tricked somehow into not adding random bytes to the + * payload with this function returning 0 for a bad data_len, the entire + * authenticated SENDME design can be bypassed leading to bad denial of + * service attacks. */ + tor_assert(data_len <= RELAY_PAYLOAD_SIZE); + + /* If the offset is larger than the cell payload size, we return an offset + * of zero indicating that no padding needs to be added. */ + size_t offset = RELAY_HEADER_SIZE + data_len + 4; + if (offset >= CELL_PAYLOAD_SIZE) { + return 0; + } + return offset; +} + +/* Add random bytes to the unused portion of the payload, to foil attacks + * where the other side can predict all of the bytes in the payload and thus + * compute the authenticated SENDME cells without seeing the traffic. See + * proposal 289. */ +static void +pad_cell_payload(uint8_t *cell_payload, size_t data_len) +{ + size_t pad_offset, pad_len; + + tor_assert(cell_payload); + + pad_offset = get_pad_cell_offset(data_len); + if (pad_offset == 0) { + /* We can't add padding so we are done. */ + return; + } + + /* Remember here that the cell_payload is the length of the header and + * payload size so we offset it using the full lenght of the cell. */ + pad_len = CELL_PAYLOAD_SIZE - pad_offset; + crypto_fast_rng_getbytes(get_thread_fast_rng(), + cell_payload + pad_offset, pad_len); +} + /** Make a relay cell out of relay_command and payload, and send * it onto the open circuit circ. stream_id is the ID on * circ for the stream that's sending the relay cell, or 0 if it's a @@ -547,8 +601,6 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *circ, cell_t cell; relay_header_t rh; cell_direction_t cell_direction; - int random_bytes_len; - size_t random_bytes_offset = 0; /* XXXX NM Split this function into a separate versions per circuit type? */ tor_assert(circ); @@ -574,22 +626,8 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *circ, if (payload_len) memcpy(cell.payload+RELAY_HEADER_SIZE, payload, payload_len); - /* Add random bytes to the unused portion of the payload, to foil attacks - * where the other side can predict all of the bytes in the payload and thus - * compute authenticated sendme cells without seeing the traffic. See - * proposal 289. - * - * We'll skip the first 4 bytes of unused data because having some unused - * zero bytes has saved us a lot of times in the past. */ - random_bytes_len = RELAY_PAYLOAD_SIZE - - (RELAY_HEADER_SIZE + payload_len + 4); - if (random_bytes_len < 0) { - random_bytes_len = 0; - } - random_bytes_offset = RELAY_PAYLOAD_SIZE - random_bytes_len; - crypto_fast_rng_getbytes(get_thread_fast_rng(), - cell.payload + random_bytes_offset, - random_bytes_len); + /* Add random padding to the cell if we can. */ + pad_cell_payload(cell.payload, payload_len); log_debug(LD_OR,"delivering %d cell %s.", relay_command, cell_direction == CELL_DIRECTION_OUT ? "forward" : "backward"); diff --git a/src/core/or/relay.h b/src/core/or/relay.h index ea1b358ffb..2248cdf381 100644 --- a/src/core/or/relay.h +++ b/src/core/or/relay.h @@ -120,6 +120,7 @@ STATIC int cell_queues_check_size(void); STATIC int connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, edge_connection_t *conn, crypt_path_t *layer_hint); +STATIC size_t get_pad_cell_offset(size_t payload_len); #endif /* defined(RELAY_PRIVATE) */ diff --git a/src/test/test_sendme.c b/src/test/test_sendme.c index d6410a7488..50943e5f46 100644 --- a/src/test/test_sendme.c +++ b/src/test/test_sendme.c @@ -6,11 +6,13 @@ #define CIRCUITLIST_PRIVATE #define NETWORKSTATUS_PRIVATE #define SENDME_PRIVATE +#define RELAY_PRIVATE #include "core/or/circuit_st.h" #include "core/or/or_circuit_st.h" #include "core/or/origin_circuit_st.h" #include "core/or/circuitlist.h" +#include "core/or/relay.h" #include "core/or/sendme.h" #include "feature/nodelist/networkstatus.h" @@ -209,6 +211,48 @@ test_v1_build_cell(void *arg) circuit_free_(circ); } +static void +test_cell_payload_pad(void *arg) +{ + size_t pad_offset, payload_len, expected_offset; + + (void) arg; + + /* Offset should be 0, not enough room for padding. */ + payload_len = RELAY_PAYLOAD_SIZE; + pad_offset = get_pad_cell_offset(payload_len); + tt_int_op(pad_offset, OP_EQ, 0); + tt_int_op(CELL_PAYLOAD_SIZE - pad_offset, OP_LE, CELL_PAYLOAD_SIZE); + + /* Still no room because we keep 4 extra bytes. */ + pad_offset = get_pad_cell_offset(payload_len - 4); + tt_int_op(pad_offset, OP_EQ, 0); + tt_int_op(CELL_PAYLOAD_SIZE - pad_offset, OP_LE, CELL_PAYLOAD_SIZE); + + /* We should have 1 byte of padding. Meaning, the offset should be the + * CELL_PAYLOAD_SIZE minus 1 byte. */ + expected_offset = CELL_PAYLOAD_SIZE - 1; + pad_offset = get_pad_cell_offset(payload_len - 5); + tt_int_op(pad_offset, OP_EQ, expected_offset); + tt_int_op(CELL_PAYLOAD_SIZE - pad_offset, OP_LE, CELL_PAYLOAD_SIZE); + + /* Now some arbitrary small payload length. The cell size is header + 10 + + * extra 4 bytes we keep so the offset should be there. */ + expected_offset = RELAY_HEADER_SIZE + 10 + 4; + pad_offset = get_pad_cell_offset(10); + tt_int_op(pad_offset, OP_EQ, expected_offset); + tt_int_op(CELL_PAYLOAD_SIZE - pad_offset, OP_LE, CELL_PAYLOAD_SIZE); + + /* Data length of 0. */ + expected_offset = RELAY_HEADER_SIZE + 4; + pad_offset = get_pad_cell_offset(0); + tt_int_op(pad_offset, OP_EQ, expected_offset); + tt_int_op(CELL_PAYLOAD_SIZE - pad_offset, OP_LE, CELL_PAYLOAD_SIZE); + + done: + ; +} + struct testcase_t sendme_tests[] = { { "v1_note_digest", test_v1_note_digest, TT_FORK, NULL, NULL }, @@ -216,6 +260,8 @@ struct testcase_t sendme_tests[] = { NULL, NULL }, { "v1_build_cell", test_v1_build_cell, TT_FORK, NULL, NULL }, + { "cell_payload_pad", test_cell_payload_pad, TT_FORK, + NULL, NULL }, END_OF_TESTCASES }; From 67c22541830cf1dfc1c02843a1d4dd81c2df16ff Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 29 Apr 2019 11:29:05 -0400 Subject: [PATCH 27/32] sendme: Move note_cell_digest() to relay_crypto module Because this function is poking within the relay_crypto_t object, move the function to the module so we can keep it opaque as much as possible. Part of #26288 Signed-off-by: David Goulet --- src/core/crypto/relay_crypto.c | 9 +++++++++ src/core/crypto/relay_crypto.h | 2 ++ src/core/or/sendme.c | 14 +++----------- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/core/crypto/relay_crypto.c b/src/core/crypto/relay_crypto.c index eddc4298e2..3a6dad4b1d 100644 --- a/src/core/crypto/relay_crypto.c +++ b/src/core/crypto/relay_crypto.c @@ -91,6 +91,15 @@ relay_crypt_one_payload(crypto_cipher_t *cipher, uint8_t *in) crypto_cipher_crypt_inplace(cipher, (char*) in, CELL_PAYLOAD_SIZE); } +/** Record the b_digest from crypto and put it in the sendme_digest. */ +void +relay_crypto_record_sendme_digest(relay_crypto_t *crypto) +{ + tor_assert(crypto); + crypto_digest_get_digest(crypto->b_digest, (char *) crypto->sendme_digest, + sizeof(crypto->sendme_digest)); +} + /** Do the appropriate en/decryptions for cell arriving on * circ in direction cell_direction. * diff --git a/src/core/crypto/relay_crypto.h b/src/core/crypto/relay_crypto.h index 45a21d14ab..1009f1841b 100644 --- a/src/core/crypto/relay_crypto.h +++ b/src/core/crypto/relay_crypto.h @@ -27,5 +27,7 @@ void relay_crypto_clear(relay_crypto_t *crypto); void relay_crypto_assert_ok(const relay_crypto_t *crypto); +void relay_crypto_record_sendme_digest(relay_crypto_t *crypto); + #endif /* !defined(TOR_RELAY_CRYPTO_H) */ diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index b384a19164..b65f30ba97 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -12,6 +12,7 @@ #include "core/or/or.h" #include "app/config/config.h" +#include "core/crypto/relay_crypto.h" #include "core/mainloop/connection.h" #include "core/or/cell_st.h" #include "core/or/circuitlist.h" @@ -286,15 +287,6 @@ send_circuit_level_sendme(circuit_t *circ, crypt_path_t *layer_hint, return 0; } -/* Put the crypto.b_digest in the sendme_digest. */ -static void -note_cell_digest(const relay_crypto_t *crypto) -{ - tor_assert(crypto); - crypto_digest_get_digest(crypto->b_digest, (char *) crypto->sendme_digest, - sizeof(crypto->sendme_digest)); -} - /* * Public API */ @@ -305,7 +297,7 @@ void sendme_circuit_note_outbound_cell(or_circuit_t *or_circ) { tor_assert(or_circ); - note_cell_digest(&or_circ->crypto); + relay_crypto_record_sendme_digest(&or_circ->crypto); } /** Keep the current inbound cell digest for the next SENDME digest. This part @@ -314,7 +306,7 @@ void sendme_circuit_note_inbound_cell(crypt_path_t *cpath) { tor_assert(cpath); - note_cell_digest(&cpath->crypto); + relay_crypto_record_sendme_digest(&cpath->crypto); } /** Return true iff the next cell for the given cell window is expected to be From 0d8b9b56c5332b8f0205f460d0b23bb7f5620eff Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 29 Apr 2019 11:38:11 -0400 Subject: [PATCH 28/32] sendme: Better function names From nickm's review, improve the names of some functions. Part of #26288 Signed-off-by: David Goulet --- src/core/crypto/relay_crypto.c | 8 ++++---- src/core/or/relay.c | 2 +- src/core/or/sendme.c | 10 +++++----- src/core/or/sendme.h | 8 ++++---- src/test/test_sendme.c | 18 +++++++++--------- 5 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/core/crypto/relay_crypto.c b/src/core/crypto/relay_crypto.c index 3a6dad4b1d..5be45d6c7a 100644 --- a/src/core/crypto/relay_crypto.c +++ b/src/core/crypto/relay_crypto.c @@ -154,8 +154,8 @@ relay_decrypt_cell(circuit_t *circ, cell_t *cell, *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_is_next_cell(thishop->deliver_window)) { - sendme_circuit_note_inbound_cell(thishop); + if (sendme_circuit_cell_is_next(thishop->deliver_window)) { + sendme_circuit_record_inbound_cell(thishop); } return 0; } @@ -230,8 +230,8 @@ relay_encrypt_cell_inbound(cell_t *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_is_next_cell(TO_CIRCUIT(or_circ)->package_window)) { - sendme_circuit_note_outbound_cell(or_circ); + if (sendme_circuit_cell_is_next(TO_CIRCUIT(or_circ)->package_window)) { + sendme_circuit_record_outbound_cell(or_circ); } /* encrypt one layer */ diff --git a/src/core/or/relay.c b/src/core/or/relay.c index d273facd55..1b2aafb866 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -701,7 +701,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_note_cell_digest(circ); + sendme_record_cell_digest(circ); } return 0; diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index b65f30ba97..ff58c1489d 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -294,7 +294,7 @@ send_circuit_level_sendme(circuit_t *circ, crypt_path_t *layer_hint, /** 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_note_outbound_cell(or_circuit_t *or_circ) +sendme_circuit_record_outbound_cell(or_circuit_t *or_circ) { tor_assert(or_circ); relay_crypto_record_sendme_digest(&or_circ->crypto); @@ -303,7 +303,7 @@ sendme_circuit_note_outbound_cell(or_circuit_t *or_circ) /** 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_note_inbound_cell(crypt_path_t *cpath) +sendme_circuit_record_inbound_cell(crypt_path_t *cpath) { tor_assert(cpath); relay_crypto_record_sendme_digest(&cpath->crypto); @@ -316,7 +316,7 @@ sendme_circuit_note_inbound_cell(crypt_path_t *cpath) * one cell (the possible SENDME cell) should be a multiple of the increment * window value. */ bool -sendme_circuit_is_next_cell(int window) +sendme_circuit_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 @@ -578,7 +578,7 @@ sendme_note_stream_data_packaged(edge_connection_t *conn) /* 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. */ void -sendme_note_cell_digest(circuit_t *circ) +sendme_record_cell_digest(circuit_t *circ) { const uint8_t *digest; @@ -592,7 +592,7 @@ sendme_note_cell_digest(circuit_t *circ) /* 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_is_next_cell(circ->package_window)) { + if (!sendme_circuit_cell_is_next(circ->package_window)) { return; } diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index 5889b41e9c..78273eb9a8 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -35,12 +35,12 @@ int sendme_note_circuit_data_packaged(circuit_t *circ, int sendme_note_stream_data_packaged(edge_connection_t *conn); /* Track cell digest. */ -void sendme_note_cell_digest(circuit_t *circ); -void sendme_circuit_note_inbound_cell(crypt_path_t *cpath); -void sendme_circuit_note_outbound_cell(or_circuit_t *or_circ); +void sendme_record_cell_digest(circuit_t *circ); +void sendme_circuit_record_inbound_cell(crypt_path_t *cpath); +void sendme_circuit_record_outbound_cell(or_circuit_t *or_circ); /* Circuit level information. */ -bool sendme_circuit_is_next_cell(int window); +bool sendme_circuit_cell_is_next(int window); /* Private section starts. */ #ifdef SENDME_PRIVATE diff --git a/src/test/test_sendme.c b/src/test/test_sendme.c index 50943e5f46..d40fbaf862 100644 --- a/src/test/test_sendme.c +++ b/src/test/test_sendme.c @@ -43,7 +43,7 @@ free_mock_consensus(void) } static void -test_v1_note_digest(void *arg) +test_v1_record_digest(void *arg) { or_circuit_t *or_circ = NULL; origin_circuit_t *orig_circ = NULL; @@ -61,7 +61,7 @@ test_v1_note_digest(void *arg) circ->purpose = CIRCUIT_PURPOSE_S_REND_JOINED; /* We should never note SENDME digest on origin circuit. */ - sendme_note_cell_digest(circ); + sendme_record_cell_digest(circ); tt_assert(!circ->sendme_last_digests); /* We do not need the origin circuit for now. */ orig_circ = NULL; @@ -73,23 +73,23 @@ test_v1_note_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_note_cell_digest(circ); + sendme_record_cell_digest(circ); tt_assert(!circ->sendme_last_digests); /* This should work now. Package window at CIRCWINDOW_INCREMENT + 1. */ circ->package_window++; - sendme_note_cell_digest(circ); + sendme_record_cell_digest(circ); 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_note_cell_digest(circ); + sendme_record_cell_digest(circ); 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_note_cell_digest(circ); + sendme_record_cell_digest(circ); tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 2); done: @@ -188,7 +188,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_note_cell_digest(circ); + sendme_record_cell_digest(circ); 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 +200,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_note_cell_digest(circ); + sendme_record_cell_digest(circ); 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. */ @@ -254,7 +254,7 @@ test_cell_payload_pad(void *arg) } struct testcase_t sendme_tests[] = { - { "v1_note_digest", test_v1_note_digest, TT_FORK, + { "v1_record_digest", test_v1_record_digest, TT_FORK, NULL, NULL }, { "v1_consensus_params", test_v1_consensus_params, TT_FORK, NULL, NULL }, From 0f2ff267c5b509d697882989341d91b9fb4c249d Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 29 Apr 2019 12:11:57 -0400 Subject: [PATCH 29/32] sendme: Do not poke at crypto.sendme_digest directly As per review from nickm, keep as much as we can the relay_crypto_t object opaque. Part of #26288 Signed-off-by: David Goulet --- src/core/crypto/relay_crypto.c | 8 ++++++++ src/core/crypto/relay_crypto.h | 1 + src/core/or/sendme.c | 6 +++--- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/core/crypto/relay_crypto.c b/src/core/crypto/relay_crypto.c index 5be45d6c7a..8931163161 100644 --- a/src/core/crypto/relay_crypto.c +++ b/src/core/crypto/relay_crypto.c @@ -91,6 +91,14 @@ relay_crypt_one_payload(crypto_cipher_t *cipher, uint8_t *in) crypto_cipher_crypt_inplace(cipher, (char*) in, CELL_PAYLOAD_SIZE); } +/** Return the sendme_digest within the crypto object. */ +uint8_t * +relay_crypto_get_sendme_digest(relay_crypto_t *crypto) +{ + tor_assert(crypto); + return crypto->sendme_digest; +} + /** Record the b_digest from crypto and put it in the sendme_digest. */ void relay_crypto_record_sendme_digest(relay_crypto_t *crypto) diff --git a/src/core/crypto/relay_crypto.h b/src/core/crypto/relay_crypto.h index 1009f1841b..bcc1531838 100644 --- a/src/core/crypto/relay_crypto.h +++ b/src/core/crypto/relay_crypto.h @@ -27,6 +27,7 @@ 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); #endif /* !defined(TOR_RELAY_CRYPTO_H) */ diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index ff58c1489d..6f451d38e6 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -388,10 +388,10 @@ sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint) log_debug(LD_CIRC,"Queuing circuit sendme."); if (layer_hint) { layer_hint->deliver_window += CIRCWINDOW_INCREMENT; - digest = layer_hint->crypto.sendme_digest; + digest = relay_crypto_get_sendme_digest(&layer_hint->crypto); } else { circ->deliver_window += CIRCWINDOW_INCREMENT; - digest = TO_OR_CIRCUIT(circ)->crypto.sendme_digest; + digest = relay_crypto_get_sendme_digest(&TO_OR_CIRCUIT(circ)->crypto); } if (send_circuit_level_sendme(circ, layer_hint, digest) < 0) { return; /* The circuit's closed, don't continue */ @@ -597,7 +597,7 @@ sendme_record_cell_digest(circuit_t *circ) } /* Add the digest to the last seen list in the circuit. */ - digest = TO_OR_CIRCUIT(circ)->crypto.sendme_digest; + digest = relay_crypto_get_sendme_digest(&TO_OR_CIRCUIT(circ)->crypto); if (circ->sendme_last_digests == NULL) { circ->sendme_last_digests = smartlist_new(); } From 535ba0d7c58c681af8251d0133b33dc5f787fb2f Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 29 Apr 2019 12:27:53 -0400 Subject: [PATCH 30/32] practracker: Update exceptions for #26288 Signed-off-by: David Goulet --- scripts/maint/practracker/exceptions.txt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 37f11bb440..f8bb2bd379 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -85,7 +85,7 @@ problem function-size /src/core/or/circuitbuild.c:circuit_extend() 147 problem function-size /src/core/or/circuitbuild.c:choose_good_exit_server_general() 206 problem include-count /src/core/or/circuitlist.c 54 problem function-size /src/core/or/circuitlist.c:HT_PROTOTYPE() 128 -problem function-size /src/core/or/circuitlist.c:circuit_free_() 137 +problem function-size /src/core/or/circuitlist.c:circuit_free_() 143 problem function-size /src/core/or/circuitlist.c:circuit_find_to_cannibalize() 102 problem function-size /src/core/or/circuitlist.c:circuit_about_to_free() 120 problem function-size /src/core/or/circuitlist.c:circuits_handle_oom() 117 @@ -102,8 +102,8 @@ problem function-size /src/core/or/circuituse.c:circuit_get_open_circ_or_launch( problem function-size /src/core/or/circuituse.c:connection_ap_handshake_attach_circuit() 244 problem function-size /src/core/or/command.c:command_process_create_cell() 156 problem function-size /src/core/or/command.c:command_process_relay_cell() 132 -problem file-size /src/core/or/connection_edge.c 4575 -problem include-count /src/core/or/connection_edge.c 64 +problem file-size /src/core/or/connection_edge.c 4595 +problem include-count /src/core/or/connection_edge.c 65 problem function-size /src/core/or/connection_edge.c:connection_ap_expire_beginning() 117 problem function-size /src/core/or/connection_edge.c:connection_ap_handshake_rewrite() 192 problem function-size /src/core/or/connection_edge.c:connection_ap_handle_onion() 188 @@ -122,11 +122,11 @@ problem function-size /src/core/or/policies.c:policy_summarize() 107 problem function-size /src/core/or/protover.c:protover_all_supported() 116 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_() 101 +problem function-size /src/core/or/relay.c:relay_send_command_from_edge_() 112 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 -problem function-size /src/core/or/relay.c:connection_edge_package_raw_inbuf() 130 +problem function-size /src/core/or/relay.c:connection_edge_package_raw_inbuf() 132 problem function-size /src/core/or/relay.c:circuit_resume_edge_reading_helper() 148 problem function-size /src/core/or/scheduler_kist.c:kist_scheduler_run() 171 problem function-size /src/core/or/scheduler_vanilla.c:vanilla_scheduler_run() 109 From 77bd219808ac82c231aef37672e7fb212cd83d15 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 2 May 2019 08:58:58 -0400 Subject: [PATCH 31/32] sendme: Improve logging messages Signed-off-by: David Goulet --- src/core/or/sendme.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index 6f451d38e6..70ff3798ba 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -129,8 +129,8 @@ cell_version_is_valid(uint8_t cell_version) /* Can we handle this version? */ if (accept_version > SENDME_MAX_SUPPORTED_VERSION) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, - "Unable to handle SENDME version %u. We only support <= %d " - "(from consensus). Probably your tor is too old?", + "Unable to accept SENDME version %u (from consensus). " + "We only support <= %d. Probably your tor is too old?", accept_version, cell_version); goto invalid; } @@ -138,8 +138,7 @@ cell_version_is_valid(uint8_t cell_version) /* We only accept a SENDME cell from what the consensus tells us. */ if (cell_version < accept_version) { log_info(LD_PROTOCOL, "Unacceptable SENDME version %d. Only " - "accepting %u (taken from the consensus). " - "Closing circuit.", + "accepting %u (from consensus). Closing circuit.", cell_version, accept_version); goto invalid; } From 562bcbcfc216df9f25cd72f1e227b9313c872172 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 2 May 2019 11:10:41 -0400 Subject: [PATCH 32/32] sendme: Add changes file for prop289 Signed-off-by: David Goulet --- changes/ticket26288 | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changes/ticket26288 diff --git a/changes/ticket26288 b/changes/ticket26288 new file mode 100644 index 0000000000..59bb856dd2 --- /dev/null +++ b/changes/ticket26288 @@ -0,0 +1,6 @@ + o Major features (flow control): + - Implement authenticated SENDMEs detailed in proposal 289. A SENDME cell + now includes the digest of the last cell received so once the end point + receives the SENDME, it can confirm the other side's knowledge of the + previous cells that were sent. This behavior is controlled by two new + consensus parameters, see proposal for more details. Fixes ticket 26288.