Opened 2 years ago

Closed 4 months ago

#22689 closed defect (duplicate)

hs: Stop intro points being used as single hop proxies

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: relay-safety, 033-triage-20180320, 033-removed-20180320, reviewer-was-teor-20190422
Cc: Actual Points:
Parent ID: #17945 Points: 0.5
Reviewer: Sponsor:

Description (last modified by teor)

This prevents them knowing both the service and client IP addresses, and therefore being targets for network traffic logging, sybil, or hacking attacks.

We need to implement the following checks:

  • if an introduction point was made using a direct connection (single onion services), refuse direct client connections,
  • for v3 intro points, always refuse direct client connections
  • for v2 intro points, refuse direct client connections based on a consensus parameter
  • if the rend point was made using a direct connection (custom client, no tor2web for HSv3), refuse direct service connections (single onion services).

See #22688 for how this is done for HSDir3s using channel_is_client(). The comments in that patch explain why it works.

We could even refactor the common code out of connection_dir_is_anonymous() into connection_is_anonymous(), and avoid including channel[tls].h into directory.c.

I'm not sure if I will get time to do this, so please feel free to take this ticket.

Child Tickets

Change History (29)

comment:1 Changed 2 years ago by teor

We can do this for HSv2 rend points when we implement this patch, because single onion services retry a 3-hop path on failure.

