[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #31541 [Core Tor/Tor]: hs-v3: Client can re-pick bad intro points
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs