[tor-bugs] #21302 [Core Tor/Tor]: hs: Multiple service issues

[Tor Bug Tracker & Wiki]

#21302: hs: Multiple service issues
------------------------------+--------------------------------
     Reporter:  dgoulet       |      Owner:
         Type:  defect        |     Status:  new
     Priority:  Medium        |  Milestone:  Tor: 0.3.0.x-final
    Component:  Core Tor/Tor  |    Version:  Tor: 0.2.9.8
     Severity:  Normal        |   Keywords:  tor-hs
Actual Points:                |  Parent ID:
       Points:                |   Reviewer:
      Sponsor:                |
------------------------------+--------------------------------
 While investigating #21297, I found several issues. Unfortunately, I can't
 find the cause of #21297 yet but maybe someone will with that list of
 issues.

 1) Integer overflow in `rend_consider_services_intro_points()`. We have a
 safe guard against that *except* if the `expiring_nodes` list is not empty
 which is totally a possibility. This is bad because it will ultimately
 create a LOT of IP circuits and fail in unknown ways...

 {{{
     if (smartlist_len(service->expiring_nodes) == 0 &&
         intro_nodes_len >= service->n_intro_points_wanted) {
       continue;
     }

     /* Number of intro points we want to open which is the wanted amount
      * minus the current amount of valid nodes. */
     n_intro_points_to_open = service->n_intro_points_wanted -
 intro_nodes_len;
 }}}

 2) In `rend_service_intro_has_opened()`, this if is subject to a bad "cast
 overflow" if we have a case that brings the `expiring_nodes` size bigger
 than the number of circuits. The following result is cast as an `unsigned
 int` so for instance "5 - 6" is actually a BIG number.

 {{{
   if ((count_intro_point_circuits(service) -
        smartlist_len(service->expiring_nodes)) >
       service->n_intro_points_wanted) {
 }}}

 The consequence of this is that if we have a bug (maybe #21297!!) that
 brings the size of `expiring_nodes` to 6 and then we try to open 5 IP
 circuits for the service (3 + 2 extras), all 5 will always be closed and
 it will loop non stop.

 3) In `remove_invalid_intro_points()`, if we put an IP object in the
 `retry_nodes` list and this IP also expires, it will be put in the
 `expiring_nodes` list of the service and *removed* from the service valid
 list. After this, we'll retry a circuit to that IP object but since it's
 not in the valid list anymore, the `rend_consider_services_intro_points()`
 will launch a bunch of other IP circuit(s) ignoring the one being retried.
 This is OK because the one we want to retry should actually expire BUT we
 could avoid a useless circuit being created since we know it will expire
 and potential bug of losing the reference to that circuit if we have
 issues with cleaning up our `expiring_nodes` list.

 My theory is that the solution to #21297 lies in a combination of the
 above bugs. Either we overflow with (2) or we have IP circuits that we've
 lost the reference to (1) making (2) always closing the 5 circuits we just
 opened and thus not having any valid IP object for the service. I don't
 think (1) is involved but you never know.

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