Opened 21 months ago

Closed 2 months ago

#24964 closed defect (fixed)

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: nickm-merge, asn-merge, 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 (25)

comment:1 Changed 20 months ago by rl1987

Component: - Select a componentCore Tor/Tor

comment:2 Changed 14 months ago by nickm

Milestone: Tor: unspecified

comment:3 Changed 4 months 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 4 months 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 4 months ago by dgoulet (previous) (diff)

comment:5 Changed 4 months 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 4 months 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 4 months ago by teor (previous) (diff)

comment:7 in reply to:  6 ; Changed 4 months 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 4 months 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 4 months 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 4 months 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 months ago by dgoulet

Reviewer: asn

comment:12 Changed 4 months 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 months ago by asn (previous) (diff)

comment:13 in reply to:  12 Changed 4 months 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 4 months 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 4 months 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 4 months 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 4 months 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 4 months 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 4 months 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 4 months 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 4 months 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 3 months ago by gaba

Keywords: network-team-roadmap-july added

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

comment:24 Changed 2 months ago by teor

Keywords: nickm-merge asn-merge added
Status: needs_reviewmerge_ready

Looks good to me!

comment:25 Changed 2 months ago by asn

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.