Opened 21 months ago

Closed 7 weeks ago

Last modified 6 weeks ago

#25568 closed defect (fixed)

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, BugSmashFund
Cc: asn, neel Actual Points: 0.1
Parent ID: #29995 Points: 0.1
Reviewer: asn Sponsor: Sponsor27-must

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 (32)

comment:1 Changed 21 months ago by asn

Cc: asn added

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

Keywords: 034-triage-20180328 added

comment:6 Changed 21 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 21 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 14 months ago by dgoulet

Status: assignednew

comment:9 Changed 14 months ago by dgoulet

Owner: dgoulet deleted
Status: newassigned

comment:10 Changed 13 months ago by neel

Cc: neel added
Owner: set to neel

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

Reviewer: dgoulet

comment:15 Changed 6 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 6 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 5 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 5 months 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 4 months ago by neel

Status: needs_revisionneeds_review

comment:20 Changed 3 months 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 months 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.

comment:22 Changed 7 weeks ago by dgoulet

Status: needs_reviewneeds_revision

Left comments on the PR.

comment:23 Changed 7 weeks ago by neel

Status: needs_revisionneeds_review

Thanks for the review.

Made the changes requested, and setting as needs review.

comment:24 Changed 7 weeks ago by dgoulet

Actual Points: 0.1
Keywords: BugSmashFund added
Points: 0.1
Reviewer: dgouletasn

Thanks neel! I realized that we actually need to not return any intro point if it is not usable right in rend_client_get_random_intro_impl() which is where it checks as well for the IP timing out.

I've taken the branch over based on your work so to get it a bit fast tracked into upstream :).

Branch: ticket25568_043_01
PR: https://github.com/torproject/tor/pull/1484

I think NO backport here because HSv2 and minor reachability issue. If we don't want that, I can provide an 035 branch then.

comment:25 Changed 7 weeks ago by dgoulet

Parent ID: #29995
Sponsor: Sponsor27-must

(This falls under s27).

comment:26 Changed 7 weeks ago by dgoulet

Parent ID: #29995

comment:27 Changed 7 weeks ago by asn

Resolution: fixed
Status: needs_reviewclosed

LGTM!

I agree with you that v3 should be covered because of intro_point_is_usable() use in client_get_random_intro().

Merging!

comment:28 in reply to:  15 ; Changed 6 weeks ago by arma

Replying to dgoulet:

It should be allowed to have more than once the same intro point in the descriptor.

Why should this be allowed? Is there any benefit to it?

Handling it correctly sounds good, but disallowing it in the spec sounds good too right?

comment:29 in reply to:  28 Changed 6 weeks ago by dgoulet

Replying to arma:

Replying to dgoulet:

It should be allowed to have more than once the same intro point in the descriptor.

Why should this be allowed? Is there any benefit to it?

Handling it correctly sounds good, but disallowing it in the spec sounds good too right?

Yeah I said that solely based on the fact that the spec doesn't specifically mention that it is not allowed.

We could easily clarify the spec there and then move the code to consider any descriptor with the same intro point a "bad descriptor". Not much incentive on my part though to make it happen "soon" :P.

comment:30 Changed 6 weeks ago by dgoulet

Parent ID: #29999

comment:31 Changed 6 weeks ago by dgoulet

Parent ID: #29999#29995

comment:32 Changed 6 weeks ago by teor

Keywords: 035-backport-maybe 040-backport-maybe 041-backport-maybe 042-backport-maybe removed

No backport

Note: See TracTickets for help on using tickets.