Opened 18 months ago

Last modified 3 days ago

#25568 needs_review defect

hs: Lookup failure cache when introducing to an intro point

Reported by: dgoulet Owned by: neel
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: security, tor-hs, 034-triage-20180328, 034-removed-20180328, 035-backport-maybe, 040-backport-maybe, 041-backport-maybe, 042-backport-maybe
Cc: asn, neel Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

It turns out that if a descriptor contains 10 times the same intro point, and that the first introduction attempt fails, we'll try to connect to the same failing intro point again for all subsequent remaining intro points.

The intro point failure cache was introduced to avoid such a situation but it is only used between two descriptors that is if an intro point failed from the first descriptor and that intro point is still present in the second descriptor fetched, we ignore it.

However, this situation is about the same intro point in the same descriptor. In normal circumstances, this can't happen but it is still allowed by the protocol.

One issue with this is that a malicious service would induce many circuits out of the client than necessary. This can be used, theoretically, for a client guard discovery attack.

This affects both v2 and v3.

Child Tickets

Change History (21)

comment:1 Changed 18 months ago by asn

Cc: asn added

comment:2 Changed 18 months ago by arma

My original suggestion for fixing was that we should consult our failure cache when we're about to launch another try.

But then I realized that maybe we should instead strip duplicate intro points when we process the descriptor.

But then I realized that maybe we should disallow duplicate intro points in the protocol, i.e. say in that spec that you can't do that, and then refuse descriptors that do.

comment:3 in reply to:  description Changed 18 months ago by arma

Replying to dgoulet:

a malicious service would induce many circuits out of the client than necessary. This can be used, theoretically, for a client guard discovery attack.

I think the same level of attack could happen just by listing a bunch of slightly different unreachable intro points. So I don't think we should think of ourselves as fixing a client guard discovery attack at all, when we fix this ticket.

comment:4 Changed 18 months ago by nickm

I think the same level of attack could happen just by listing a bunch of slightly different unreachable intro points.

Right. For example, I think it would waste waste just about as much resources if the descriptor were to just list 10 randomly generated nonexistent introduction points.

comment:5 Changed 18 months ago by nickm

Keywords: 034-triage-20180328 added

comment:6 Changed 18 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:7 Changed 18 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:8 Changed 11 months ago by dgoulet

Status: assignednew

comment:9 Changed 11 months ago by dgoulet

Owner: dgoulet deleted
Status: newassigned

comment:10 Changed 10 months ago by neel

Cc: neel added
Owner: set to neel

comment:12 Changed 3 months ago by neel

I noticed teor left some comments on my GitHub PR. Teor, I'm sorry if you're not done reading it.

I made the fixes to the changes file.

Splitting the functions do not make sense as a lot of the code in the functions you suggest should be split depend on goto routines, so it would be harder to split.

comment:13 in reply to:  12 Changed 3 months ago by teor

Replying to neel:

I noticed teor left some comments on my GitHub PR. Teor, I'm sorry if you're not done reading it.

I made the fixes to the changes file.

Thanks!

Splitting the functions do not make sense as a lot of the code in the functions you suggest should be split depend on goto routines, so it would be harder to split.

That's fine, I'll leave it to the reviewer to decide if they want to help you split those functions.

comment:14 Changed 3 months ago by dgoulet

Reviewer: dgoulet

comment:15 Changed 3 months ago by dgoulet

Status: needs_reviewneeds_revision

@neel: I left one comment there because I think there was a mis-understand with this ticket.

It should be allowed to have more than once the same intro point in the descriptor. What we want is to avoid reconnecting to the same intro points (for the same auth key!) multiple times if that intro point is in the failure cache (for a single descriptor).

comment:16 Changed 3 months ago by neel

Status: needs_revisionneeds_review

I have a new PR on a different branch here: https://github.com/torproject/tor/pull/1161

comment:17 in reply to:  16 Changed 2 months ago by dgoulet

Status: needs_reviewneeds_revision

Replying to neel:

I have a new PR on a different branch here: https://github.com/torproject/tor/pull/1161

I don't think this will work as expected.

First, I believe this is only a v2 problem because in v3, when picking an intro point from the descriptor, we do _not_ pick unusable IPs.

Where with v2, this check is not done when picking the intro point but rather when sending the INTRO cell. Problem lies with rend_client_any_intro_points_usable() I believe because it select a new intro point and only checks at ip->timed_out and not the failure cache.

Once a NACK arrives, the v2 code actually removes the intro point from the parsed descriptor so we can't even check the IP object for an error. We really need to query the failure cache.

comment:18 Changed 7 weeks ago by neel

Thanks for the clarification.

New PR is here: https://github.com/torproject/tor/pull/1192

New PR because this is completely different from the old one, so using the old PR/branch is not an option.

comment:19 Changed 6 weeks ago by neel

Status: needs_revisionneeds_review

comment:20 Changed 4 days ago by teor

Keywords: 035-backport-maybe 040-backport-maybe 041-backport-maybe 042-backport-maybe added
Milestone: Tor: unspecifiedTor: 0.4.3.x-final
Reviewer: dgoulet

dgoulet is on leave, please re-assign this review.

comment:21 Changed 3 days ago by asn

Reviewer: dgoulet

Hello. We discussed during the meeting that we will leave this to dgoulet since it's been waiting for a while anyway and David has already done a few passes over this.

Note: See TracTickets for help on using tickets.