Opened 16 months ago

Last modified 2 weeks 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: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: security, tor-hs, 034-triage-20180328, 034-removed-20180328
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 (16)

comment:1 Changed 16 months ago by asn

Cc: asn added

comment:2 Changed 16 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 16 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 16 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 16 months ago by nickm

Keywords: 034-triage-20180328 added

comment:6 Changed 16 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 15 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 9 months ago by dgoulet

Status: assignednew

comment:9 Changed 9 months ago by dgoulet

Owner: dgoulet deleted
Status: newassigned

comment:10 Changed 8 months ago by neel

Cc: neel added
Owner: set to neel

comment:12 Changed 3 weeks 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 weeks 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 weeks ago by dgoulet

Reviewer: dgoulet

comment:15 Changed 3 weeks 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 2 weeks ago by neel

Status: needs_revisionneeds_review

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

Note: See TracTickets for help on using tickets.