commit ffa7b15950a3274a03b0957425bcaa9952213046 Author: Nick Mathewson <nickm@torproject.org> Date: Thu Nov 12 11:07:33 2020 -0500 Deliberately close OR connections if proxies leave extra data We already did this, but we did it by accident, which is pretty risky: if we hadn't, then our code would have treated extra data in the inbuf as having been transmitted as TLS-authenticated data. Closes ticket 40017; Found by opara. --- changes/bug40017 | 5 +++++ src/core/or/connection_or.c | 28 +++++++++++++++------------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/changes/bug40017 b/changes/bug40017 new file mode 100644 index 0000000000..3f5c2da968 --- /dev/null +++ b/changes/bug40017 @@ -0,0 +1,5 @@ + o Minor features (protocol, proxy support, defense in depth): + - Respond more deliberately to misbehaving proxies that leave leftover + data on their connections, so as to be even less likely as to allow + them to pass their data off as having come from a relay. + Closes ticket 40017. diff --git a/src/core/or/connection_or.c b/src/core/or/connection_or.c index bf29cd2c3a..3be0b65f47 100644 --- a/src/core/or/connection_or.c +++ b/src/core/or/connection_or.c @@ -566,11 +566,6 @@ connection_or_reached_eof(or_connection_t *conn) int connection_or_process_inbuf(or_connection_t *conn) { - /** Don't let the inbuf of a nonopen OR connection grow beyond this many - * bytes: it's either a broken client, a non-Tor client, or a DOS - * attempt. */ -#define MAX_OR_INBUF_WHEN_NONOPEN 0 - int ret = 0; tor_assert(conn); @@ -581,6 +576,15 @@ connection_or_process_inbuf(or_connection_t *conn) /* start TLS after handshake completion, or deal with error */ if (ret == 1) { tor_assert(TO_CONN(conn)->proxy_state == PROXY_CONNECTED); + if (buf_datalen(conn->base_.inbuf) != 0) { + log_fn(LOG_PROTOCOL_WARN, LD_NET, "Found leftover (%d bytes) " + "when transitioning from PROXY_HANDSHAKING state on %s: " + "closing.", + (int)buf_datalen(conn->base_.inbuf), + connection_describe(TO_CONN(conn))); + connection_or_close_for_error(conn, 0); + return -1; + } if (connection_tls_start_handshake(conn, 0) < 0) ret = -1; /* Touch the channel's active timestamp if there is one */ @@ -601,14 +605,12 @@ connection_or_process_inbuf(or_connection_t *conn) break; /* don't do anything */ } - /* This check was necessary with 0.2.2, when the TLS_SERVER_RENEGOTIATING - * check would otherwise just let data accumulate. It serves no purpose - * in 0.2.3. - * - * XXXX Remove this check once we verify that the above paragraph is - * 100% true. */ - if (buf_datalen(conn->base_.inbuf) > MAX_OR_INBUF_WHEN_NONOPEN) { - log_fn(LOG_PROTOCOL_WARN, LD_NET, "Accumulated too much data (%d bytes) " + /* This check makes sure that we don't have any data on the inbuf if we're + * doing our TLS handshake: if we did, they were probably put there by a + * SOCKS proxy trying to trick us into accepting unauthenticated data. + */ + if (buf_datalen(conn->base_.inbuf) != 0) { + log_fn(LOG_PROTOCOL_WARN, LD_NET, "Accumulated data (%d bytes) " "on non-open %s; closing.", (int)buf_datalen(conn->base_.inbuf), connection_describe(TO_CONN(conn)));