BridgeDB distributors do not handle time intervals correctly

Reported via arma on tor-assistants@lists.torproject.org, via a report from Francisco on IRC. Thank you to both of you!

Fix is done. Details forthcoming. I wanted a ticket to reference in the branch name, and to keep track of which version(s) it needs to get backported to.

added bridgedb-0.2.2 bridgedb-https component::circumvention/bridgedb owner::isis priority::very high resolution::fixed security status::closed type::defect labels

Fixed in my hotfix/11215_12147-intervalstart branch. Deployed in version 0.2.2.

The commit message explains more about the nature and effect of this bug:

    Call schedule.intervalStart() to get epoch for HTTPS bridge request.
    
    The ``epoch`` of a request is a value that is supposed to be the
    interval of time which the request occurred within, i.e. a request at
    14:18 is in the 10-minute interval of 14:10-14:20. This ``epoch`` is
    used to obtain bridges in response to a client's request, specifically,
    it's a parameter to the ``bridgedb.Dist.getBridgesForIP()`` method,
    which does all the real work.
    
    In implementation (up until a couple weeks ago), there was an odd thing
    in that a request's ``epoch`` was always hardcoded to be ``"1970"``. I
    changed the part which returns ``"1970"`` to return an ISO-8601
    timestamp, under the assumtion that anything asking for an interval
    would use the ``intervalStart()`` or ``nextIntervalStarts()`` methods to
    compare the curr ent timestamp to the interval it should reside
    within. My assumption was wrong; in ``bridgedb.Dist.getBridgesForIP()``,
    in the first line of that method, ``schedule.getInterval()`` is called
    instead. I had even made an XXX note a long time ago stating that this
    was a dumb thing to do. I forgot to change it. Oops.
    
    The fix is to change the first line of
    ``bridgedb.Dist.getBridgesForIP()`` from ``self.schedule.getInterval()``
    to ``self.schedule.intervalStarts()` `.  This was also preventing the
    CAPTCHA expiration from functioning correctly.
    
    After making this change, it exhibits the correct behaviour, which is,
    first, to only respond after determining that we're within the 10-minute
    interval in which the CAPTCHA was issued, and second, determine if the
    solution to the CATPCHA is correct (and if so give the bridges that we
    would give to that IP address cluster, ignoring time intervals
    altogether).
    
     * FIXES #12147
     * THANKS to arma for forwarding to the original bug report to
       tor-assistants@lists.torproject.org.
     * THANKS TO Francisco on IRC for discovering and reporting the issue.

Trac:
Status: new to closed
Resolution: N/A to fixed

This patch may not go far enough and the resolution may need further investigation.

Trac:
Status: closed to reopened
Resolution: fixed to N/A

Providing some clarification, this affects the email distributor as much as it affects the http distributor (though the overall impact on the captcha system is worse). The email distributor also calls getInterval(). This should also be patched.

Trac:
Summary: BridgeDB bridge requests over HTTPS have another timeout issue to BridgeDB distributors do not handle time intervals correctly

Robert Ransom supplied a patch, adding the following unittest to check for this issue:

commit a9e65a0c9c3e40359731b6fe3408e068f9c1501a
Author:     Robert Ransom <rransom.8774@gmail.com>
AuthorDate: Tue Jul 15 04:04:46 2014 -0400
Commit:     Isis Lovecruft <isis@torproject.org>
CommitDate: Wed Jul 16 01:55:32 2014 +0000

    Fix test suite for bridgedb.scheduled.Unscheduled
    
    Previously, the test suite checked only that Unscheduled.getInterval would
    return a particular value when invoked with no argument.  Now, the test
    suite checks the only property of Unscheduled.getInterval that its callers
    rely on: that it returns the same string regardless of which time is passed
    in to it.

diff --git a/lib/bridgedb/test/test_schedule.py b/lib/bridgedb/test/test_schedule.py
index 24c45ef..178e3e4 100644
--- a/lib/bridgedb/test/test_schedule.py
+++ b/lib/bridgedb/test/test_schedule.py
@@ -36,10 +36,21 @@ class UnscheduledTests(unittest.TestCase):
         self.assertIsInstance(time, int)
         self.assertEquals(time, -62135596800)
 
-    def test_Unscheduled_getInterval_noargs(self):
-        time = self.sched.getInterval()
-        self.assertIsInstance(time, str)
-        self.assertEquals(time, "1970-01-01 00:00:00")
+    def test_Unscheduled_getInterval_is_constant(self):
+        import time
+        now = time.time()
+
+        interval_default = self.sched.getInterval()
+        self.assertIsInstance(interval_default, str)
+
+        interval_zero = self.sched.getInterval(0)
+        self.assertIsInstance(interval_zero, str)
+
+        interval_now = self.sched.getInterval(now)
+        self.assertIsInstance(interval_now, str)
+
+        self.assertEquals(interval_default, interval_zero)
+        self.assertEquals(interval_default, interval_now)
 
     def test_Unscheduled_nextIntervalStarts_noargs(self):
         time = self.sched.nextIntervalStarts()

I subsequently committed the following trivial fix to effectively ignore the when parameter in bridgedb.schedule.Unscheduled.getInterval():

commit a7551e22cc141d48bd7498afcc07d4378c0c0fcc (tpo-isis/rransom/fix/12147-schedule-unittest, isislovecruft/rransom/fix/12147-schedule-unittest, greyarea/rransom/fix/12147-schedule-unittest, rransom/fix/12147-sc
Author:     Isis Lovecruft <isis@torproject.org>
AuthorDate: Wed Jul 16 04:13:30 2014 +0000
Commit:     Isis Lovecruft <isis@torproject.org>
CommitDate: Wed Jul 16 04:13:30 2014 +0000

    Fix bug shown in test_Unscheduled_getInterval_is_constant() unittest.
    
     * FIXES an additional problem for #12147 pointed out by Robert Ransom's
       `bridgedb.test.test_Unscheduled_getInterval_is_constant()` unittest.

diff --git a/lib/bridgedb/schedule.py b/lib/bridgedb/schedule.py
index 6cef1a6..930f2e7 100644
--- a/lib/bridgedb/schedule.py
+++ b/lib/bridgedb/schedule.py
@@ -91,6 +91,10 @@ class Unscheduled(object):
     def getInterval(self, when=0):
         """Get the interval that contains the time **when**.
 
+        .. note: We explicitly ignore the ``when`` parameter in this
+            implementation because if something is Unscheduled then
+            all timestamps should reside within the same period.
+
         :param int when: The time which we're trying to find the corresponding
             interval for.
         :rtype: str
@@ -98,8 +102,9 @@ class Unscheduled(object):
             specificity depends on what type of interval we're using. For
             example, if using ``"month"``, the return value would be something
             like ``"2013-12"``.
         """
-        return fromUnixSeconds(when).strftime('%04Y-%02m-%02d %02H:%02M:%02S')
+        return fromUnixSeconds(0).strftime('%04Y-%02m-%02d %02H:%02M:%02S')
 
     def nextIntervalStarts(self, when=0):
         """Return the start time of the interval starting _after_ when.

This should be fixed. For good.

Trac:
Resolution: N/A to fixed
Status: reopened to closed

closed

moved to tpo/anti-censorship/bridgedb#12147 (closed)