[tor-commits] [tor/release-0.4.5] Deliberately close OR connections if proxies leave extra data

[dgoulet at torproject.org]

commit ffa7b15950a3274a03b0957425bcaa9952213046
Author: Nick Mathewson <nickm at 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)));