Merge remote-tracking branch 'rransom-tor/bug1297b-v2'

10 files changed, 211 insertions, 24 deletions

diff --git a/changes/bug1297b b/changes/bug1297b
new file mode 100644
index 000000000..fb0d00cab
--- /dev/null
+++ b/changes/bug1297b
@@ -0,0 +1,20 @@
+  o Minor bugfixes:
+
+    - Don't close hidden service client circuits which have almost
+      finished connecting to their destination when they reach the
+      normal circuit-build timeout.  Previously, we would close
+      introduction circuits which are waiting for an acknowledgement
+      from the introduction-point relay and rendezvous circuits which
+      have been specified in an INTRODUCE1 cell sent to a hidden
+      service after the normal CBT; now, we mark them as 'timed out',
+      and launch another rendezvous attempt in parallel.  This
+      behaviour change can be disabled using the new
+      CloseHSClientCircuitsImmediatelyOnTimeout option.  Fixes part of
+      bug 1297.
+
+    - Don't close hidden-service-side rendezvous circuits when they
+      reach the normal circuit-build timeout.  Previously, we would
+      close them.  This behaviour change can be disabled using the new
+      CloseHSServiceRendCircuitsImmediatelyOnTimeout option.  Fixes
+      the remaining part of bug 1297.
+
diff --git a/changes/bug4759 b/changes/bug4759
new file mode 100644
index 000000000..19138ab8b
--- /dev/null
+++ b/changes/bug4759
@@ -0,0 +1,14 @@
+  o Minor bugfixes:
+
+    - Make sure we never mark the wrong rendezvous circuit as having
+      had its introduction cell acknowleged by the introduction-point
+      relay.  Previously, when we received an INTRODUCE_ACK cell on a
+      client-side hidden-service introduction circuit, we might have
+      marked a rendezvous circuit other than the one we specified in
+      the INTRODUCE1 cell as INTRO_ACKED, which would have produced a
+      warning message and interfered with the hidden service
+      connection-establishment process.  Bugfix on 0.2.3.3-alpha, when
+      the stream-isolation feature which might cause Tor to open
+      multiple rendezvous circuits for the same hidden service was
+      added.  Fixes bug 4759.
+
diff --git a/doc/tor.1.txt b/doc/tor.1.txt
index 56621315e..91a7c69a5 100644
--- a/doc/tor.1.txt
+++ b/doc/tor.1.txt
@@ -644,16 +644,6 @@ The following options are useful only for clients (that is, if
     **FascistFirewall** is set. This option is deprecated; use ReachableAddresses
     instead. (Default: 80, 443)
 
-**HidServAuth** __onion-address__ __auth-cookie__ [__service-name__]::
-    Client authorization for a hidden service. Valid onion addresses contain 16
-    characters in a-z2-7 plus ".onion", and valid auth cookies contain 22
-    characters in A-Za-z0-9+/. The service name is only used for internal
-    purposes, e.g., for Tor controllers. This option may be used multiple times
-    for different hidden services. If a hidden service uses authorization and
-    this option is not set, the hidden service is not accessible. Hidden
-    services can be configured to require authorization using the 
-    **HiddenServiceAuthorizeClient** option.
-
 **ReachableAddresses** __ADDR__[/__MASK__][:__PORT__]...::
     A comma-separated list of IP addresses and ports that your firewall allows
     you to connect to. The format is as for the addresses in ExitPolicy, except
@@ -683,6 +673,34 @@ The following options are useful only for clients (that is, if
     and some limit HTTP GET requests (which Tor uses for fetching directory
     information) to port 80.
 
+**HidServAuth** __onion-address__ __auth-cookie__ [__service-name__]::
+    Client authorization for a hidden service. Valid onion addresses contain 16
+    characters in a-z2-7 plus ".onion", and valid auth cookies contain 22
+    characters in A-Za-z0-9+/. The service name is only used for internal
+    purposes, e.g., for Tor controllers. This option may be used multiple times
+    for different hidden services. If a hidden service uses authorization and
+    this option is not set, the hidden service is not accessible. Hidden
+    services can be configured to require authorization using the 
+    **HiddenServiceAuthorizeClient** option.
+
+**CloseHSClientCircuitsImmediatelyOnTimeout** **0**|**1**::
+    If 1, Tor will close unfinished hidden service client circuits
+    which have not moved closer to connecting to their destination
+    hidden service when their internal state has not changed for the
+    duration of the current circuit-build timeout.  Otherwise, such
+    circuits will be left open, in the hope that they will finish
+    connecting to their destination hidden services.  In either case,
+    another set of introduction and rendezvous circuits for the same
+    destination hidden service will be launched. (Default: 0)
+
+**CloseHSServiceRendCircuitsImmediatelyOnTimeout** **0**|**1**::
+    If 1, Tor will close unfinished hidden-service-side rendezvous
+    circuits after the current circuit-build timeout.  Otherwise, such
+    circuits will be left open, in the hope that they will finish
+    connecting to their destinations.  In either case, another
+    rendezvous circuit for the same destination client will be
+    launched. (Default: 0)
+
 **LongLivedPorts** __PORTS__::
     A list of ports for services that tend to have long-running connections
     (e.g. chat and interactive shells). Circuits for streams that use these
diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c
index 4fa188830..97b616260 100644
--- a/src/or/circuitlist.c
+++ b/src/or/circuitlist.c
@@ -930,26 +930,30 @@ circuit_unlink_all_from_or_conn(or_connection_t *conn, int reason)
   }
 }
 
