[tor-bugs] #3825 [Tor Hidden Services]: HS intro points overloaded with CREATE cells cause connectivity failures

[Tor Bug Tracker & Wiki]

#3825: HS intro points overloaded with CREATE cells cause connectivity failures
---------------------------------+------------------------------------------
 Reporter:  atoruser             |          Owner:  rransom           
     Type:  defect               |         Status:  needs_review      
 Priority:  major                |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor Hidden Services  |        Version:  Tor: unspecified  
 Keywords:                       |         Parent:                    
   Points:                       |   Actualpoints:                    
---------------------------------+------------------------------------------

Comment(by rransom):

 Replying to [comment:28 hellais]:

 > > Sorry.  That isn't the formula I meant to use there.
 > >
 > > See my bug3825c branch for a fix.

 > Ok. That makes *much* more sense. Though I am still dubious about the
 1.5 factor and if that is a good usage factor.

 The 1.5 is meant to bias it toward more intro points rather than less.

 > Another thing that might be a problem is that NUM_INTRO_POINTS_MAX is
 not being used to compute the *total* maximum number of IP, but rather the
 maximum number of IP to be rebuilt once one dies.
 >
 > This means that I could potentially overload a set of IP's and force
 them to always recreate 10 new ones and therefore lead to a very big
 number of IP (theoretically infinite).

 I clamp `n_intro_points_wanted_to_replace_this_one` to a maximum of
 `NUM_INTRO_POINT_MAX` only to avoid integer overflow when
 `n_intro_points_wanted_now` is updated.  `n_intro_points_wanted_now` is
 also clamped to between `NUM_INTRO_POINTS_DEFAULT` and
 `NUM_INTRO_POINTS_MAX`, so no HS will establish more than a total of
 `NUM_INTRO_POINTS_MAX` intro points at a time.

 > Is this in your intentions? I believe we should pick 60 as the maximum
 number of IP that a HS is connected to (for reasoning of this choice check
 out: #4862).

 60 is too many.  The point of using 10 intro points is to ensure that the
 load is spread out enough to not overload the intro-point relays and cause
 buggy misdesigned clients to keep pounding the intro-point relays so they
 stay overloaded.  If that client handle-congestion-by-DDoSing bug didn't
 exist, 6 intro points would be enough for any HS, because any client load
 capable of overloading the intro points would also overload the service's
 6 HSDir nodes.

 (If a service maintains too many intro points, it will load its HSDir
 nodes more by republishing its descriptor more often.  In theory, we could
 put only a subset of a service's intro points into each of its 6
 descriptors, but that would involve far more extensive changes than I want
 to implement unless we have evidence that they're really necessary.)

 > line 1001 should read instead:
 >
 > - if (n_intro_points_wanted_now < NUM_INTRO_POINTS_DEFAULT) {
 > + if ((current_intro_point_countr + n_intro_points_wanted_now) <
 NUM_INTRO_POINTS_DEFAULT) {
 >
 > Do you agree?

 No.  `n_intro_points_wanted_now` is the new total number of intro points
 for the service to maintain:
 {{{
     log_info(LD_REND, "Replacing closing intro point for service %s "
              "with %d new intro points (wanted %g replacements); "
              "service will now try to have %u intro points",
              rend_service_describe_for_log(service),
              n_intro_points_really_replacing_this_one,
              fractional_n_intro_points_wanted_to_replace_this_one,
              n_intro_points_really_wanted_now);

     service->n_intro_points_wanted = n_intro_points_really_wanted_now;
 }}}

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