[tor-bugs] #4581 [Tor]: Dir auths should defend themselves from too many begindir requests per address

[Tor Bug Tracker & Wiki]

#4581: Dir auths should defend themselves from too many begindir requests per
address
-------------------------------------------------+-------------------------
 Reporter:  arma                                 |          Owner:  andrea
     Type:  defect                               |         Status:
 Priority:  High                                 |  needs_revision
Component:  Tor                                  |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.8.x-final
 Keywords:  maybe-proposal, tor-auth,            |        Version:  Tor:
  027-triaged-1-in, TorCoreTeam201509,           |  0.2.7
  028-triaged                                    |     Resolution:
Parent ID:                                       |  Actual Points:
  Sponsor:  SponsorU                             |         Points:  medium
-------------------------------------------------+-------------------------
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 * In dirdosfilter.c
         - dirdosfilter_bump_anon_dirport() has an unsused param:
 `dst_port`. Same
           for dirdosfilter_bump_direct(). Same goes for `dst_addr`.
         - dirdosfilter_bump_direct() also doesn't use `dst_addr`.
         - Most logging statement cast uint32_t to unsigned int which is
 fine in
           terms of size but we should either use a `U32_PRINTF_ARG` or
 `PRIu32`
           from inttypes.h (that is already included).
         - dirdosfilter_bump_circuit_begindir(): Is it possible to _not_
 have a
           circuit there? Maybe an assert() is a bit too agressive (do not
 know) but
           if we should have a circuit and we do not, I would put a BUG log
 since
           BEGIN_DIR without circuit seems weird. Same goes for no channel,
 since
           `circ->p_chan->global_identifier` is used, if we can't find the
 channel,
           I would be surprised...
         - dirdosfilter_set_time_constant(): Could be nothing but this is
 used by
           config.c to update the constant. Seems weird though to update
 all
           counters _before_ we change the `dirdosfilter_lambda`. Shouldn't
 we
           update lambda and then update all counters to fit it's new
 value? If not,
           a small comment on explaining that the effect of this function
 is to
           simply change the constant and "at some point in time" counters
 will be
           updated using the new value.

 * In main.c
         - `time_t dirdosfilter_ht_compact` is not initialized in `time_to`
 just below
         - Probably a typo, it should be `COMPACT_DIRDOSFILTER_HT_INTERVAL`
 and
         `time_to.dirdosfilter_ht_compact` instead:
         {{{
             time_to.retry_dns_init = now + RETRY_DNS_INTERVAL;
         }}}

 * I just want to discuss the choice of the torrc DirDosFilter values.
 There is
   nothing in the proposal about those, explaining why we chose "2.0" for
   `DirDoSFilterMaxDirectConnRatePerIP` or 32 for DirPort connection rate.
 Would
   be nice to have an idea on why we think 32 connections for instance is a
 good
   high limit or it's just "we do not know, let's try with this".

 * I see that you are working on tests, great! This directly impacts
 directory
   authority which is the heart and brain of the network so the more the
 merrier! :)

 That's it for now.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/4581#comment:27>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online