Relays now reject risky extend cells: if the extend cell includes

a digest of all zeroes, or asks to extend back to the relay that
sent the extend cell, tear down the circuit. Ideas suggested
by rovv.


svn:r16605

ChangeLog

@@ -7,12 +7,16 @@ Changes in version 0.2.1.5-alpha - 2008-08-??
       hidden services): associate keys, client lists, and authorization
       types with hidden services.
 
-  o Major bugfixes:
+  o Major bugfixes (on 0.2.0.x and before):
     - When sending CREATED cells back for a given circuit, use a 64-bit
       connection ID to find the right connection, rather than an addr:port
       combination.  Now that we can have multiple OR connections between the
       same ORs, it is no longer possible to use addr:port to uniquely
       identify a connection.
+    - Relays now reject risky extend cells: if the extend cell includes
+      a digest of all zeroes, or asks to extend back to the relay that
+      sent the extend cell, tear down the circuit. Ideas suggested
+      by rovv.
 
   o Minor bugfixes:
     - Recover 3-7 bytes that were wasted per memory chunk.  Fixes bug

doc/spec/tor-spec.txt

@@ -398,9 +398,9 @@ see tor-design.pdf.
    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 @@ see tor-design.pdf.
 
    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

src/or/circuitbuild.c

@@ -712,10 +712,13 @@ circuit_note_clock_jumped(int seconds_elapsed)
   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 @@ circuit_extend(cell_t *cell, circuit_t *circ)
   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