Opened 18 months ago

Last modified 21 hours ago

#24964 needs_review defect

dos: Block single hop client at the HSDir

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dos, tor2web, tor-hs, network-team-roadmap-july
Cc: Actual Points: 0.4
Parent ID: #24962 Points: 0.1
Reviewer: teor Sponsor: Sponsor27-must

Description

This is so we remove load from spammy tor2web clients at the HSDir level.

Opened after discussion in #24902.

Child Tickets

Change History (23)

comment:1 Changed 17 months ago by rl1987

Component: - Select a componentCore Tor/Tor

comment:2 Changed 11 months ago by nickm

Milestone: Tor: unspecified

comment:3 Changed 5 weeks ago by dgoulet

Milestone: Tor: unspecifiedTor: 0.4.2.x-final
Owner: set to dgoulet
Points: 0.1
Sponsor: Sponsor27-must
Status: newaccepted
Summary: dos: Add an option to block tor2web requests at the HSDirdos: Block single hop client at the HSDir

comment:4 Changed 5 weeks ago by dgoulet

This one... I have honestly no idea how to pull this off with our current tor code.

Problem is that when the GET requests comes in for the descriptor, we only learn what it is in the directory connection layer which doesn't have any clue about the circuit.

I've tried to take the approach we use with edge_connection_t where we put the circuit pointer in it (on_circuit) but since the directory request code is called from the connection read callback, there is no access to the circuit at that level either...

Solution is to test the directory connection channel to know if it is a client or not.

Last edited 5 weeks ago by dgoulet (previous) (diff)

comment:5 Changed 5 weeks ago by dgoulet

Actual Points: 0.1
Status: acceptedneeds_review

Branch: ticket24964_042_01
PR: https://github.com/torproject/tor/pull/1122

comment:6 Changed 5 weeks ago by teor

Status: needs_reviewneeds_revision

