Don't extend circuits over noncanonical connections with mismatched addresses.

Also, refactor the logic to check whether we will use a connection or
launch a new one into a new function.

svn:r17628

ChangeLog

@@ -1,4 +1,10 @@
 Changes in version 0.2.1.9-alpha - 200?-??-??
+  o Major features:
+    - Never use a connection with a mismatched address to extend a
+      circuit, unless that connections is canonical.  A canonical
+      connection is one whose address is authenticated by the router's
+      identity key, either in a NETINFO cell or in a router descriptor.
+
   o Major bugfixes:
     - Fix a logic error that would automatically reject all but the first
       configured DNS server.  Bugfix on 0.2.1.5-alpha.  Possible fix for part

doc/TODO.021

@@ -27,7 +27,7 @@ Things Roger would be excited to see:
 Nick
   * Look at Roger's proposal 141 discussions on or-dev, and help us
     decide how to proceed.
-  - Tors start believing the contents of NETINFO cells.
+  . Tors start believing the contents of NETINFO cells.
   - respond to Steven's red-team TLS testing (a.k.a, look at a packet
     dump and compare)
 
@@ -166,10 +166,10 @@ K   o 155: Four Improvements of Hidden Service Performance
     - 145: Separate "suitable from a guard" from "suitable as a new guard"
     - 146: Adding new flag to reflect long-term stability
     - 149: Using data from NETINFO cells
-      * Don't extend a circuit over a noncanonical connection with
+      o Don't extend a circuit over a noncanonical connection with
         mismatched address.
         o Apply rovv's bugfixes wrt preferring canonical connections.
-        - Make sure that having a non-canonical connection doesn't count
+        o Make sure that having a non-canonical connection doesn't count
           as _having_ a connection for the purpose of connecting to others,
           and that when no canonical connection exists, we make one.
       - Learn our outgoing IP address from netinfo cells?
@@ -282,6 +282,8 @@ P   - Figure out why dll's compiled in mingw don't work right in WinXP.
 P   - create a "make win32-bundle" for vidalia-privoxy-tor-torbutton bundle
 
   - Refactor bad code:
+    - connection_or_get_by_identity_digest() and connection_good_enough_for
+      _extend() could be merged into a smarter variant, perhaps.
     - Refactor the HTTP logic so the functions aren't so large.
     - Refactor buf_read and buf_write to have sensible ways to return
       error codes after partial writes

src/or/circuitbuild.c

@@ -334,6 +334,46 @@ circuit_establish_circuit(uint8_t purpose, extend_info_t *exit, int flags)
   return circ;
 }
 
+/** Return true iff <b>n_conn</b> (a connection with a desired identity), is
+ * an acceptable choice for extending or launching a circuit to the address
+ * <b>target_addr</b>.  If it is not, set <b>state_out</b> to a message
+ * describing the connection's state and our next action, and set
+ * <b>launch_out</b> to a boolean for whether we should launch a new
+ * connection or not. */
+static int
+connection_good_enough_for_extend(const or_connection_t *n_conn,
+                                  const tor_addr_t *target_addr,
+                                  const char **state_out,
+                                  int *launch_out)
+{
+  tor_assert(state_out);
+  tor_assert(launch_out);
+  tor_assert(target_addr);
+
+  if (!n_conn) {
+    *state_out = "not connected. Connecting.";
+    *launch_out = 1;
+    return 0;
+  } else if (n_conn->_base.state != OR_CONN_STATE_OPEN) {
+    *state_out = "in progress. Waiting.";
+    *launch_out = 0; /* We'll just wait till the connection finishes. */
+    return 0;
+  } else if (n_conn->_base.or_is_obsolete) {
+    *state_out = "too old. Launching a new one.";
+    *launch_out = 1;
+    return 0;
+  } else if (tor_addr_compare(&n_conn->_base.addr, target_addr, CMP_EXACT) &&
+             ! n_conn->is_canonical) {
+    *state_out = "is not from a canonical address. Launching a new one.";
+    *launch_out = 1;
+    return 0;
+  } else {
+    *state_out = "is fine; using it.";
+    *launch_out = 0;
+    return 1;
+  }
+}
+
 /** Start establishing the first hop of our circuit. Figure out what
  * OR we should connect to, and if necessary start the connection to
  * it. If we're already connected, then send the 'create' cell.
@@ -344,6 +384,8 @@ circuit_handle_first_hop(origin_circuit_t *circ)
   crypt_path_t *firsthop;
   or_connection_t *n_conn;
   int err_reason = 0;
+  const char *msg = NULL;
+  int should_launch = 0;
 
   firsthop = onion_next_hop_in_cpath(circ->cpath);
   tor_assert(firsthop);
@@ -356,15 +398,17 @@ circuit_handle_first_hop(origin_circuit_t *circ)
 
   n_conn = connection_or_get_by_identity_digest(
          firsthop->extend_info->identity_digest);
+
   /* If we don't have an open conn, or the conn we have is obsolete
    * (i.e. old or broken) and the other side will let us make a second
    * connection without dropping it immediately... */
