Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#31541 closed defect (invalid)

hs-v3: Client can re-pick bad intro points

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Major Keywords: tor-hs tor-client
Cc: Actual Points:
Parent ID: #30200 Points: 1
Reviewer: asn Sponsor: Sponsor27-must

Description (last modified by dgoulet)

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.

Child Tickets

Change History (6)

comment:1 Changed 14 months ago by dgoulet

Keywords: 035-backport 040-backport 041-backport added

comment:2 Changed 14 months ago by nickm

Severity: NormalMajor

comment:3 Changed 14 months ago by nickm

I don't understand the bug. smartlist_del will remove ip from the smartlist, but ip is not invalidated by smartlist_del.

comment:4 Changed 14 months ago by dgoulet

Description: modified (diff)

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 [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

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).

comment:5 Changed 14 months ago by dgoulet

Resolution: invalid
Status: assignedclosed

I'm invalidating this one. The issue is not on the client side IP failure cache as far as I can tell.

There seems to be issues with intro circuit being TRUNCATED for which the client doesn't consider that a failure and thus repeats quite a bit. I haven't figured out yet why IP circuit get truncated like that after an INTRO1 cell is sent.

comment:6 Changed 14 months ago by teor

Keywords: 035-backport 040-backport 041-backport removed
Note: See TracTickets for help on using tickets.