tor

commit ffa7b15950a3274a03b0957425bcaa9952213046
parent e2d3c9c5f82a1369385dd99765c31ba479ba8f23
Author: Nick Mathewson <nickm@torproject.org>
Date:   Thu, 12 Nov 2020 11:07:33 -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.

Diffstat:
A
changes/bug40017
 | 
5
+++++
M
src/core/or/connection_or.c
 | 
28
+++++++++++++++-------------

2 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/changes/bug40017 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
@@ -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)));