[tor-bugs] #31541 [Core Tor/Tor]: hs-v3: Client can re-pick bad intro points

[Tor Bug Tracker & Wiki]

#31541: hs-v3: Client can re-pick bad intro points
-------------------------------------------------+-------------------------
 Reporter:  dgoulet                              |          Owner:  dgoulet
     Type:  defect                               |         Status:
                                                 |  assigned
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.4.2.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Major                                |     Resolution:
 Keywords:  tor-hs tor-client 035-backport,      |  Actual Points:
  040-backport, 041-backport                     |
Parent ID:  #30200                               |         Points:  1
 Reviewer:  asn                                  |        Sponsor:
                                                 |  Sponsor27-must
-------------------------------------------------+-------------------------

Old description:

> Ok this one took me a while to figure out!
>
> This is sorta related to #25882 as it is a bug that makes the client
> retry constantly the same intro point even though it was flagged as
> having an error.
>
> Problem lies in `client_get_random_intro()` which randomly selects an
> intro point from the descriptor. Sparing the detail, this is where it
> goes wrong:
>
> {{{
>     ip = smartlist_get(usable_ips, idx);
>     smartlist_del(usable_ips, idx);
> }}}
>
> ... and then we use `ip` to check if usable and if yes, we connect to it.
>
> We can't `del()` the value from the list until we are done with the `ip`
> object. The `smartlist_get()` returns a pointer to location *in the list*
> so if we change the list order right after acquiring the reference, we
> are accessing bad things, possibly junk.
>
> So basically, junk can be used, the wrong IP can be used even though it
> would not pass the `intro_point_is_usable()` if it was correct pointer.
>
> I was able to find this using a pathological case where the HS pins its
> intro point to 3 specific nodes. So, even upon a restart or desc
> rotation, the same IPs are re-used but with different auth key.
>
> If a client had already connected to that service and thus had those IPs
> in its failure cache, it will fail to eternity to connect to the service
> because it basically never realize it needs to refetch a new descriptor.
>
> Even though there is a catch all with
> `hs_client_any_intro_points_usable()` before extending to an intro point,
> the problem lies with the above which can make the code NEVER pick a
> certain intro point so the catch all always validate to true since there
> is one intro point in the list that was never tried.
>
> This is very bad for reachability, network load, but also simple "code
> safety". I strongly recommend backport to our maintained version >= 032.
>
> Fortunately for us, the fix is trivial, we should only `del()` when done
> with the IP object.

New description:

 Ok this one took me a while to figure out!

 This is sorta related to #25882 as it is a bug that makes the client retry
 constantly the same intro point even though it was flagged as having an
 error.

 Problem lies in `client_get_random_intro()` which randomly selects an
 intro point from the descriptor. Sparing the detail, this is where it goes
 wrong:

 {{{
     ip = smartlist_get(usable_ips, idx);
     smartlist_del(usable_ips, idx);
 }}}
 ... and then we use `ip` to check if usable and if yes, we connect to it.

 ~~We can't `del()` the value from the list until we are done with the `ip`
 object. The `smartlist_get()` returns a pointer to location *in the list*
 so if we change the list order right after acquiring the reference, we are
 accessing bad things, possibly junk.~~

 ~~So basically, junk can be used, the wrong IP can be used even though it
 would not pass the `intro_point_is_usable()` if it was correct pointer.~~

 I was able to find this using a pathological case where the HS pins its
 intro point to 3 specific nodes. So, even upon a restart or desc rotation,
 the same IPs are re-used but with different auth key.

 If a client had already connected to that service and thus had those IPs
 in its failure cache, it will fail to eternity to connect to the service
 because it basically never realize it needs to refetch a new descriptor.

 Even though there is a catch all with
 `hs_client_any_intro_points_usable()` before extending to an intro point,
 the problem lies with the above which can make the code NEVER pick a
 certain intro point so the catch all always validate to true since there
 is one intro point in the list that was never tried.

 This is very bad for reachability, network load, but also simple "code
 safety". I strongly recommend backport to our maintained version >= 032.

 Fortunately for us, the fix is trivial, we should only `del()` when done
 with the IP object.

--

Comment (by dgoulet):

 Indeed, I actually was mistaken there and re-tested it much more. Even
 though I move the `del()`, it can still happen. As an example of what I
 see (with extra logging I added):

 {{{
 Aug 28 07:57:02.571 [info] handle_introduce_ack_bad(): Received
 INTRODUCE_ACK nack by $27F3833453C4006DF1E21C6BF62E4FCD8E99DEF2~ at
 66.70.207.168 [tEKumka0yCHtYKKWcUa07RbmwV+C1eZ3HCHWRtk/5RI]. Reason: 1
 ...
 Aug 28 07:57:02.571 [warn] IP Picked:
 UHOg6T4ZYjJWX5t134V2BMVu3JLshZNWu8/lQptUBFM
 Aug 28 07:57:02.571 [info] hs_client_reextend_intro_circuit(): Re-
 extending circ 2711850788, this time to
 $27F3833453C4006DF1E21C6BF62E4FCD8E99DEF2~ at 66.70.207.168.
 }}}

 Notice that we are re-extending to the same IP that just NACK us even
 though the `client_get_random_intro()` picked another auth key
 (`UHOg6T4ZYjJWX5t134V2BMVu3JLshZNWu8/lQptUBFM`).

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