Remove remaining timing-dependency in choosing nodes by bandwidth

The old approach, because of its "tmp >= rand_bw &&
!i_has_been_chosen" check, would run through the second part of the
loop slightly slower than the first part.  Now, we remove
i_has_been_chosen, and instead set rand_bw = UINT64_MAX, so that
every instance of the loop will do exactly the same amount of work
regardless of the initial value of rand_bw.

Fix for bug 6538.

diff --git a/changes/bug6538 b/changes/bug6538
index 1e882eb1cce11aa49e4ff1b9ec9ed58df6686b77..fc9e583d52a0290f3544e8371e043c98760eb536 100644 (file)
--- a/changes/bug6538
+++ b/changes/bug6538
@@ -2,3 +2,11 @@
     - Switch weighted node selection rule from using a list of doubles
       to using a list of int64_t. This should make the process slightly
       easier to debug and maintain. Needed for fix for bug 6538.
+
+  o Security features:
+    - Switch to a completely time-invariant approach for picking nodes
+      weighted by bandwidth. Our old approach would run through the
+      part of the loop after it had made its choice slightly slower
+      than it ran through the part of the loop before it had made its
+      choice. Fix for bug 6538.
+

diff --git a/src/or/routerlist.c b/src/or/routerlist.c
index 382d45746c0b4b3a122bba371855b43129a13ae0..75cb3f2e69fdb07e25672223564d312d2c85ef1a 100644 (file)
--- a/src/or/routerlist.c
+++ b/src/or/routerlist.c
@@ -1710,7 +1710,6 @@ smartlist_choose_node_by_bandwidth_weights(smartlist_t *sl,
   uint64_t tmp;
   unsigned int i;
   unsigned int i_chosen;
-  unsigned int i_has_been_chosen;
   int have_unknown = 0; /* true iff sl contains element not in consensus. */
 
   /* Can't choose exit and guard at same time */
@@ -1881,13 +1880,12 @@ smartlist_choose_node_by_bandwidth_weights(smartlist_t *sl,
 
   /* Last, count through sl until we get to the element we picked */
   i_chosen = (unsigned)smartlist_len(sl);
-  i_has_been_chosen = 0;
   tmp = 0;
   for (i=0; i < (unsigned)smartlist_len(sl); i++) {
     tmp += bandwidths[i];
-    if (tmp >= rand_bw && !i_has_been_chosen) {
+    if (tmp >= rand_bw) {
       i_chosen = i;
-      i_has_been_chosen = 1;
+      rand_bw = UINT64_MAX;
     }
   }
   i = i_chosen;
@@ -1926,7 +1924,6 @@ smartlist_choose_node_by_bandwidth(smartlist_t *sl,
 {
   unsigned int i;
   unsigned int i_chosen;
-  unsigned int i_has_been_chosen;
   int32_t *bandwidths;
   int is_exit;
   int is_guard;
@@ -2127,7 +2124,6 @@ smartlist_choose_node_by_bandwidth(smartlist_t *sl,
   /* Last, count through sl until we get to the element we picked */
   tmp = 0;
   i_chosen = (unsigned)smartlist_len(sl);
-  i_has_been_chosen = 0;
   for (i=0; i < (unsigned)smartlist_len(sl); i++) {
     is_exit = bitarray_is_set(exit_bits, i);
     is_guard = bitarray_is_set(guard_bits, i);
@@ -2142,9 +2138,9 @@ smartlist_choose_node_by_bandwidth(smartlist_t *sl,
     else
       tmp += bandwidths[i];
 
-    if (tmp >= rand_bw && !i_has_been_chosen) {
+    if (tmp >= rand_bw) {
       i_chosen = i;
-      i_has_been_chosen = 1;
+      rand_bw = UINT64_MAX;
     }
   }
   i = i_chosen;