Merge branch 'bug13790_rebased'

6 files changed, 225 insertions, 11 deletions

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index f08e2ee6af..f8b3609757 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -73,7 +73,6 @@ static int circuit_deliver_create_cell(circuit_t *circ,
 static int onion_pick_cpath_exit(origin_circuit_t *circ, extend_info_t *exit);
 static crypt_path_t *onion_next_hop_in_cpath(crypt_path_t *cpath);
 static int onion_extend_cpath(origin_circuit_t *circ);
-static int count_acceptable_nodes(smartlist_t *routers);
 static int onion_append_hop(crypt_path_t **head_ptr, extend_info_t *choice);
 
 /** This function tries to get a channel to the specified endpoint,
@@ -1546,13 +1545,98 @@ onionskin_answer(or_circuit_t *circ,
   return 0;
 }
 
-/** Choose a length for a circuit of purpose <b>purpose</b>: three + the
- * number of endpoints that would give something away about our destination.
+/** Helper for new_route_len().  Choose a circuit length for purpose
+ * <b>purpose</b>: DEFAULT_ROUTE_LEN (+ 1 if someone else chose the
+ * exit).  If someone else chose the exit, they could be colluding
+ * with the exit, so add a randomly selected node to preserve
+ * anonymity.
+ *
+ * Here, "exit node" sometimes means an OR acting as an internal
+ * endpoint, rather than as a relay to an external endpoint.  This
+ * means there need to be at least DEFAULT_ROUTE_LEN routers between
+ * us and the internal endpoint to preserve the same anonymity
+ * properties that we would get when connecting to an external
+ * endpoint.  These internal endpoints can include:
+ *
+ *   - Connections to a directory of hidden services
+ *     (CIRCUIT_PURPOSE_C_GENERAL)
+ *
+ *   - A client connecting to an introduction point, which the hidden
+ *     service picked (CIRCUIT_PURPOSE_C_INTRODUCING, via
+ *     circuit_get_open_circ_or_launch() which rewrites it from
+ *     CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT)
+ *
+ *   - A hidden service connecting to a rendezvous point, which the
+ *     client picked (CIRCUIT_PURPOSE_S_CONNECT_REND, via
+ *     rend_service_receive_introduction() and
+ *     rend_service_relaunch_rendezvous)
+ *
+ * There are currently two situations where we picked the exit node
+ * ourselves, making DEFAULT_ROUTE_LEN a safe circuit length:
+ *
+ *   - We are a hidden service connecting to an introduction point
+ *     (CIRCUIT_PURPOSE_S_ESTABLISH_INTRO, via
+ *     rend_service_launch_establish_intro())
+ *
+ *   - We are a router testing its own reachabiity
+ *     (CIRCUIT_PURPOSE_TESTING, via consider_testing_reachability())
+ *
+ * onion_pick_cpath_exit() bypasses us (by not calling
+ * new_route_len()) in the one-hop tunnel case, so we don't need to
+ * handle that.
+ */
+static int
+route_len_for_purpose(uint8_t purpose, extend_info_t *exit_ei)
+{
+  int routelen = DEFAULT_ROUTE_LEN;
+  int known_purpose = 0;
+
+  if (!exit_ei)
+    return routelen;
+
+  switch (purpose) {
+    /* These two purposes connect to a router that we chose, so
+     * DEFAULT_ROUTE_LEN is safe. */
+  case CIRCUIT_PURPOSE_S_ESTABLISH_INTRO:
+    /* hidden service connecting to introduction point */
+  case CIRCUIT_PURPOSE_TESTING:
+    /* router reachability testing */
+    known_purpose = 1;
+    break;
+
+    /* These three purposes connect to a router that someone else
+     * might have chosen, so add an extra hop to protect anonymity. */
+  case CIRCUIT_PURPOSE_C_GENERAL:
+    /* connecting to hidden service directory */
+  case CIRCUIT_PURPOSE_C_INTRODUCING:
+    /* client connecting to introduction point */
+  case CIRCUIT_PURPOSE_S_CONNECT_REND:
+    /* hidden service connecting to rendezvous point */
+    known_purpose = 1;
+    routelen++;
+    break;
+
+  default:
+    /* Got a purpose not listed above along with a chosen exit.
+     * Increase the circuit length by one anyway for safety. */
+    routelen++;
+    break;
+  }
+
+  if (BUG(exit_ei && !known_purpose)) {
+    log_warn(LD_BUG, "Unhandled purpose %d with a chosen exit; "
+             "assuming routelen %d.", purpose, routelen);
+  }
+  return routelen;
+}
+
+/** Choose a length for a circuit of purpose <b>purpose</b> and check
+ * if enough routers are available.
  *
  * If the routerlist <b>nodes</b> doesn't have enough routers
  * to handle the desired path length, return -1.
  */
-static int
+STATIC int
 new_route_len(uint8_t purpose, extend_info_t *exit_ei, smartlist_t *nodes)
 {
   int num_acceptable_routers;
@@ -1560,11 +1644,7 @@ new_route_len(uint8_t purpose, extend_info_t *exit_ei, smartlist_t *nodes)
 
   tor_assert(nodes);
 
-  routelen = DEFAULT_ROUTE_LEN;
-  if (exit_ei &&
-      purpose != CIRCUIT_PURPOSE_TESTING &&
-      purpose != CIRCUIT_PURPOSE_S_ESTABLISH_INTRO)
-    routelen++;
+  routelen = route_len_for_purpose(purpose, exit_ei);
 
   num_acceptable_routers = count_acceptable_nodes(nodes);
 
@@ -2188,8 +2268,8 @@ circuit_extend_to_new_exit(origin_circuit_t *circ, extend_info_t *exit_ei)
 /** Return the number of routers in <b>routers</b> that are currently up
  * and available for building circuits through.
  */
-static int
-count_acceptable_nodes(smartlist_t *nodes)
+MOCK_IMPL(STATIC int,
+count_acceptable_nodes, (smartlist_t *nodes))
 {
   int num=0;
 
diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h
index 62a80b2b89..45d9b2fb75 100644
--- a/src/or/circuitbuild.h
+++ b/src/or/circuitbuild.h
@@ -77,6 +77,9 @@ void circuit_upgrade_circuits_from_guard_wait(void);
 
 #ifdef CIRCUITBUILD_PRIVATE
 STATIC circid_t get_unique_circ_id_by_chan(channel_t *chan);
+STATIC int new_route_len(uint8_t purpose, extend_info_t *exit_ei,
+                         smartlist_t *nodes);
+MOCK_DECL(STATIC int, count_acceptable_nodes, (smartlist_t *nodes));
 #if defined(ENABLE_TOR2WEB_MODE) || defined(TOR_UNIT_TESTS)
 STATIC const node_t *pick_tor2web_rendezvous_node(router_crn_flags_t flags,
                                                   const or_options_t *options);
diff --git a/src/test/include.am b/src/test/include.am
index 7ced7baa77..9670fd8189 100644
--- a/src/test/include.am
+++ b/src/test/include.am
@@ -82,6 +82,7 @@ src_test_test_SOURCES = \
 	src/test/test_checkdir.c \
 	src/test/test_circuitlist.c \
 	src/test/test_circuitmux.c \
+	src/test/test_circuitbuild.c \
 	src/test/test_circuituse.c \
 	src/test/test_compat_libevent.c \
 	src/test/test_config.c \
diff --git a/src/test/test.c b/src/test/test.c
index d9723d55d3..fc1e3e89c0 100644
--- a/src/test/test.c
+++ b/src/test/test.c
@@ -1188,6 +1188,7 @@ struct testgroup_t testgroups[] = {
   { "channel/", channel_tests },
   { "channeltls/", channeltls_tests },
   { "checkdir/", checkdir_tests },
+  { "circuitbuild/", circuitbuild_tests },
   { "circuitlist/", circuitlist_tests },
   { "circuitmux/", circuitmux_tests },
   { "circuituse/", circuituse_tests },
diff --git a/src/test/test.h b/src/test/test.h
index 0692040656..ec4d715422 100644
--- a/src/test/test.h
+++ b/src/test/test.h
@@ -183,6 +183,7 @@ extern struct testcase_t cell_queue_tests[];
 extern struct testcase_t channel_tests[];
 extern struct testcase_t channeltls_tests[];
 extern struct testcase_t checkdir_tests[];
+extern struct testcase_t circuitbuild_tests[];
 extern struct testcase_t circuitlist_tests[];
 extern struct testcase_t circuitmux_tests[];
 extern struct testcase_t circuituse_tests[];
diff --git a/src/test/test_circuitbuild.c b/src/test/test_circuitbuild.c
new file mode 100644
index 0000000000..338bc1a667
--- /dev/null
+++ b/src/test/test_circuitbuild.c
@@ -0,0 +1,128 @@
+/* Copyright (c) 2001-2004, Roger Dingledine.
+ * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
+ * Copyright (c) 2007-2016, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+#define CIRCUITBUILD_PRIVATE
+
+#include "or.h"
+#include "test.h"
+#include "test_helpers.h"
+#include "config.h"
+#include "circuitbuild.h"
+
+/* Dummy nodes smartlist for testing */
+static smartlist_t dummy_nodes;
+/* Dummy exit extend_info for testing */
+static extend_info_t dummy_ei;
+
+static int
+mock_count_acceptable_nodes(smartlist_t *nodes)
+{
+  (void)nodes;
+
+  return DEFAULT_ROUTE_LEN + 1;
+}
+
+/* Test route lengths when the caller of new_route_len() doesn't
+ * specify exit_ei. */
+static void
+test_new_route_len_noexit(void *arg)
+{
+  int r;
+
+  (void)arg;
+  MOCK(count_acceptable_nodes, mock_count_acceptable_nodes);
+
+  r = new_route_len(CIRCUIT_PURPOSE_C_GENERAL, NULL, &dummy_nodes);
+  tt_int_op(DEFAULT_ROUTE_LEN, OP_EQ, r);
+
+  r = new_route_len(CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT, NULL, &dummy_nodes);
+  tt_int_op(DEFAULT_ROUTE_LEN, OP_EQ, r);
+
+  r = new_route_len(CIRCUIT_PURPOSE_S_CONNECT_REND, NULL, &dummy_nodes);
+  tt_int_op(DEFAULT_ROUTE_LEN, OP_EQ, r);
+
+ done:
+  UNMOCK(count_acceptable_nodes);
+}
+
+/* Test route lengths where someone else chose the "exit" node, which
+ * require an extra hop for safety. */
+static void
+test_new_route_len_unsafe_exit(void *arg)
+{
+  int r;
+
+  (void)arg;
+  MOCK(count_acceptable_nodes, mock_count_acceptable_nodes);
+
+  /* connecting to hidden service directory */
+  r = new_route_len(CIRCUIT_PURPOSE_C_GENERAL, &dummy_ei, &dummy_nodes);
+  tt_int_op(DEFAULT_ROUTE_LEN + 1, OP_EQ, r);
+
+  /* client connecting to introduction point */
+  r = new_route_len(CIRCUIT_PURPOSE_C_INTRODUCING, &dummy_ei, &dummy_nodes);
+  tt_int_op(DEFAULT_ROUTE_LEN + 1, OP_EQ, r);
+
+  /* hidden service connecting to rendezvous point */
+  r = new_route_len(CIRCUIT_PURPOSE_S_CONNECT_REND, &dummy_ei, &dummy_nodes);
+  tt_int_op(DEFAULT_ROUTE_LEN + 1, OP_EQ, r);
+
+ done:
+  UNMOCK(count_acceptable_nodes);
+}
+
+/* Test route lengths where we chose the "exit" node, which don't
+ * require an extra hop for safety. */
+static void
+test_new_route_len_safe_exit(void *arg)
+{
+  int r;
+
+  (void)arg;
+  MOCK(count_acceptable_nodes, mock_count_acceptable_nodes);
+
+  /* hidden service connecting to introduction point */
+  r = new_route_len(CIRCUIT_PURPOSE_S_ESTABLISH_INTRO, &dummy_ei,
+                    &dummy_nodes);
+  tt_int_op(DEFAULT_ROUTE_LEN, OP_EQ, r);
+
+  /* router testing its own reachability */
+  r = new_route_len(CIRCUIT_PURPOSE_TESTING, &dummy_ei, &dummy_nodes);
+  tt_int_op(DEFAULT_ROUTE_LEN, OP_EQ, r);
+
+ done:
+  UNMOCK(count_acceptable_nodes);
+}
+
+/* Make sure a non-fatal assertion fails when new_route_len() gets an
+ * unexpected circuit purpose. */
+static void
+test_new_route_len_unhandled_exit(void *arg)
+{
+  int r;
+
+  (void)arg;
+  MOCK(count_acceptable_nodes, mock_count_acceptable_nodes);
+
+  tor_capture_bugs_(1);
+  r = new_route_len(CIRCUIT_PURPOSE_CONTROLLER, &dummy_ei, &dummy_nodes);
+  tt_int_op(DEFAULT_ROUTE_LEN + 1, OP_EQ, r);
+  tt_int_op(smartlist_len(tor_get_captured_bug_log_()), OP_EQ, 1);
+  tt_str_op(smartlist_get(tor_get_captured_bug_log_(), 0), OP_EQ,
+            "!(exit_ei && !known_purpose)");
+  tor_end_capture_bugs_();
+
+ done:
+  UNMOCK(count_acceptable_nodes);
+}
+
+struct testcase_t circuitbuild_tests[] = {
+  { "noexit", test_new_route_len_noexit, 0, NULL, NULL },
+  { "safe_exit", test_new_route_len_safe_exit, 0, NULL, NULL },
+  { "unsafe_exit", test_new_route_len_unsafe_exit, 0, NULL, NULL },
+  { "unhandled_exit", test_new_route_len_unhandled_exit, 0, NULL, NULL },
+  END_OF_TESTCASES
+};
+