Merge branch 'maint-0.3.1' into release-0.3.1

6 files changed, 70 insertions, 8 deletions

diff --git a/changes/bug22753 b/changes/bug22753
new file mode 100644
index 0000000000..32a6dfa56c
--- /dev/null
+++ b/changes/bug22753
@@ -0,0 +1,7 @@
+  o Major bugfixes (path selection, security):
+    - When choosing which guard to use for a circuit, avoid the
+      exit's family along with the exit itself. Previously, the new
+      guard selection logic avoided the exit, but did not consider
+      its family.  Fixes bug 22753; bugfix on 0.3.0.1-alpha. Tracked
+      as TROVE-2016-006 and CVE-2017-0377.
+
diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c
index be9f85a89f..fa768fc4a6 100644
--- a/src/or/entrynodes.c
+++ b/src/or/entrynodes.c
@@ -1428,6 +1428,38 @@ entry_guard_passes_filter(const or_options_t *options, guard_selection_t *gs,
   }
 }
 
+/** Return true iff <b>guard</b> is in the same family as <b>node</b>.
+ */
+static int
+guard_in_node_family(const entry_guard_t *guard, const node_t *node)
+{
+  const node_t *guard_node = node_get_by_id(guard->identity);
+  if (guard_node) {
+    return nodes_in_same_family(guard_node, node);
+  } else {
+    /* If we don't have a node_t for the guard node, we might have
+     * a bridge_info_t for it. So let's check to see whether the bridge
+     * address matches has any family issues.
+     *
+     * (Strictly speaking, I believe this check is unnecessary, since we only
+     * use it to avoid the exit's family when building circuits, and we don't
+     * build multihop circuits until we have a routerinfo_t for the
+     * bridge... at which point, we'll also have a node_t for the
+     * bridge. Nonetheless, it seems wise to include it, in case our
+     * assumptions change down the road.  -nickm.)
+     */
+    if (get_options()->EnforceDistinctSubnets && guard->bridge_addr) {
+      tor_addr_t node_addr;
+      node_get_addr(node, &node_addr);
+      if (addrs_in_same_network_family(&node_addr,
+                                       &guard->bridge_addr->addr)) {
+        return 1;
+      }
+    }
+    return 0;
+  }
+}
+
 /**
  * Return true iff <b>guard</b> obeys the restrictions defined in <b>rst</b>.
  * (If <b>rst</b> is NULL, there are no restrictions.)
@@ -1440,7 +1472,12 @@ entry_guard_obeys_restriction(const entry_guard_t *guard,
   if (! rst)
     return 1; // No restriction?  No problem.
 
-  // Only one kind of restriction exists right now
+  // Only one kind of restriction exists right now: excluding an exit
+  // ID and all of its family.
+  const node_t *node = node_get_by_id((const char*)rst->exclude_id);
+  if (node && guard_in_node_family(guard, node))
+    return 0;
+
   return tor_memneq(guard->identity, rst->exclude_id, DIGEST_LEN);
 }
 
diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h
index 11b618bb50..735c7738ba 100644
--- a/src/or/entrynodes.h
+++ b/src/or/entrynodes.h
@@ -276,16 +276,17 @@ struct entry_guard_handle_t;
  * A restriction to remember which entry guards are off-limits for a given
  * circuit.
  *
- * Right now, we only use restrictions to block a single guard from being
- * selected; this mechanism is designed to be more extensible in the future,
- * however.
+ * Right now, we only use restrictions to block a single guard and its family
+ * from being selected; this mechanism is designed to be more extensible in
+ * the future, however.
  *
  * Note: This mechanism is NOT for recording which guards are never to be
  * used: only which guards cannot be used on <em>one particular circuit</em>.
  */
 struct entry_guard_restriction_t {
   /**
-   * The guard's RSA identity digest must not equal this.
+   * The guard's RSA identity digest must not equal this; and it must not
+   * be in the same family as any node with this digest.
    */
   uint8_t exclude_id[DIGEST_LEN];
 };