This code will allow HSDir connections from relays that are not in the consensus.
So an attacker could configure their client/onion service as an unpublished relay to pass this check.
(I'm not sure if tor2web mode supports a relay on the same instance, but I think it probably does.)
Do we want to allow this workaround?

We could check that the previous hop is a relay in the consensus.
If we do that check. then a small number of HSDir requests will fail, and the client will try another HSDir with another circuit.
Do we want to pay this cost?

What do you think the best tradeoff is?

Last edited 5 weeks ago by teor (previous) (diff)

comment:7 in reply to:  6 ; Changed 5 weeks ago by arma

Replying to teor:

We could check that the previous hop is a relay in the consensus.
If we do that check. then a small number of HSDir requests will fail, and the client will try another HSDir with another circuit.

Careful there! That might be true for client requests (doing a GET), but it will be less true for service requests (doing a POST).

comment:8 Changed 5 weeks ago by arma

Maybe I missed it, but, is there something specific we're aiming to fix with this patch? Or is this just completeness from the earlier "stop allowing single-hop anything" changes?

comment:9 in reply to:  7 Changed 5 weeks ago by dgoulet

Replying to arma:

Replying to teor:

We could check that the previous hop is a relay in the consensus.
If we do that check. then a small number of HSDir requests will fail, and the client will try another HSDir with another circuit.

Careful there! That might be true for client requests (doing a GET), but it will be less true for service requests (doing a POST).

This checks if the previous channel is client or not. That is unauthenticated. If the link is unauthenticated, then it is denied.

The case of a service posting a descriptor will always work as long as the service does it through relays in consensus or not. If I'm not mistaken (?), all public relays will authenticate.

The case of the client trying to go around that check with a relay not in the consensus I believe will still authenticate on the link? Unless it is a bridge?

What am I missing here?

comment:10 Changed 5 weeks ago by dgoulet

Status: needs_revisionneeds_review

After discussion with teor on IRC, it appears the patch are good.

Reason is that a single onion service will always 3-hop to the HSDir. Thus anything not authenticating on the directory connection channel means it is not a public relay.

The goal of this is also to not allow C -> Bridge -> HSDir.

Maybe I missed it, but, is there something specific we're aiming to fix with this patch? Or is this just completeness from the earlier "stop allowing single-hop anything" changes?

To answer your question Roger, completeness yes. Point is to close down any access to HS component in a single hop fashion to both remove load on the network but also stop very early any single hop clients instead of stopping them at the rendezvous point only.

comment:11 Changed 4 weeks ago by dgoulet

Reviewer: asn

comment:12 Changed 4 weeks ago by asn

Status: needs_reviewneeds_revision

Code looks good to me, and this is not something that I consider unittestable, but I'd really appreciate good real-life testing. Have we tested this in the real network or chutney? It would be great to verify that descriptor uploads/downloads work just fine with client/HS/single-HS.

Also travis seems broken.

Last edited 4 weeks ago by asn (previous) (diff)

comment:13 in reply to:  12 Changed 4 weeks ago by teor

Replying to asn:

Code looks good to me, and this is not something that I consider unittestable, but I'd really appreciate good real-life testing. Have we tested this in the real network or chutney? It would be great to verify that descriptor uploads/downloads work just fine with client/HS/single-HS.

We can use "make test-network-all", or merge this branch with #29280, and Travis will run chutney to check client, HS, and single onion services.
We should also check the tor logs in chutney, because sometimes bugs show up there was warnings.

Also travis seems broken.

And Appveyor didn't even run.
I closed and re-opened the pull request.

comment:14 Changed 3 weeks ago by dgoulet

Ok turns out that a unit test needed more love to pass the "connection is anonymous" test added by this branch.

HOWEVER, it highlighted a problem with the approach.

The linked_conn onto a directory connection is always of type EXIT. I'm going back to the point where I do not know how to get the or_connection_t that the dir_connection_t is coming from...

comment:15 in reply to:  14 Changed 3 weeks ago by teor

Replying to dgoulet:

Ok turns out that a unit test needed more love to pass the "connection is anonymous" test added by this branch.

HOWEVER, it highlighted a problem with the approach.

The linked_conn onto a directory connection is always of type EXIT. I'm going back to the point where I do not know how to get the or_connection_t that the dir_connection_t is coming from...

BEGINDIR connections are dir connections, linked to an exit stream, which is on an OR circuit, which is on an OR connection.

So you have to walk a few more links:

  • dir connection to edge connection via linked_conn
  • edge connection to OR circuit via on_circuit
  • OR circuit to OR connection via p_chan

Here's some code I wrote a little while ago, that walks the links with all the appropriate checks:
https://github.com/teor2345/tor-old/commit/10290066c8ee6b4aa40ec048222fdd4f572ef8d9#diff-c56fd972333216da3bb1852bcc89f57dR1587

comment:16 Changed 3 weeks ago by dgoulet

Actual Points: 0.10.4
Status: needs_revisionneeds_review

Thanks teor!

I've almost redid the entire branch so I just rebased-squashed to latest master and force push the PR. It needs the review to start from the beginning. Unit tests and chutney tests pass.

Branch: ticket24964_042_01
PR: ​https://github.com/torproject/tor/pull/1122

comment:17 Changed 3 weeks ago by teor

Do we need to check for "marked for close" on the circuit or channel as well?

comment:18 in reply to:  17 ; Changed 3 weeks ago by dgoulet

Replying to teor:

Do we need to check for "marked for close" on the circuit or channel as well?

I'm not entirely sure actually... If the circuit or channel is closed, the descriptor will never be sent back. But if the edge connection is closed, then we sorta need to also deny the request even though it is not really suppose to happen...

Thoughts?

comment:19 Changed 3 weeks ago by asn

I added some comment to the PR but I feel like I don't know enough about the conn/circuit subsystem to be able to review this effectively, given its reachability conseuqences in case of bug. Perhaps I can pass this to someone else (like Nick or Tim) next week?

comment:20 in reply to:  18 Changed 3 weeks ago by teor

Replying to dgoulet:

Replying to teor:

Do we need to check for "marked for close" on the circuit or channel as well?

I'm not entirely sure actually... If the circuit or channel is closed, the descriptor will never be sent back. But if the edge connection is closed, then we sorta need to also deny the request even though it is not really suppose to happen...

Thoughts?

Let's do the closed and mark for closed checks, but BUG() if they ever happen?
Then we can switch the ones that do happen to info logs.

comment:21 in reply to:  19 Changed 3 weeks ago by teor

Replying to asn:

I added some comment to the PR but I feel like I don't know enough about the conn/circuit subsystem to be able to review this effectively, given its reachability conseuqences in case of bug. Perhaps I can pass this to someone else (like Nick or Tim) next week?

I can do the review. I wrote the code that this function is based on.

comment:22 Changed 6 days ago by gaba

Keywords: network-team-roadmap-july added

comment:23 Changed 21 hours ago by asn

Reviewer: asnteor

I'm officially passing this over to teor as discussed in net team meeting. Tim please feel free to pass me any of your review tickets. Cheers.

Note: See TracTickets for help on using tickets.