-  if (!n_conn || n_conn->_base.state != OR_CONN_STATE_OPEN ||
+  if (!connection_good_enough_for_extend(n_conn, &firsthop->extend_info->addr,
-      (n_conn->_base.or_is_obsolete)) {
+                                         &msg, &should_launch)) {
+    /* XXXX021 log msg, maybe. */
     /* not currently connected */
     circ->_base.n_hop = extend_info_dup(firsthop->extend_info);
 
-    if (!n_conn || n_conn->_base.or_is_obsolete) { /* launch the connection */
+    if (should_launch) {
       if (circ->build_state->onehop_tunnel)
         control_event_bootstrap(BOOTSTRAP_STATUS_CONN_DIR, 0);
       n_conn = connection_or_connect(&firsthop->extend_info->addr,
@@ -723,8 +767,11 @@ circuit_extend(cell_t *cell, circuit_t *circ)
   relay_header_t rh;
   char *onionskin;
   char *id_digest=NULL;
-  uint32_t n_addr;
+  uint32_t n_addr32;
   uint16_t n_port;
+  tor_addr_t n_addr;
+  const char *msg = NULL;
+  int should_launch = 0;
 
   if (circ->n_conn) {
     log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
@@ -747,10 +794,11 @@ circuit_extend(cell_t *cell, circuit_t *circ)
     return -1;
   }
 
-  n_addr = ntohl(get_uint32(cell->payload+RELAY_HEADER_SIZE));
+  n_addr32 = ntohl(get_uint32(cell->payload+RELAY_HEADER_SIZE));
   n_port = ntohs(get_uint16(cell->payload+RELAY_HEADER_SIZE+4));
   onionskin = cell->payload+RELAY_HEADER_SIZE+4+2;
   id_digest = cell->payload+RELAY_HEADER_SIZE+4+2+ONIONSKIN_CHALLENGE_LEN;
+  tor_addr_from_ipv4h(&n_addr, n_addr32);
 
   /* 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),
@@ -779,26 +827,23 @@ circuit_extend(cell_t *cell, circuit_t *circ)
   /* If we don't have an open conn, or the conn we have is obsolete
    * (i.e. old or broken) and the other side will let us make a second
    * connection without dropping it immediately... */
-  if (!n_conn || n_conn->_base.state != OR_CONN_STATE_OPEN ||
+  if (!connection_good_enough_for_extend(n_conn, &n_addr, &msg,
-      n_conn->_base.or_is_obsolete) {
+                                         &should_launch)) {
-    tor_addr_t addr;
+    log_debug(LD_CIRC|LD_OR,"Next router (%s:%d) %s",
-    tor_addr_from_ipv4h(&addr, n_addr);
+              fmt_addr(&n_addr), (int)n_port, msg?msg:"????");
-
-    log_debug(LD_CIRC|LD_OR,"Next router (%s:%d) not connected. Connecting.",
-              fmt_addr(&addr), (int)n_port);
 
     circ->n_hop = extend_info_alloc(NULL /*nickname*/,
                                     id_digest,
                                     NULL /*onion_key*/,
-                                    &addr, n_port);
+                                    &n_addr, n_port);
 
     circ->n_conn_onionskin = tor_malloc(ONIONSKIN_CHALLENGE_LEN);
     memcpy(circ->n_conn_onionskin, onionskin, ONIONSKIN_CHALLENGE_LEN);
     circuit_set_state(circ, CIRCUIT_STATE_OR_WAIT);
 
-    if (! (n_conn && !n_conn->_base.or_is_obsolete)) {
+    if (should_launch) {
       /* we should try to open a connection */
-      n_conn = connection_or_connect(&addr, n_port, id_digest);
+      n_conn = connection_or_connect(&n_addr, n_port, id_digest);
       if (!n_conn) {
         log_info(LD_CIRC,"Launching n_conn failed. Closing circuit.");
         circuit_mark_for_close(circ, END_CIRC_REASON_CONNECTFAILED);