But we can't fix HSv2 intro points, because tor2web doesn't retry a 3-hop path (#19662).

comment:2 Changed 2 years ago by teor

Description: modified (diff)
Status: newneeds_revision

See cddff59c0 in my branch bug22688-031 for code that might work for OR circuits.
(I accidentally wrote the OR version rather than the directory version.)

comment:3 Changed 2 years ago by teor

Owner: set to teor
Status: needs_revisionassigned

comment:4 Changed 2 years ago by teor

Status: assignedneeds_revision

comment:5 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_review

Hrmm I think that should be in needs_review.

comment:6 Changed 2 years ago by teor

Status: needs_reviewneeds_revision

No, it really does need revision. And someone needs to actually *run* it.

comment:7 Changed 2 years ago by teor

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final

I'm not going to get time to do these in 0.3.2.
Moving them to 0.3.3.

comment:8 Changed 21 months ago by teor

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Moving most of my assigned tickets to 0.3.4

comment:9 Changed 20 months ago by teor

Status: needs_revisionneeds_review

Latest code in #17945

comment:10 Changed 20 months ago by dgoulet

Keywords: prop224 removed
Milestone: Tor: 0.3.4.x-finalTor: 0.3.3.x-final
Owner: changed from teor to dgoulet
Status: needs_reviewassigned
Summary: prop224: Stop rend and intro points being used as single hop proxieshs: Stop rend and intro points being used as single hop proxies

Changing ticket title to reflect that it now applies to all HS version. Then moving it to 033 and I'm taking it, I'll submit a branch soon. We are splitting things out of #17945 so we don't get too confused.

comment:11 Changed 20 months ago by dgoulet

Status: assignedneeds_review

Branch: ticket22689_033_01

It is not that small because I did an HS specific interface but it is as simple as the original patch.

comment:12 Changed 20 months ago by teor

Reviewer: teor

I'll review this later today, after I get some Rust done.

Do we want a consensus parameter to block Tor2web at Intros, like the one at Rendezvous?
I think the answer is "yes, but not on by default, and not on right now, and maybe just in 0.3.4".
I opened #25371 to do it in a separate task.

comment:13 in reply to:  12 Changed 20 months ago by dgoulet

Replying to teor:

I'll review this later today, after I get some Rust done.

Do we want a consensus parameter to block Tor2web at Intros, like the one at Rendezvous?
I think the answer is "yes, but not on by default, and not on right now, and maybe just in 0.3.4".
I opened #25371 to do it in a separate task.

Yes I think ultimately (hopefully 034 imo), we come down to rejecting single hop client for any part of the HS dance (single onion or not). Lets open a ticket for that at the RDV?

I've modified the branch based on asn's comment to merge the suitable functions into one. I've renamed the function also to reflect a bit more what it is doing.

See fixup 216c754e6f.

comment:14 Changed 20 months ago by asn

Patch LGTM.

Are we sure this is not gonna make single onion services (and tor2web clients) go crazy with reconnecting and end up hurting the network?

comment:15 in reply to:  14 Changed 20 months ago by dgoulet

Replying to asn:

Are we sure this is not gonna make single onion services (and tor2web clients) go crazy with reconnecting and end up hurting the network?

In theory, because the circuits are closed at the RP with the "TORPROTOCOL" reason, the client should pick a new rendezvous point. However, there is a good chance the tor2web client will just loop over all RPs it can find and try them all in a loop. That would need to be tested.

comment:16 Changed 20 months ago by teor

v2 Intro:

v2 Tor2web will extend to another intro point, and then succeed because it's no longer a single hop path.

v2 Rendezvous:

v2 single onion services retry failed rendezvous with a 3-hop path, so they should retry once.
But we turned off retries to try to reduce the DDoS load.
So I guess they will just fail.
And then v2 Tor2web will try another rendezvous point, and eventually give up.
But we turned off Tor2web at rendezvous points.

So we should use one of these combination of rendezvous retry and Tor2web settings in the consensus:

  • banning Tor2web works, or
  • allowing Tor2web and at least 1 retry also works.

But allowing Tor2web and with no retries will put extra load on the network from Tor2web / single onion service failures.

v3:

We never implemented any special retry behaviour for v3 single onion services, but that doesn't matter, because there is no v3 Tor2web.

comment:17 in reply to:  16 ; Changed 20 months ago by teor

Replying to teor:

v2 Intro:

v2 Tor2web will extend to another intro point, and then succeed because it's no longer a single hop path.

The client will only extend if it thinks that the service isn't connected to the intro point.
So maybe we shouldn't close intro circuits, but we should force them to extend instead?

And then we can have an option for closing them as a DDoS defence.

comment:18 in reply to:  17 Changed 20 months ago by dgoulet

Replying to teor:

Replying to teor:

v2 Intro:

v2 Tor2web will extend to another intro point, and then succeed because it's no longer a single hop path.

The client will only extend if it thinks that the service isn't connected to the intro point.
So maybe we shouldn't close intro circuits, but we should force them to extend instead?

In theory, just a NACK received by the client will make it reuse the circuit and re-extend. See handle_introduce_ack(). This patch makes the intro return HS_CELL_INTRO_ACK_NORELAY which triggers a re-extend. Same goes for v2 in rend_client_introduction_acked().

comment:19 Changed 20 months ago by teor

Ok, let's put these behind DDoS options that are on by default, so we can turn them off if we get it wrong.

comment:20 Changed 20 months ago by asn

Status: needs_reviewneeds_revision

I feel like we are spending too much time on this ticket, while it's not a priority and we are not sure what's the best way to do it. I'm marking it as needs_revision because of the recent discussion about DoS options.

Perhaps we can revisit this in Rome and figure the best approach for handling and supporting 1-hop everything. I'm fine with delaying this to 034 too.

comment:21 Changed 19 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:22 Changed 19 months ago by nickm

Keywords: 033-removed-20180320 added

Mark all not-already-included tickets as pending review for removal from 0.3.3 milestone.

comment:23 Changed 19 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: unspecified

These tickets were marked as removed, and nobody has said that they can fix them. Let's remember to look at 033-removed-20180320 as we re-evaluate our triage process, to see whether we're triaging out unnecessarily, and to evaluate whether we're deferring anything unnecessarily. But for now, we can't do these: we need to fix the 033-must stuff now.

comment:24 Changed 16 months ago by teor

Summary: hs: Stop rend and intro points being used as single hop proxieshs: Stop intro points being used as single hop proxies

#24902 stops rend points being used as single hop proxies, but we still need to fix intro points (and HSDirs).

comment:25 Changed 16 months ago by teor

Description: modified (diff)

comment:26 Changed 16 months ago by teor

Description: modified (diff)

comment:27 Changed 6 months ago by teor

Keywords: reviewer-was-teor-20190422 added
Reviewer: teor

If these tickets go back in to needs_review, and I am on leave, they will need another reviewer.

comment:28 Changed 4 months ago by gaba

Owner: dgoulet deleted
Status: needs_revisionassigned

dgoulet will assign himself to the ones he is working on right now.

comment:29 Changed 4 months ago by dgoulet

Resolution: duplicate
Status: assignedclosed

I believe this will be fixed by implementing #24963 which will prevent single hop client circuit to the introduction point (INTRODUCE1).

Now that we've removed Tor2web from the tor code, we can simplify quite a bit our approach.

Closing in favor of #24963.

Note: See TracTickets for help on using tickets.