Fix some hidden service edge cases.

3 files changed, 59 insertions, 18 deletions

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index af36cb2c3..7eae0e7a9 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -1155,10 +1155,14 @@ pathbias_should_count(origin_circuit_t *circ)
   char *rate_msg = NULL;
 
   /* We can't do path bias accounting without entry guards.
-   * Testing and controller circuits also have no guards. */
+   * Testing and controller circuits also have no guards.
+   * We also don't count server-side rends, because their
+   * endpoint could be chosen maliciously. */
   if (get_options()->UseEntryGuards == 0 ||
           circ->base_.purpose == CIRCUIT_PURPOSE_TESTING ||
-          circ->base_.purpose == CIRCUIT_PURPOSE_CONTROLLER) {
+          circ->base_.purpose == CIRCUIT_PURPOSE_CONTROLLER ||
+          circ->base_.purpose == CIRCUIT_PURPOSE_S_CONNECT_REND ||
+          circ->base_.purpose == CIRCUIT_PURPOSE_S_REND_JOINED) {
     return 0;
   }
 
@@ -1384,22 +1388,37 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason)
 {
   circuit_t *circ = &ocirc->base_;
 
+  if (!pathbias_should_count(circ)) {
+    return;
+  }
+
   if (ocirc->path_state == PATH_STATE_BUILD_SUCCEEDED) {
     if (circ->timestamp_dirty) {
+      /* Any circuit where there were attempted streams but no successful
+       * streams could be bias */
       // XXX: May open up attacks if the adversary can force connections
       // on unresponsive hosts to use new circs. Vidalia displayes a "Retrying"
       // state.. Can we use that? Does optimistic data change this?
-      // XXX: For the hidserv side, we could only care about INTRODUCING purposes
-      // for server+client, and REND purposes for the server... Can we
-      // somehow only count those?
-      /* Any circuit where there were attempted streams but no successful
-       * streams could be bias */
-      log_info(LD_CIRC,
+      // XXX: Sub-attack: in collusion with an intro point, you can induce bias
+      // through the web. Need a Torbutton patch to prevent this.
+
+      /* FIXME: This is not ideal, but it prevents the case where a
+       * CPU overloaded intro point is chosen.
+       * XXX: Is this reason code authenticated?  */
+      if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCING &&
+          reason ==
+            END_CIRC_REASON_FLAG_REMOTE|END_CIRC_REASON_RESOURCELIMIT) {
+        log_info(LD_CIRC,
+            "Ignoring CPU overload intro circuit without successful use. "
+            "Circuit purpose %d currently %s.",
+            reason, circ->purpose, circuit_state_to_string(circ->state));
+      } else {
+        log_info(LD_CIRC,
             "Circuit closed without successful use for reason %d. "
-            "Circuit is a %s currently %s.",
-            reason, circuit_purpose_to_string(circ->purpose),
-            circuit_state_to_string(circ->state));
-      pathbias_count_unusable(ocirc);
+            "Circuit purpose %d currently %s.",
+            reason, circ->purpose, circuit_state_to_string(circ->state));
+        pathbias_count_unusable(ocirc);
+      }
     } else {
       if (reason & END_CIRC_REASON_FLAG_REMOTE) {
         /* Unused remote circ close reasons all could be bias */
@@ -1409,9 +1428,8 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason)
         //  == reasons: 2,3,8. Client-side timeouts?
         log_info(LD_CIRC,
             "Circuit remote-closed without successful use for reason %d. "
-            "Circuit is a %s currently %s.",
-            reason, circuit_purpose_to_string(circ->purpose),
-            circuit_state_to_string(circ->state));
+            "Circuit purpose %d currently %s.",
+            reason, circ->purpose, circuit_state_to_string(circ->state));
         pathbias_count_collapse(ocirc);
       } else if ((reason & ~END_CIRC_REASON_FLAG_REMOTE)
                   == END_CIRC_REASON_CHANNEL_CLOSED &&
@@ -1423,10 +1441,9 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason)
          * What about clock jumps/suspends? */
         log_info(LD_CIRC,
             "Circuit's channel closed without successful use for reason %d, "
-            "channel reason %d. Circuit is a %s currently %s.",
+            "channel reason %d. Circuit purpose %d currently %s.",
             reason, circ->n_chan->reason_for_closing,
-            circuit_purpose_to_string(circ->purpose),
-            circuit_state_to_string(circ->state));
+            circ->purpose, circuit_state_to_string(circ->state));
         pathbias_count_collapse(ocirc);
       } else {
         pathbias_count_successful_close(ocirc);
@@ -1548,6 +1565,13 @@ pathbias_count_timeout(origin_circuit_t *circ)
     return;
   }
 
+  /* For hidden service circs, they can actually be used
+   * successfully and then time out later (because
+   * the other side declines to use them). */
+  if (circ->path_state == PATH_STATE_USE_SUCCEEDED) {
+    return;
+  }
+
   if (circ->cpath && circ->cpath->extend_info) {
     guard = entry_guard_get_by_id_digest(
               circ->cpath->extend_info->identity_digest);
diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index 77822a36a..9362e2420 100644
--- a/src/or/circuituse.c
+++ b/src/or/circuituse.c
@@ -1402,6 +1402,16 @@ circuit_launch_by_extend_info(uint8_t purpose,
                build_state_get_exit_nickname(circ->build_state), purpose,
                circuit_purpose_to_string(purpose));
 
+      if (purpose == CIRCUIT_PURPOSE_S_CONNECT_REND &&
+          circ->path_state == PATH_STATE_BUILD_SUCCEEDED) {
+        /* Path bias: Cannibalized rends pre-emptively count as a
+         * successfully used circ. We don't wait until the extend,
+         * because the rend point could be malicious. */
+        circ->path_state = PATH_STATE_USE_SUCCEEDED;
+        /* This must be called before the purpose change */
+        pathbias_check_close(circ);
+      }
+
       circuit_change_purpose(TO_CIRCUIT(circ), purpose);
       /* Reset the start date of this circ, else expire_building
        * will see it and think it's been trying to build since it
diff --git a/src/or/rendclient.c b/src/or/rendclient.c
index 3393e0f62..88241a4b2 100644
--- a/src/or/rendclient.c
+++ b/src/or/rendclient.c
@@ -862,6 +862,13 @@ rend_client_rendezvous_acked(origin_circuit_t *circ, const uint8_t *request,
   /* Set timestamp_dirty, because circuit_expire_building expects it
    * to specify when a circuit entered the _C_REND_READY state. */
   circ->base_.timestamp_dirty = time(NULL);
+
+  /* From a path bias point of view, this circuit is now successfully used.
+   * Waiting any longer opens us up to attacks from Bob. He could induce
+   * Alice to attempt to connect to his hidden service and never reply
+   * to her rend requests */
+  circ->path_state = PATH_STATE_USE_SUCCEEDED;
+
   /* XXXX This is a pretty brute-force approach. It'd be better to
    * attach only the connections that are waiting on this circuit, rather
    * than trying to attach them all. See comments bug 743. */