diff --git a/src/or/nodelist.c b/src/or/nodelist.c
index 0ef9741243..3ac5c3e302 100644
--- a/src/or/nodelist.c
+++ b/src/or/nodelist.c
@@ -1332,7 +1332,7 @@ nodelist_refresh_countries(void)
 
 /** Return true iff router1 and router2 have similar enough network addresses
  * that we should treat them as being in the same family */
-static inline int
+int
 addrs_in_same_network_family(const tor_addr_t *a1,
                              const tor_addr_t *a2)
 {
diff --git a/src/or/nodelist.h b/src/or/nodelist.h
index 6c063de8a3..95ae778a5b 100644
--- a/src/or/nodelist.h
+++ b/src/or/nodelist.h
@@ -93,6 +93,8 @@ int node_is_unreliable(const node_t *router, int need_uptime,
 int router_exit_policy_all_nodes_reject(const tor_addr_t *addr, uint16_t port,
                                         int need_uptime);
 void router_set_status(const char *digest, int up);
+int addrs_in_same_network_family(const tor_addr_t *a1,
+                                 const tor_addr_t *a2);
 
 /** router_have_minimum_dir_info tests to see if we have enough
  * descriptor information to create circuits.
diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c
index 3db7e63ee3..1f008d93b3 100644
--- a/src/test/test_entrynodes.c
+++ b/src/test/test_entrynodes.c
@@ -121,6 +121,8 @@ big_fake_network_setup(const struct testcase_t *testcase)
 
     n->is_running = n->is_valid = n->is_fast = n->is_stable = 1;
 
+    /* Note: all these guards have the same address, so you'll need to
+     * disable EnforceDistinctSubnets when a restriction is applied. */
     n->rs->addr = 0x04020202;
     n->rs->or_port = 1234;
     n->rs->is_v2_dir = 1;
@@ -1846,14 +1848,17 @@ test_entry_guard_select_for_circuit_confirmed(void *arg)
   tt_uint_op(state, OP_EQ, GUARD_CIRC_STATE_USABLE_IF_NO_BETTER_GUARD);
   tt_i64_op(g2->last_tried_to_connect, OP_EQ, approx_time());
 
-  // If we say that the next confirmed guard in order is excluded, we get
-  // The one AFTER that.
+  // If we say that the next confirmed guard in order is excluded, and
+  // we disable EnforceDistinctSubnets, we get the guard AFTER the
+  // one we excluded.
+  get_options_mutable()->EnforceDistinctSubnets = 0;
   g = smartlist_get(gs->confirmed_entry_guards,
                      smartlist_len(gs->primary_entry_guards)+2);
   entry_guard_restriction_t rst;
   memset(&rst, 0, sizeof(rst));
   memcpy(rst.exclude_id, g->identity, DIGEST_LEN);
   g2 = select_entry_guard_for_circuit(gs, GUARD_USAGE_TRAFFIC, &rst, &state);
+  tt_ptr_op(g2, OP_NE, NULL);
   tt_ptr_op(g2, OP_NE, g);
   tt_int_op(g2->confirmed_idx, OP_EQ,
             smartlist_len(gs->primary_entry_guards)+3);
@@ -1873,6 +1878,16 @@ test_entry_guard_select_for_circuit_confirmed(void *arg)
   tt_assert(g->is_pending);
   tt_int_op(g->confirmed_idx, OP_EQ, -1);
 
+  // If we EnforceDistinctSubnets and apply a restriction, we get
+  // nothing, since we put all of the nodes in the same /16.
+  // Regression test for bug 22753/TROVE-2017-006.
+  get_options_mutable()->EnforceDistinctSubnets = 1;
+  g = smartlist_get(gs->confirmed_entry_guards, 0);
+  memset(&rst, 0, sizeof(rst));
+  memcpy(rst.exclude_id, g->identity, DIGEST_LEN);
+  g2 = select_entry_guard_for_circuit(gs, GUARD_USAGE_TRAFFIC, &rst, &state);
+  tt_ptr_op(g2, OP_EQ, NULL);
+
  done:
   guard_selection_free(gs);
 }