Re: [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:  closed
 Priority:  Medium |  Milestone:  Tor: 0.4.2.x-final
Component:  Core Tor/Tor   |Version:
 Severity:  Major  | Resolution:  invalid
 Keywords:  tor-hs tor-client  |  Actual Points:
Parent ID:  #30200 | Points:  1
 Reviewer:  asn|Sponsor:  Sponsor27-must
---+
Changes (by teor):

 * keywords:  tor-hs tor-client 035-backport, 040-backport, 041-backport =>
 tor-hs tor-client


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  closed
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Major| Resolution:  invalid
 Keywords:  tor-hs tor-client 035-backport,  |  Actual Points:
  040-backport, 041-backport |
Parent ID:  #30200   | Points:  1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by dgoulet):

 * status:  assigned => closed
 * resolution:   => invalid


Comment:

 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.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

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

Re: [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
-+-

Comment (by nickm):

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

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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
-+-
Changes (by nickm):

 * severity:  Normal => Major


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  Normal   | Resolution:
 Keywords:  tor-hs tor-client 035-backport,  |  Actual Points:
  040-backport, 041-backport |
Parent ID:  #30200   | Points:  1
 Reviewer:  asn  |Sponsor:
 |  Sponsor27-must
-+-
Changes (by dgoulet):

 * keywords:  tor-hs tor-client => tor-hs tor-client 035-backport,
 040-backport, 041-backport


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs