Merge remote-tracking branch 'mikeperry/bug25870_rebase'

changes/bug25870

@@ -0,0 +1,6 @@
+  o Minor bugfixes (vanguards):
+    - Allow the last hop in a vanguard circuit to be the same as our first,
+      to prevent the adversary from influencing guard node choice by choice
+      of last hop. Also prevent the creation of A - B - A paths, or A - A
+      paths, which are forbidden by relays. Fixes bug 25870; bugfix on
+      0.3.3.1-alpha.

doc/tor.1.txt

@@ -1591,6 +1591,14 @@ The following options are useful only for clients (that is, if
     ExcludeNodes have higher priority than HSLayer2Nodes,
     which means that nodes specified in ExcludeNodes will not be
     picked.
+ +
+    When either this option or HSLayer3Nodes are set, the /16 subnet
+    and node family restrictions are removed for hidden service
+    circuits. Additionally, we allow the guard node to be present
+    as the Rend, HSDir, and IP node, and as the hop before it. This
+    is done to prevent the adversary from inferring information
+    about our guard, layer2, and layer3 node choices at later points
+    in the path.
  +
     This option is meant to be managed by a Tor controller such as
     https://github.com/mikeperry-tor/vanguards that selects and
@@ -1637,6 +1645,14 @@ The following options are useful only for clients (that is, if
     ExcludeNodes have higher priority than HSLayer3Nodes,
     which means that nodes specified in ExcludeNodes will not be
     picked.
+ +
+    When either this option or HSLayer2Nodes are set, the /16 subnet
+    and node family restrictions are removed for hidden service
+    circuits. Additionally, we allow the guard node to be present
+    as the Rend, HSDir, and IP node, and as the hop before it. This
+    is done to prevent the adversary from inferring information
+    about our guard, layer2, and layer3 node choices at later points
+    in the path.
   +
     This option is meant to be managed by a Tor controller such as
     https://github.com/mikeperry-tor/vanguards that selects and

src/or/circuitbuild.c

@@ -72,10 +72,7 @@ static channel_t * channel_connect_for_circuit(const tor_addr_t *addr,
 static int circuit_deliver_create_cell(circuit_t *circ,
                                        const create_cell_t *create_cell,
                                        int relayed);
-static int onion_pick_cpath_exit(origin_circuit_t *circ, extend_info_t *exit,
-                                 int is_hs_v3_rp_circuit);
 static crypt_path_t *onion_next_hop_in_cpath(crypt_path_t *cpath);
-static int onion_extend_cpath(origin_circuit_t *circ);
 STATIC int onion_append_hop(crypt_path_t **head_ptr, extend_info_t *choice);
 static int circuit_send_first_onion_skin(origin_circuit_t *circ);
 static int circuit_build_no_more_hops(origin_circuit_t *circ);
@@ -2283,7 +2280,7 @@ warn_if_last_router_excluded(origin_circuit_t *circ,
  * be used as an HS v3 rendezvous point.
  *
  * Return 0 if ok, -1 if circuit should be closed. */
-static int
+STATIC int
 onion_pick_cpath_exit(origin_circuit_t *circ, extend_info_t *exit_ei,
                       int is_hs_v3_rp_circuit)
 {
@@ -2453,13 +2450,72 @@ cpath_get_n_hops(crypt_path_t **head_ptr)
 
 #endif /* defined(TOR_UNIT_TESTS) */
 
+/**
+ * Build the exclude list for vanguard circuits.
+ *
+ * For vanguard circuits we exclude all the already chosen nodes (including the
+ * exit) from being middle hops to prevent the creation of A - B - A subpaths.
+ * We also allow the 4th hop to be the same as the guard node so as to not leak
+ * guard information to RP/IP/HSDirs.
+ *
+ * For vanguard circuits, we don't apply any subnet or family restrictions.
+ * This is to avoid impossible-to-build circuit paths, or just situations where
+ * our earlier guards prevent us from using most of our later ones.
+ *
+ * The alternative is building the circuit in reverse. Reverse calls to
+ * onion_extend_cpath() (ie: select outer hops first) would then have the
+ * property that you don't gain information about inner hops by observing
+ * outer ones. See https://trac.torproject.org/projects/tor/ticket/24487
+ * for this.
+ *
+ * (Note further that we still exclude the exit to prevent A - B - A
+ * at the end of the path. */
+static smartlist_t *
+build_vanguard_middle_exclude_list(uint8_t purpose,
+                                   cpath_build_state_t *state,
+                                   crypt_path_t *head,
+                                   int cur_len)
+{
+  smartlist_t *excluded;
+  const node_t *r;
+  crypt_path_t *cpath;
+  int i;
+
+  (void) purpose;
+
+  excluded = smartlist_new();
+
+  /* Add the exit to the exclude list (note that the exit/last hop is always
+   * chosen first in circuit_establish_circuit()). */
+  if ((r = build_state_get_exit_node(state))) {
+    smartlist_add(excluded, (node_t*)r);
+  }
+
+  /* If we are picking the 4th hop, allow that node to be the guard too.
+   * This prevents us from avoiding the Guard for those hops, which
+   * gives the adversary information about our guard if they control
+   * the RP, IP, or HSDIR. We don't do this check based on purpose
+   * because we also want to allow HS_VANGUARDS pre-build circuits
+   * to use the guard for that last hop.
+   */
+  if (cur_len == DEFAULT_ROUTE_LEN+1) {
+    /* Skip the first hop for the exclude list below */
+    head = head->next;
+    cur_len--;
+  }
+
+  for (i = 0, cpath = head; cpath && i < cur_len; ++i, cpath=cpath->next) {
+    if ((r = node_get_by_id(cpath->extend_info->identity_digest))) {
+      smartlist_add(excluded, (node_t*)r);
+    }
+  }
+
+  return excluded;
+}
+
 /**
  * Build a list of nodes to exclude from the choice of this middle
  * hop, based on already chosen nodes.
- *
- * XXX: At present, this function does not exclude any nodes from
- * the vanguard circuits. See
- * https://trac.torproject.org/projects/tor/ticket/24487
  */
 static smartlist_t *
 build_middle_exclude_list(uint8_t purpose,
@@ -2472,32 +2528,21 @@ build_middle_exclude_list(uint8_t purpose,
   crypt_path_t *cpath;
   int i;
 
+  /** Vanguard circuits have their own path selection rules */
+  if (circuit_should_use_vanguards(purpose)) {
+    return build_vanguard_middle_exclude_list(purpose, state, head, cur_len);
+  }
+
   excluded = smartlist_new();
 
-  /* Add the exit to the exclude list (note that the exit/last hop is always
+  /* For non-vanguard circuits, add the exit and its family to the exclude list
-   * chosen first in circuit_establish_circuit()). */
+   * (note that the exit/last hop is always chosen first in
+   * circuit_establish_circuit()). */
   if ((r = build_state_get_exit_node(state))) {
     nodelist_add_node_and_family(excluded, r);
   }
 
-  /* XXX: We don't apply any other previously selected node restrictions for
+  /* also exclude all other already chosen nodes and their family */
-   * vanguards, and allow nodes to be reused for those hop positions in the
-   * same circuit. This is because after many rotations, you get to learn
-   * inner guard nodes through the nodes that are not selected for outer
-   * hops.
-   *
-   * The alternative is building the circuit in reverse. Reverse calls to
-   * onion_extend_cpath() (ie: select outer hops first) would then have the
-   * property that you don't gain information about inner hops by observing
-   * outer ones. See https://trac.torproject.org/projects/tor/ticket/24487
-   * for this.
-   *
-   * (Note further that we can and do still exclude the exit in the block
-   * above, because it is chosen first in circuit_establish_circuit()..) */
-  if (circuit_should_use_vanguards(purpose)) {
-    return excluded;
-  }
-
   for (i = 0, cpath = head; cpath && i < cur_len; ++i, cpath=cpath->next) {
     if ((r = node_get_by_id(cpath->extend_info->identity_digest))) {
       nodelist_add_node_and_family(excluded, r);
@@ -2597,7 +2642,9 @@ choose_good_middle_server(uint8_t purpose,
   /** If a hidden service circuit wants a specific middle node, pin it. */
   if (middle_node_must_be_vanguard(options, purpose, cur_len)) {
     log_debug(LD_GENERAL, "Picking a sticky node (cur_len = %d)", cur_len);
-    return pick_vanguard_middle_node(options, flags, cur_len, excluded);
+    choice = pick_vanguard_middle_node(options, flags, cur_len, excluded);
+    smartlist_free(excluded);
+    return choice;
   }
 
   choice = router_choose_random_node(excluded, options->ExcludeNodes, flags);
@@ -2637,7 +2684,7 @@ choose_good_entry_server(uint8_t purpose, cpath_build_state_t *state,
     /* This request is for an entry server to use for a regular circuit,
      * and we use entry guard nodes.  Just return one of the guard nodes.  */
     tor_assert(guard_state_out);
-    return guards_choose_guard(state, guard_state_out);
+    return guards_choose_guard(state, purpose, guard_state_out);
   }
 
   excluded = smartlist_new();
@@ -2680,7 +2727,7 @@ onion_next_hop_in_cpath(crypt_path_t *cpath)
  * Return 1 if the path is complete, 0 if we successfully added a hop,
  * and -1 on error.
  */
-static int
+STATIC int
 onion_extend_cpath(origin_circuit_t *circ)
 {
   uint8_t purpose = circ->base_.purpose;

src/or/circuitbuild.h

@@ -83,6 +83,13 @@ 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));
+
+STATIC int onion_extend_cpath(origin_circuit_t *circ);
+
+STATIC int
+onion_pick_cpath_exit(origin_circuit_t *circ, extend_info_t *exit_ei,
+                      int is_hs_v3_rp_circuit);
+
 #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);

src/or/entrynodes.c

@@ -118,6 +118,7 @@
 #include "circpathbias.h"
 #include "circuitbuild.h"
 #include "circuitlist.h"
+#include "circuituse.h"
 #include "circuitstats.h"
 #include "config.h"
 #include "confparse.h"
@@ -3479,12 +3480,18 @@ guards_update_all(void)
     used. */
 const node_t *
 guards_choose_guard(cpath_build_state_t *state,
-                   circuit_guard_state_t **guard_state_out)
+                    uint8_t purpose,
+                    circuit_guard_state_t **guard_state_out)
 {
   const node_t *r = NULL;
   const uint8_t *exit_id = NULL;
   entry_guard_restriction_t *rst = NULL;
-  if (state && (exit_id = build_state_get_exit_rsa_id(state))) {
+
+  /* Only apply restrictions if we have a specific exit node in mind, and only
+   * if we are not doing vanguard circuits: we don't want to apply guard
+   * restrictions to vanguard circuits. */
+  if (state && !circuit_should_use_vanguards(purpose) &&
+      (exit_id = build_state_get_exit_rsa_id(state))) {
     /* We're building to a targeted exit node, so that node can't be
      * chosen as our guard for this circuit.  Remember that fact in a
      * restriction. */

src/or/entrynodes.h

@@ -322,6 +322,7 @@ struct circuit_guard_state_t {
 /* Common entry points for old and new guard code */
 int guards_update_all(void);
 const node_t *guards_choose_guard(cpath_build_state_t *state,
+                                  uint8_t purpose,
                                   circuit_guard_state_t **guard_state_out);
 const node_t *guards_choose_dirguard(uint8_t dir_purpose,
                                      circuit_guard_state_t **guard_state_out);

src/test/test_entrynodes.c

@@ -4,6 +4,7 @@
 #include "orconfig.h"
 
 #define CIRCUITLIST_PRIVATE
+#define CIRCUITBUILD_PRIVATE
 #define STATEFILE_PRIVATE
 #define ENTRYNODES_PRIVATE
 #define ROUTERLIST_PRIVATE
@@ -14,6 +15,7 @@
 
 #include "bridges.h"
 #include "circuitlist.h"
+#include "circuitbuild.h"
 #include "config.h"
 #include "confparse.h"
 #include "crypto_rand.h"
@@ -75,6 +77,17 @@ bfn_mock_node_get_by_id(const char *id)
   return NULL;
 }
 
+/* Helper function to free a test node. */
+static void
+test_node_free(node_t *n)
+{
+  tor_free(n->rs);
+  tor_free(n->md->onion_curve25519_pkey);
+  short_policy_free(n->md->exit_policy);
+  tor_free(n->md);
+  tor_free(n);
+}
+
 /* Unittest cleanup function: Cleanup the fake network. */
 static int
 big_fake_network_cleanup(const struct testcase_t *testcase, void *ptr)
@@ -84,9 +97,7 @@ big_fake_network_cleanup(const struct testcase_t *testcase, void *ptr)
 
   if (big_fake_net_nodes) {
     SMARTLIST_FOREACH(big_fake_net_nodes, node_t *, n, {
-      tor_free(n->rs);
+      test_node_free(n);
-      tor_free(n->md);
-      tor_free(n);
     });
     smartlist_free(big_fake_net_nodes);
   }
@@ -114,9 +125,18 @@ big_fake_network_setup(const struct testcase_t *testcase)
 
   big_fake_net_nodes = smartlist_new();
   for (i = 0; i < N_NODES; ++i) {
+    curve25519_secret_key_t curve25519_secret_key;
+
     node_t *n = tor_malloc_zero(sizeof(node_t));
     n->md = tor_malloc_zero(sizeof(microdesc_t));
 
+    /* Generate curve25519 key for this node */
+    n->md->onion_curve25519_pkey =
+      tor_malloc_zero(sizeof(curve25519_public_key_t));
+    curve25519_secret_key_generate(&curve25519_secret_key, 0);
+    curve25519_public_key_generate(n->md->onion_curve25519_pkey,
+                                   &curve25519_secret_key);
+
     crypto_rand(n->identity, sizeof(n->identity));
     n->rs = tor_malloc_zero(sizeof(routerstatus_t));
 
@@ -136,8 +156,8 @@ big_fake_network_setup(const struct testcase_t *testcase)
     {
       char nickname_binary[8];
       crypto_rand(nickname_binary, sizeof(nickname_binary));
-      base64_encode(n->rs->nickname, sizeof(n->rs->nickname),
+      base32_encode(n->rs->nickname, sizeof(n->rs->nickname),
-                    nickname_binary, sizeof(nickname_binary), 0);
+                    nickname_binary, sizeof(nickname_binary));
     }
 
     /* Call half of the nodes a possible guard. */
@@ -145,6 +165,12 @@ big_fake_network_setup(const struct testcase_t *testcase)
       n->is_possible_guard = 1;
       n->rs->guardfraction_percentage = 100;
       n->rs->has_guardfraction = 1;
+      n->rs->is_possible_guard = 1;
+    }
+
+    /* Make some of these nodes a possible exit */
+    if (i % 7 == 0) {
+      n->md->exit_policy = parse_short_policy("accept 443");
     }
 
     smartlist_add(big_fake_net_nodes, n);
@@ -1076,9 +1102,7 @@ test_entry_guard_expand_sample_small_net(void *arg)
   /* Fun corner case: not enough guards to make up our whole sample size. */
   SMARTLIST_FOREACH(big_fake_net_nodes, node_t *, n, {
     if (n_sl_idx >= 15) {
-      tor_free(n->rs);
+      test_node_free(n);
-      tor_free(n->md);
-      tor_free(n);
       SMARTLIST_DEL_CURRENT(big_fake_net_nodes, n);
     } else {
       n->rs->addr = 0; // make the filter reject this.
@@ -1172,9 +1196,7 @@ test_entry_guard_update_from_consensus_status(void *arg)
     entry_guard_t *g = smartlist_get(gs->sampled_entry_guards, 5);
     node_t *n = (node_t*) bfn_mock_node_get_by_id(g->identity);
     smartlist_remove(big_fake_net_nodes, n);
-    tor_free(n->rs);
+    test_node_free(n);
-    tor_free(n->md);
-    tor_free(n);
   }
   update_approx_time(start + 300);
   sampled_guards_update_from_consensus(gs);
@@ -2805,6 +2827,161 @@ test_entry_guard_outdated_dirserver_exclusion(void *arg)
   }
 }
 
+/** Test helper to extend the <b>oc</b> circuit path <b>n</b> times and then
+ *  ensure that the circuit is now complete. */
+static void
+helper_extend_circuit_path_n_times(origin_circuit_t *oc, int n)
+{
+  int retval;
+  int i;
+
+  /* Extend path n times */
+  for (i = 0 ; i < n ; i++) {
+    retval = onion_extend_cpath(oc);
+    tt_int_op(retval, OP_EQ, 0);
+    tt_int_op(circuit_get_cpath_len(oc), OP_EQ, i+1);
+  }
+
+  /* Now do it one last time and see that circ is complete */
+  retval = onion_extend_cpath(oc);
+  tt_int_op(retval, OP_EQ, 1);
+
+ done:
+  ;
+}
+
+/** Test for basic Tor path selection. Makes sure we build 3-hop circuits. */
+static void
+test_entry_guard_basic_path_selection(void *arg)
+{
+  (void) arg;
+
+  int retval;
+
+  /* Enable entry guards */
+  or_options_t *options = get_options_mutable();
+  options->UseEntryGuards = 1;
+
+  /* disables /16 check since all nodes have the same addr... */
+  options->EnforceDistinctSubnets = 0;
+
+  /* Create our circuit */
+  circuit_t *circ = dummy_origin_circuit_new(30);
+  origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ);
+  oc->build_state = tor_malloc_zero(sizeof(cpath_build_state_t));
+
+  /* First pick the exit and pin it on the build_state */
+  retval = onion_pick_cpath_exit(oc, NULL, 0);
+  tt_int_op(retval, OP_EQ, 0);
+
+  /* Extend path 3 times. First we pick guard, then middle, then exit. */
+  helper_extend_circuit_path_n_times(oc, 3);
+
+ done:
+  circuit_free_(circ);
+}
+
+/** Test helper to build an L2 and L3 vanguard list. The vanguard lists
+ *  produced should be completely disjoint. */
+static void
+helper_setup_vanguard_list(or_options_t *options)
+{
+  int i = 0;
+
+  /* Add some nodes to the vanguard L2 list */
+  options->HSLayer2Nodes = routerset_new();
+  for (i = 0; i < 10 ; i += 2) {
+    node_t *vanguard_node = smartlist_get(big_fake_net_nodes, i);
+    tt_assert(vanguard_node->is_possible_guard);
+    routerset_parse(options->HSLayer2Nodes, vanguard_node->rs->nickname, "l2");
+  }
+  /* also add some nodes to vanguard L3 list
+   * (L2 list and L3 list should be disjoint for this test to work) */
+  options->HSLayer3Nodes = routerset_new();
+  for (i = 10; i < 20 ; i += 2) {
+    node_t *vanguard_node = smartlist_get(big_fake_net_nodes, i);
+    tt_assert(vanguard_node->is_possible_guard);
+    routerset_parse(options->HSLayer3Nodes, vanguard_node->rs->nickname, "l3");
+  }
+
+ done:
+  ;
+}
+
+/** Test to ensure that vanguard path selection works properly.  Ensures that
+ *  default vanguard circuits are 4 hops, and that path selection works
+ *  correctly given the vanguard settings. */
+static void
+test_entry_guard_vanguard_path_selection(void *arg)
+{
+  (void) arg;
+
+  int retval;
+
+  /* Enable entry guards */
+  or_options_t *options = get_options_mutable();
+  options->UseEntryGuards = 1;
+
+  /* XXX disables /16 check */
+  options->EnforceDistinctSubnets = 0;
+
+  /* Setup our vanguard list */
+  helper_setup_vanguard_list(options);
+
+  /* Create our circuit */
+  circuit_t *circ = dummy_origin_circuit_new(30);
+  origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ);
+  oc->build_state = tor_malloc_zero(sizeof(cpath_build_state_t));
+  oc->build_state->is_internal = 1;
+
+  /* Switch circuit purpose to vanguards */
+  circ->purpose = CIRCUIT_PURPOSE_HS_VANGUARDS;
+
+  /* First pick the exit and pin it on the build_state */
+  tt_int_op(oc->build_state->desired_path_len, OP_EQ, 0);
+  retval = onion_pick_cpath_exit(oc, NULL, 0);
+  tt_int_op(retval, OP_EQ, 0);
+
+  /* Ensure that vanguards make 4-hop circuits by default */
+  tt_int_op(oc->build_state->desired_path_len, OP_EQ, 4);
+
+  /* Extend path as many times as needed to have complete circ. */
+  helper_extend_circuit_path_n_times(oc, oc->build_state->desired_path_len);
+
+  /* Test that the cpath linked list is set correctly. */
+  crypt_path_t *l1_node = oc->cpath;
+  crypt_path_t *l2_node = l1_node->next;
+  crypt_path_t *l3_node = l2_node->next;
+  crypt_path_t *l4_node = l3_node->next;
+  crypt_path_t *l1_node_again = l4_node->next;
+  tt_ptr_op(l1_node, OP_EQ, l1_node_again);
+
+  /* Test that L2 is indeed HSLayer2Node */
+  retval = routerset_contains_extendinfo(options->HSLayer2Nodes,
+                                         l2_node->extend_info);
+  tt_int_op(retval, OP_EQ, 4);
+  /* test that L3 node is _not_ contained in HSLayer2Node */
+  retval = routerset_contains_extendinfo(options->HSLayer2Nodes,
+                                         l3_node->extend_info);
+  tt_int_op(retval, OP_LT, 4);
+
+  /* Test that L3 is indeed HSLayer3Node */
+  retval = routerset_contains_extendinfo(options->HSLayer3Nodes,
+                                         l3_node->extend_info);
+  tt_int_op(retval, OP_EQ, 4);
+  /* test that L2 node is _not_ contained in HSLayer3Node */
+  retval = routerset_contains_extendinfo(options->HSLayer3Nodes,
+                                         l2_node->extend_info);
+  tt_int_op(retval, OP_LT, 4);
+
+  /* TODO: Test that L1 can be the same as exit. To test this we need start
+     enforcing EnforceDistinctSubnets again, which means that we need to give
+     each test node a different address which currently breaks some tests. */
+
+ done:
+  circuit_free_(circ);
+}
+
 static const struct testcase_setup_t big_fake_network = {
   big_fake_network_setup, big_fake_network_cleanup
 };
@@ -2868,6 +3045,8 @@ struct testcase_t entrynodes_tests[] = {
   BFN_TEST(select_and_cancel),
   BFN_TEST(drop_guards),
   BFN_TEST(outdated_dirserver_exclusion),
+  BFN_TEST(basic_path_selection),
+  BFN_TEST(vanguard_path_selection),
 
   UPGRADE_TEST(upgrade_a_circuit, "c1-done c2-done"),
   UPGRADE_TEST(upgrade_blocked_by_live_primary_guards, "c1-done c2-done"),