Rejecting risky extend cells?

[Roger Dingledine <arma@xxxxxxx>]

Hi Nick, others,

Does this need a proposal, or is it a clearly good patch? Ideas
suggested by rovv.

--Roger

Index: src/or/circuitbuild.c
===================================================================
--- src/or/circuitbuild.c	(revision 16587)
+++ src/or/circuitbuild.c	(working copy)
@@ -712,10 +712,13 @@
   circuit_expire_all_dirty_circs();
 }
 
-/** Take the 'extend' cell, pull out addr/port plus the onion skin. Make
- * sure we're connected to the next hop, and pass it the onion skin using
- * a create cell. Return -1 if we want to warn and tear down the circuit,
- * else return 0.
+/** Take the 'extend' <b>cell</b>, pull out addr/port plus the onion
+ * skin and identity digest for the next hop. If we're already connected,
+ * pass the onion skin to the next hop using a create cell; otherwise
+ * launch a new OR connection, and <b>circ</b> will notice when the
+ * connection succeeds or fails.
+ *
+ * Return -1 if we want to warn and tear down the circuit, else return 0.
  */
 int
 circuit_extend(cell_t *cell, circuit_t *circ)
@@ -753,6 +756,28 @@
   onionskin = cell->payload+RELAY_HEADER_SIZE+4+2;
   id_digest = cell->payload+RELAY_HEADER_SIZE+4+2+ONIONSKIN_CHALLENGE_LEN;
 
+  /* First, check if they asked us for 0000..0000. We support using
+   * an empty fingerprint for the first hop (e.g. for a bridge relay),
+   * but we don't want to let people send us extend cells for empty
+   * fingerprints -- a) because it opens the user up to a mitm attack,
+   * and b) because it lets an attacker force the relay to hold open a
+   * new TLS connection for each extend request. */
+  if (tor_digest_is_zero(id_digest)) {
+    log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
+           "Client asked me to extend without specifying an id_digest.");
+    return -1;
+  }
+
+  /* Next, check if we're being asked to connect to the hop that the
+   * extend cell came from. There isn't any reason for that, and it can
+   * assist circular-path attacks. */
+  if (!memcmp(id_digest, TO_OR_CIRCUIT(circ)->p_conn->identity_digest,
+              DIGEST_LEN)) {
+    log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
+           "Client asked me to extend back to the previous hop.");
+    return -1;
+  }
+
   n_conn = connection_or_get_by_identity_digest(id_digest);
 
   /* If we don't have an open conn, or the conn we have is obsolete
Index: doc/spec/tor-spec.txt
===================================================================
--- doc/spec/tor-spec.txt	(revision 16587)
+++ doc/spec/tor-spec.txt	(working copy)
@@ -398,9 +398,9 @@
    The port and address field denote the IPV4 address and port of the next
    onion router in the circuit; the public key hash is the hash of the PKCS#1
    ASN1 encoding of the next onion router's identity (signing) key.  (See 0.3
-   above.)  (Including this hash allows the extending OR verify that it is
+   above.)  Including this hash allows the extending OR verify that it is
    indeed connected to the correct target OR, and prevents certain
-   man-in-the-middle attacks.)
+   man-in-the-middle attacks.
 
    The payload for a CREATED cell, or the relay payload for an
    EXTENDED cell, contains:
@@ -525,10 +525,12 @@
 
    When an onion router receives an EXTEND relay cell, it sends a CREATE
    cell to the next onion router, with the enclosed onion skin as its
-   payload.  The initiating onion router chooses some circID not yet
-   used on the connection between the two onion routers.  (But see
-   section 5.1. above, concerning choosing circIDs based on
-   lexicographic order of nicknames.)
+   payload.  As special cases, if the extend cell includes a digest of
+   all zeroes, or asks to extend back to the relay that sent the extend
+   cell, the circuit will fail and be torn down. The initiating onion
+   router chooses some circID not yet used on the connection between the
+   two onion routers.  (But see section 5.1. above, concerning choosing
+   circIDs based on lexicographic order of nicknames.)
 
    When an onion router receives a CREATE cell, if it already has a
    circuit on the given connection with the given circID, it drops the