-/** Return a circ such that:
- *  - circ-\>rend_data-\>onion_address is equal to <b>rend_query</b>, and
- *  - circ-\>purpose is equal to <b>purpose</b>.
+/** Return a circ such that
+ *  - circ-\>rend_data-\>onion_address is equal to
+ *    <b>rend_data</b>-\>onion_address,
+ *  - circ-\>rend_data-\>rend_cookie is equal to
+ *    <b>rend_data</b>-\>rend_cookie, and
+ *  - circ-\>purpose is equal to CIRCUIT_PURPOSE_C_REND_READY.
  *
  * Return NULL if no such circuit exists.
  */
 origin_circuit_t *
-circuit_get_by_rend_query_and_purpose(const char *rend_query, uint8_t purpose)
+circuit_get_ready_rend_circ_by_rend_data(const rend_data_t *rend_data)
 {
   circuit_t *circ;
 
-  tor_assert(CIRCUIT_PURPOSE_IS_ORIGIN(purpose));
-
   for (circ = global_circuitlist; circ; circ = circ->next) {
     if (!circ->marked_for_close &&
-        circ->purpose == purpose) {
+        circ->purpose == CIRCUIT_PURPOSE_C_REND_READY) {
       origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
       if (ocirc->rend_data &&
-          !rend_cmp_service_ids(rend_query,
-                                ocirc->rend_data->onion_address))
+          !rend_cmp_service_ids(rend_data->onion_address,
+                                ocirc->rend_data->onion_address) &&
+          tor_memeq(ocirc->rend_data->rend_cookie,
+                    rend_data->rend_cookie,
+                    REND_COOKIE_LEN))
         return ocirc;
     }
   }
diff --git a/src/or/circuitlist.h b/src/or/circuitlist.h
index 8ed089c81..e2298c69a 100644
--- a/src/or/circuitlist.h
+++ b/src/or/circuitlist.h
@@ -33,8 +33,8 @@ int circuit_id_in_use_on_orconn(circid_t circ_id, or_connection_t *conn);
 circuit_t *circuit_get_by_edge_conn(edge_connection_t *conn);
 void circuit_unlink_all_from_or_conn(or_connection_t *conn, int reason);
 origin_circuit_t *circuit_get_by_global_id(uint32_t id);
-origin_circuit_t *circuit_get_by_rend_query_and_purpose(const char *rend_query,
-                                                        uint8_t purpose);
+origin_circuit_t *circuit_get_ready_rend_circ_by_rend_data(
+  const rend_data_t *rend_data);
 origin_circuit_t *circuit_get_next_by_pk_and_purpose(origin_circuit_t *start,
                                          const char *digest, uint8_t purpose);
 or_circuit_t *circuit_get_rendezvous(const char *cookie);
diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index ef4ac6faa..c07d434b7 100644
--- a/src/or/circuituse.c
+++ b/src/or/circuituse.c
@@ -75,6 +75,11 @@ circuit_is_acceptable(const origin_circuit_t *origin_circ,
       return 0;
   }
 
+  /* If this is a timed-out hidden service circuit, skip it. */
+  if (origin_circ->hs_circ_has_timed_out) {
+    return 0;
+  }
+
   if (purpose == CIRCUIT_PURPOSE_C_GENERAL ||
       purpose == CIRCUIT_PURPOSE_C_REND_JOINED)
     if (circ->timestamp_dirty &&
@@ -351,7 +356,9 @@ circuit_expire_building(void)
    * circuit_build_times_get_initial_timeout() if we haven't computed
    * custom timeouts yet */
   struct timeval general_cutoff, begindir_cutoff, fourhop_cutoff,
-    cannibalize_cutoff, close_cutoff, extremely_old_cutoff;
+    cannibalize_cutoff, close_cutoff, extremely_old_cutoff,
+    hs_extremely_old_cutoff;
+  const or_options_t *options = get_options();
   struct timeval now;
   cpath_build_state_t *build_state;
 
@@ -371,6 +378,10 @@ circuit_expire_building(void)
   SET_CUTOFF(close_cutoff, circ_times.close_ms);
   SET_CUTOFF(extremely_old_cutoff, circ_times.close_ms*2 + 1000);
 
+  SET_CUTOFF(hs_extremely_old_cutoff,
+             MAX(circ_times.close_ms*2 + 1000,
+                 options->SocksTimeout * 1000));
+
   while (next_circ) {
     struct timeval cutoff;
     victim = next_circ;
@@ -392,6 +403,9 @@ circuit_expire_building(void)
     else
       cutoff = general_cutoff;
 
+    if (TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out)
+      cutoff = hs_extremely_old_cutoff;
+
     if (timercmp(&victim->timestamp_created, &cutoff, >))
       continue; /* it's still young, leave it alone */
 
@@ -497,6 +511,62 @@ circuit_expire_building(void)
       }
     }
 
