backport r16605: relays reject risky extend cells

svn:r16728

3 files changed, 36 insertions, 6 deletions

diff --git a/ChangeLog b/ChangeLog
index 9c072952ea..54c6008be0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,8 +1,12 @@
-Changes in version 0.2.0.31 - 2008-08-??
+Changes in version 0.2.0.31 - 2008-09-??
  o Major bugfixes:
     - Make sure that two circuits can never exist on the same connection
       with the same circuit ID, even if one is marked for close.  This
       is conceivably a bugfix for bug 779; fixes a bug on 0.1.0.4-rc.
+    - 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:
     - Fix a small alignment and memory-wasting bug on buffer chunks.  Spotted
diff --git a/doc/TODO.020 b/doc/TODO.020
index d87db8c940..a5c9669894 100644
--- a/doc/TODO.020
+++ b/doc/TODO.020
@@ -12,6 +12,6 @@ Backport for 0.2.0 once better tested:
   o r16136: prevent circid collision.  [Also backport to 0.1.2.x??]
   - r16143: generate stream close events from connection_edge_destroy().
   o r16450: open /dev/pf before dropping privileges.
-  - r16605: relays reject risky extend cells.
+  o r16605: relays reject risky extend cells.
   - r16698: don't use a new entry guard that's also your exit.
 
diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 3be2689995..b121c6e196 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -705,10 +705,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)
@@ -744,6 +747,29 @@ 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