hs: Lookup failure cache when introducing to an intro point

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.

changed milestone to %Tor: 0.4.3.x-final

added 034-removed-20180328 034-triage-20180328 BugSmashFund actualpoints::0.1 component::core tor/tor milestone::Tor: 0.4.3.x-final owner::neel parent::29995 points::0.1 priority::medium resolution::fixed reviewer::asn security severity::normal sponsor::27-must status::closed tor-hs type::defect labels

Trac:
Cc: N/A to asn

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.

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.

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.

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

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

Trac:
Keywords: N/A deleted, 034-removed-20180328 added

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

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: unspecified

Trac:
Status: assigned to new

Trac:
Owner: dgoulet to N/A
Status: new to assigned

Trac:
Owner: N/A to neel
Cc: asn to asn, neel

Tor PR: https://github.com/torproject/tor/pull/1143 Torspec PR: https://github.com/torproject/torspec/pull/85

Trac:
Status: assigned to needs_review

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.

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.

Trac:
Reviewer: N/A to dgoulet

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

Trac:
Status: needs_review to needs_revision

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

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to needs_revision

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.