+    /* If this is a hidden service client circuit which is far enough
+     * along in connecting to its destination, and we haven't already
+     * flagged it as 'timed out', and the user has not told us to
+     * close such circs immediately on timeout, flag it as 'timed out'
+     * so we'll launch another intro or rend circ, but don't mark it
+     * for close yet.
+     *
+     * (Circs flagged as 'timed out' are given a much longer timeout
+     * period above, so we won't close them in the next call to
+     * circuit_expire_building.) */
+    if (!(options->CloseHSClientCircuitsImmediatelyOnTimeout) &&
+        !(TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out)) {
+      switch (victim->purpose) {
+      case CIRCUIT_PURPOSE_C_REND_READY:
+        /* We only want to spare a rend circ if it has been specified in
+         * an INTRODUCE1 cell sent to a hidden service.  A circ's
+         * pending_final_cpath field is non-NULL iff it is a rend circ
+         * and we have tried to send an INTRODUCE1 cell specifying it.
+         * Thus, if the pending_final_cpath field *is* NULL, then we
+         * want to not spare it. */
+        if (TO_ORIGIN_CIRCUIT(victim)->build_state->pending_final_cpath ==
+            NULL)
+          break;
+      case CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT:
+      case CIRCUIT_PURPOSE_C_REND_READY_INTRO_ACKED:
+        /* If we have reached this line, we want to spare the circ for now. */
+        log_info(LD_CIRC,"Marking circ %s:%d:%d (state %d:%s, purpose %d) "
+                 "as timed-out HS circ",
+                 victim->n_conn->_base.address, victim->n_conn->_base.port,
+                 victim->n_circ_id,
+                 victim->state, circuit_state_to_string(victim->state),
+                 victim->purpose);
+        TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out = 1;
+        continue;
+      default:
+        break;
+      }
+    }
+
+    /* If this is a service-side rendezvous circuit which is far
+     * enough along in connecting to its destination, consider sparing
+     * it. */
+    if (!(options->CloseHSServiceRendCircuitsImmediatelyOnTimeout) &&
+        !(TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out) &&
+        victim->purpose == CIRCUIT_PURPOSE_S_CONNECT_REND) {
+      log_info(LD_CIRC,"Marking circ %s:%d:%d (state %d:%s, purpose %d) "
+               "as timed-out HS circ; relaunching rendezvous attempt.",
+               victim->n_conn->_base.address, victim->n_conn->_base.port,
+               victim->n_circ_id,
+               victim->state, circuit_state_to_string(victim->state),
+               victim->purpose);
+      TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out = 1;
+      rend_service_relaunch_rendezvous(TO_ORIGIN_CIRCUIT(victim));
+      continue;
+    }
+
     if (victim->n_conn)
       log_info(LD_CIRC,"Abandoning circ %s:%d:%d (state %d:%s, purpose %d)",
                victim->n_conn->_base.address, victim->n_conn->_base.port,
diff --git a/src/or/config.c b/src/or/config.c
index be4113f41..740a9dbfd 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -306,6 +306,8 @@ static config_var_t _option_vars[] = {
   V(HidServAuth,                 LINELIST, NULL),
   V(HSAuthoritativeDir,          BOOL,     "0"),
   OBSOLETE("HSAuthorityRecordStats"),
+  V(CloseHSClientCircuitsImmediatelyOnTimeout, BOOL, "0"),
+  V(CloseHSServiceRendCircuitsImmediatelyOnTimeout, BOOL, "0"),
   V(HTTPProxy,                   STRING,   NULL),
   V(HTTPProxyAuthenticator,      STRING,   NULL),
   V(HTTPSProxy,                  STRING,   NULL),
diff --git a/src/or/or.h b/src/or/or.h
index 63ff5c4b3..2acb62101 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -2607,6 +2607,30 @@ typedef struct origin_circuit_t {
    * cannibalized circuits. */
   unsigned int has_opened : 1;
 
+  /** Set iff this is a hidden-service circuit which has timed out
+   * according to our current circuit-build timeout, but which has
+   * been kept around because it might still succeed in connecting to
+   * its destination, and which is not a fully-connected rendezvous
+   * circuit.
+   *
+   * (We clear this flag for client-side rendezvous circuits when they
+   * are 'joined' to the other side's rendezvous circuit, so that
+   * connection_ap_handshake_attach_circuit can put client streams on
+   * the circuit.  We also clear this flag for service-side rendezvous
+   * circuits when they are 'joined' to a client's rend circ, but only
+   * for symmetry with the client case.  Client-side introduction
+   * circuits are closed when we get a joined rend circ, and
+   * service-side introduction circuits never have this flag set.) */
+  unsigned int hs_circ_has_timed_out : 1;
+
+  /** Set iff this is a service-side rendezvous circuit for which a
+   * new connection attempt has been launched.  We consider launching
+   * a new service-side rend circ to a client when the previous one
+   * fails; now that we don't necessarily close a service-side rend
+   * circ when we launch a new one to the same client, this flag keeps
+   * us from launching two retries for the same failed rend circ. */
+  unsigned int hs_service_side_rend_circ_has_been_relaunched : 1;
+
   /** What commands were sent over this circuit that decremented the
    * RELAY_EARLY counter? This is for debugging task 878. */
   uint8_t relay_early_commands[MAX_RELAY_EARLY_CELLS_PER_CIRCUIT];
@@ -3050,6 +3074,15 @@ typedef struct {
    * circuits.) */
   int Tor2webMode;
 
+  /** Close hidden service client circuits immediately when they reach
+   * the normal circuit-build timeout, even if they have already sent
+   * an INTRODUCE1 cell on its way to the service. */
+  int CloseHSClientCircuitsImmediatelyOnTimeout;
+
+  /** Close hidden-service-side rendezvous circuits immediately when
+   * they reach the normal circuit-build timeout. */
+  int CloseHSServiceRendCircuitsImmediatelyOnTimeout;
+
   int ConnLimit; /**< Demanded minimum number of simultaneous connections. */
   int _ConnLimit; /**< Maximum allowed number of simultaneous connections. */
   int RunAsDaemon; /**< If true, run in the background. (Unix only) */
diff --git a/src/or/rendclient.c b/src/or/rendclient.c
index 8aae8c5cb..17c92ab6a 100644
--- a/src/or/rendclient.c
+++ b/src/or/rendclient.c
@@ -275,6 +275,12 @@ rend_client_send_introduction(origin_circuit_t *introcirc,
   payload_len = DIGEST_LEN + r;
   tor_assert(payload_len <= RELAY_PAYLOAD_SIZE); /* we overran something */
 
+  /* Copy the rendezvous cookie from rendcirc to introcirc, so that
+   * when introcirc gets an ack, we can change the state of the right
+   * rendezvous circuit. */
+  memcpy(rendcirc->rend_data->rend_cookie, introcirc->rend_data->rend_cookie,
+         REND_COOKIE_LEN);
+
   log_info(LD_REND, "Sending an INTRODUCE1 cell");
   if (relay_send_command_from_edge(0, TO_CIRCUIT(introcirc),
                                    RELAY_COMMAND_INTRODUCE1,
@@ -344,8 +350,7 @@ rend_client_introduction_acked(origin_circuit_t *circ,
      * and tell it.
      */
     log_info(LD_REND,"Received ack. Telling rend circ...");
-    rendcirc = circuit_get_by_rend_query_and_purpose(
-               circ->rend_data->onion_address, CIRCUIT_PURPOSE_C_REND_READY);
+    rendcirc = circuit_get_ready_rend_circ_by_rend_data(circ->rend_data);
     if (rendcirc) { /* remember the ack */
 #ifndef NON_ANONYMOUS_MODE_ENABLED
       tor_assert(!(rendcirc->build_state->onehop_tunnel));
@@ -890,6 +895,11 @@ rend_client_receive_rendezvous(origin_circuit_t *circ, const uint8_t *request,
   hop->package_window = circuit_initial_package_window();
   hop->deliver_window = CIRCWINDOW_START;
 
+  /* Now that this circuit has finished connecting to its destination,
+   * make sure circuit_get_open_circ_or_launch is willing to return it
+   * so we can actually use it. */
+  circ->hs_circ_has_timed_out = 0;
+
   onion_append_to_cpath(&circ->cpath, hop);
   circ->build_state->pending_final_cpath = NULL; /* prevent double-free */
 
diff --git a/src/or/rendservice.c b/src/or/rendservice.c
index 76caeffd0..bbc9c9186 100644
--- a/src/or/rendservice.c
+++ b/src/or/rendservice.c
@@ -1419,6 +1419,17 @@ rend_service_relaunch_rendezvous(origin_circuit_t *oldcirc)
 
   tor_assert(oldcirc->_base.purpose == CIRCUIT_PURPOSE_S_CONNECT_REND);
 
+  /* Don't relaunch the same rend circ twice. */
+  if (oldcirc->hs_service_side_rend_circ_has_been_relaunched) {
+    log_info(LD_REND, "Rendezvous circuit to %s has already been relaunched; "
+             "not relaunching it again.",
+             oldcirc->build_state ?
+             safe_str(extend_info_describe(oldcirc->build_state->chosen_exit))
+             : "*unknown*");
+    return;
+  }
+  oldcirc->hs_service_side_rend_circ_has_been_relaunched = 1;
+
   if (!oldcirc->build_state ||
       oldcirc->build_state->failure_count > MAX_REND_FAILURES ||
       oldcirc->build_state->expiry_time < time(NULL)) {
@@ -1727,6 +1738,11 @@ rend_service_rendezvous_has_opened(origin_circuit_t *circuit)
            "cookie %s for service %s",
            circuit->_base.n_circ_id, hexcookie, serviceid);
 
+  /* Clear the 'in-progress HS circ has timed out' flag for
+   * consistency with what happens on the client side; this line has
+   * no effect on Tor's behaviour. */
+  circuit->hs_circ_has_timed_out = 0;
+
   service = rend_service_get_by_pk_digest(
                 circuit->rend_data->rend_pk_digest);
   if (!service) {