From 3c97ab3c24ba4a133377c7ec6ec89cc6903ffb2e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Sep 2019 09:09:36 -0400 Subject: [PATCH 1/3] Treat an unexpected constant-sized VERSIONS cell as a PROTOCOL_WARN. We previously used tor_fragile_assert() to declare that this case could not happen: VERSIONS cells are always supposed to be variable-sized, right? This is incorrect, though. On a v1 link protocol connection, all cells are fixed-sized. There aren't supposed to be any VERSIONS cells with this version of the protocol, but apparently, somebody was messing up. (The v1 link protocol is obsolete, so probably the implementer responsible didn't mean to be using it.) Fixes bug 31107. Bugfix on 0.2.4.4-alpha, when we introduced a tor_fragile_assert() for this case. --- changes/bug31107 | 4 ++++ src/or/channeltls.c | 10 +++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 changes/bug31107 diff --git a/changes/bug31107 b/changes/bug31107 new file mode 100644 index 0000000000..9652927c30 --- /dev/null +++ b/changes/bug31107 @@ -0,0 +1,4 @@ + o Minor bugfixes (logging, protocol violations): + - Do not log a nonfatal assertion failure when receiving a VERSIONS + cell on a connection using the obsolete v1 link protocol. Log a + protocol_warn instead. Fixes bug 31107; bugfix on 0.2.4.4-alpha. diff --git a/src/or/channeltls.c b/src/or/channeltls.c index d44f719138..6f4e413dc6 100644 --- a/src/or/channeltls.c +++ b/src/or/channeltls.c @@ -1098,7 +1098,15 @@ channel_tls_handle_cell(cell_t *cell, or_connection_t *conn) /* do nothing */ break; case CELL_VERSIONS: - tor_fragile_assert(); + /* A VERSIONS cell should always be a variable-length cell, and + * so should never reach this function (which handles constant-sized + * cells). But if the connection is using the (obsolete) v1 link + * protocol, all cells will be treated as constant-sized, and so + * it's possible we'll reach this code. + */ + log_fn(LOG_PROTOCOL_WARN, LD_CHANNEL, + "Received unexpected VERSIONS cell on a channel using link " + "protocol %d; ignoring.", conn->link_proto); break; case CELL_NETINFO: ++stats_n_netinfo_cells_processed; From 7f5c6a603cafff9870f487bc03fa2874dcf00f8d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Sep 2019 11:40:14 -0400 Subject: [PATCH 2/3] Move PROCESS_CELL macros outside of channel_tls_process_cell(). These macros are used in multiple functions, and as such really don't belong within a single function. Also #undef them once we are done with them. This change makes practracker pass again. --- src/core/or/channeltls.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/core/or/channeltls.c b/src/core/or/channeltls.c index 25f3baf924..2bc2cba313 100644 --- a/src/core/or/channeltls.c +++ b/src/core/or/channeltls.c @@ -1027,6 +1027,16 @@ channel_tls_time_process_cell(cell_t *cell, channel_tls_t *chan, int *time, } #endif /* defined(KEEP_TIMING_STATS) */ +#ifdef KEEP_TIMING_STATS +#define PROCESS_CELL(tp, cl, cn) STMT_BEGIN { \ + ++num ## tp; \ + channel_tls_time_process_cell(cl, cn, & tp ## time , \ + channel_tls_process_ ## tp ## _cell); \ + } STMT_END +#else /* !(defined(KEEP_TIMING_STATS)) */ +#define PROCESS_CELL(tp, cl, cn) channel_tls_process_ ## tp ## _cell(cl, cn) +#endif /* defined(KEEP_TIMING_STATS) */ + /** * Handle an incoming cell on a channel_tls_t. * @@ -1046,16 +1056,6 @@ channel_tls_handle_cell(cell_t *cell, or_connection_t *conn) channel_tls_t *chan; int handshaking; -#ifdef KEEP_TIMING_STATS -#define PROCESS_CELL(tp, cl, cn) STMT_BEGIN { \ - ++num ## tp; \ - channel_tls_time_process_cell(cl, cn, & tp ## time , \ - channel_tls_process_ ## tp ## _cell); \ - } STMT_END -#else /* !(defined(KEEP_TIMING_STATS)) */ -#define PROCESS_CELL(tp, cl, cn) channel_tls_process_ ## tp ## _cell(cl, cn) -#endif /* defined(KEEP_TIMING_STATS) */ - tor_assert(cell); tor_assert(conn); @@ -1328,6 +1328,8 @@ channel_tls_handle_var_cell(var_cell_t *var_cell, or_connection_t *conn) } } +#undef PROCESS_CELL + /** * Update channel marks after connection_or.c has changed an address. * From 2d744de355d65c8f13c721558025bbfbfaa0758e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Sep 2019 11:42:19 -0400 Subject: [PATCH 3/3] Correct a comment in channel_tls_process_cell. --- src/core/or/channeltls.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/or/channeltls.c b/src/core/or/channeltls.c index 2bc2cba313..f727799387 100644 --- a/src/core/or/channeltls.c +++ b/src/core/or/channeltls.c @@ -1073,7 +1073,8 @@ channel_tls_handle_cell(cell_t *cell, or_connection_t *conn) return; /* Reject all but VERSIONS and NETINFO when handshaking. */ - /* (VERSIONS should actually be impossible; it's variable-length.) */ + /* (VERSIONS actually indicates a protocol warning: it's variable-length, + * so if it reaches this function, we're on a v1 connection.) */ if (handshaking && cell->command != CELL_VERSIONS && cell->command != CELL_NETINFO) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,