dos: Block single hop client at the HSDir
This is so we remove load from spammy tor2web clients at the HSDir level.
Opened after discussion in #24902 (moved).
Activity
changed milestone to %Tor: 0.4.2.x-final
added actualpoints::0.4 asn-merge component::core tor/tor milestone::Tor: 0.4.2.x-final network-team-roadmap-july nickm-merge owner::dgoulet parent::24962 points::0.1 priority::medium resolution::fixed reviewer::teor severity::normal sponsor::27-must status::closed tor-dos tor-hs tor2web type::defect labels
This one... I have honestly no idea how to pull this off with our current tor code.Problem is that when theGETrequests 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 withedge_connection_twhere 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.
-
Branch:
ticket24964_042_01PR: https://github.com/torproject/tor/pull/1122Trac:
Actualpoints: N/A to 0.1
Status: accepted to needs_review 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?
Trac:
Status: needs_review to needs_revision
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).
-
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?
-
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.
Trac:
Status: needs_revision to needs_review 
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.
Trac:
Status: needs_review to needs_revisionReplying 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 (moved), 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.
-
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_connonto 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... 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_connonto 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
-
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_01PR: https://github.com/torproject/tor/pull/1122Trac:
Actualpoints: 0.1 to 0.4
Status: needs_revision to needs_review -
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?
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.
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.
mentioned in issue #31958 (moved)
moved to tpo/core/tor#24964 (closed)
mentioned in issue tpo/core/tor#31958 (closed)
