[tor-commits] [tor/master] Sadly, we can't safely count client intro circ success

[nickm at torproject.org]

commit b599a6ed07fd588500a256ef815e87729449626a
Author: Mike Perry <mikeperry-git at fscked.org>
Date:   Sat Dec 8 14:16:29 2012 -0800

    Sadly, we can't safely count client intro circ success
---
 src/or/circuitbuild.c |   42 +++++++++++++++++-------------------------
 src/or/circuituse.c   |    9 +++++++--
 2 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 7eae0e7..9b1236f 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -739,8 +739,12 @@ circuit_send_next_onion_skin(origin_circuit_t *circ)
       circuit_has_opened(circ); /* do other actions as necessary */
 
       /* We're done with measurement circuits here. Just close them */
-      if (circ->base_.purpose == CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT)
+      if (circ->base_.purpose == CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) {
+        /* If a measurement circ ever gets back to us, consider it
+         * succeeded for path bias */
+        circ->path_state = PATH_STATE_USE_SUCCEEDED;
         circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_FINISHED);
+      }
       return 0;
     }
 
@@ -1156,13 +1160,19 @@ pathbias_should_count(origin_circuit_t *circ)
 
   /* We can't do path bias accounting without entry guards.
    * Testing and controller circuits also have no guards.
+   *
    * We also don't count server-side rends, because their
-   * endpoint could be chosen maliciously. */
+   * endpoint could be chosen maliciously.
+   * Similarly, we can't count client-side intro attempts,
+   * because clients can be manipulated into connecting to
+   * malicious intro points. */
   if (get_options()->UseEntryGuards == 0 ||
           circ->base_.purpose == CIRCUIT_PURPOSE_TESTING ||
           circ->base_.purpose == CIRCUIT_PURPOSE_CONTROLLER ||
           circ->base_.purpose == CIRCUIT_PURPOSE_S_CONNECT_REND ||
-          circ->base_.purpose == CIRCUIT_PURPOSE_S_REND_JOINED) {
+          circ->base_.purpose == CIRCUIT_PURPOSE_S_REND_JOINED ||
+          (circ->base_.purpose >= CIRCUIT_PURPOSE_C_INTRODUCING && 
+           circ->base_.purpose <= CIRCUIT_PURPOSE_C_INTRODUCE_ACKED)) {
     return 0;
   }
 
@@ -1388,7 +1398,7 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason)
 {
   circuit_t *circ = &ocirc->base_;
 
-  if (!pathbias_should_count(circ)) {
+  if (!pathbias_should_count(ocirc)) {
     return;
   }
 
@@ -1399,33 +1409,15 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason)
       // 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: 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,
+
+      log_info(LD_CIRC,
             "Circuit closed without successful use for reason %d. "
             "Circuit purpose %d currently %s.",
             reason, circ->purpose, circuit_state_to_string(circ->state));
-        pathbias_count_unusable(ocirc);
-      }
+      pathbias_count_unusable(ocirc);
     } else {
       if (reason & END_CIRC_REASON_FLAG_REMOTE) {
         /* Unused remote circ close reasons all could be bias */
-        // XXX: We hit this a lot for hidserv circs with purposes:
-        // CIRCUIT_PURPOSE_S_CONNECT_REND (reasons: 514,517,520)
-        // CIRCUIT_PURPOSE_S_REND_JOINED (reasons: 514,517,520)
-        //  == reasons: 2,3,8. Client-side timeouts?
         log_info(LD_CIRC,
             "Circuit remote-closed without successful use for reason %d. "
             "Circuit purpose %d currently %s.",
diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index 9362e24..0b799b1 100644
--- a/src/or/circuituse.c
+++ b/src/or/circuituse.c
@@ -1402,11 +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 &&
+      if ((purpose == CIRCUIT_PURPOSE_S_CONNECT_REND ||
+           purpose == CIRCUIT_PURPOSE_C_INTRODUCING) &&
           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. */
+         * because the rend point could be malicious. 
+         *
+         * Same deal goes for client side introductions. Clients
+         * can be manipulated to connect repeatedly to them
+         * (especially web clients). */
         circ->path_state = PATH_STATE_USE_SUCCEEDED;
         /* This must be called before the purpose change */
         pathbias_check_close(circ);