hs: stop HSDirs being used as single-hop "proxies"
handle_post_hs_descriptor and handle_get_hs_descriptor_v3 should check that the connection is:
- encrypted, and
- not from a client (channel_is_client in 0.3.1.1-alpha and later correctly identifies unauthenticated peers, which are clients and bridges).
For HSv2, we should allow direct Tor2web client connections by default, but have a consensus parameter to turn them off. Direct service connections should always be refused,
Activity
changed milestone to %Tor: unspecified
Please see my branch bug22688-031 on github.
If we want to backport it to 0.3.0, we also need to backport the channel_is_client fix in #21406 (moved), which was merged in 0.3.1.1-alpha.
This compiles, but can't actually test this, so dgoulet or asn will need to check it against their working HSv3 service and client code.
This breaks the direct descriptor downloads tor2web used to do in HSv2, see #20104 (moved). But we don't plan on tor2web in HSv3, so that's ok. (And if we do, this is something we should fix.)
(This patch doesn't check if the circuit is from a relay, that check would be redundant.)
Trac:
Points: 0.2 to 0.3
Summary: HSDir3s should refuse direct client descriptor uploads and downloads, even if encrypted to Make sure HSDir3s never know service, client, or bridge IP addresses
Actualpoints: 0.2 to 0.3
Status: new to needs_review
Keywords: no-030-backport deleted, maybe-030-backport-with-21406 addedTrac:
Parent: N/A to #17945 (moved)Replying to teor:
Do we want to do this for HSv2 service descriptor uploads as well?
(We can't do it for HSv2 client descriptor downloads, because tor2web does them directly.)
Yolo, let's do the service descriptor post fix for HSv2 as well. See the latest commits in bug22688-031: there was a bug in the original code which I fixuped.
I tested this for v2 hidden services and single onion services (make test-network-all) and they all worked. Someone should probably test it with a HS with a bridge. (We don't have that set up in chutney yet.)
Some comments:
- We should break this assert() in two different ones else if triggered, we won't know which condition triggered it:
+ /* A clever compiler might complain this is always true */ + tor_assert(TO_CONN(conn) && TO_CONN(conn)->linked);- How do we know that this is a
one-hopcircuit with this condition?
+ /* Well, we won't be sending anything back on that, will we? + * (Avoid giving the wrong answer because state has been reset.) */ + if (TO_CONN(conn)->linked_conn_is_closed || + !l_conn || l_conn->marked_for_close) { + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Refusing %s one-hop encrypted directory connection.", + TO_CONN(conn)->linked_conn_is_closed ? "closed linked" : + !l_conn ? "NULL linked" : "marked for closed linked"); + return 0; + }Same goes with these condition later:
+ if (BUG(!exitconn) || !exitconn->on_circuit) { [...] + if (BUG(!orcirc) || !orcirc->p_chan) {- Would using
CIRCUIT_IS_ORCIRC()me more appropriate here?
+ /* We should always be using an OR circuit */ + if (BUG(exitconn->on_circuit->purpose != CIRCUIT_PURPOSE_OR)) { + return 0; + }- I'm unclear on where this is checked? Maybe it's done through some indirect checks that I haven't spotted but is there a way you can know that with an
or_circuit_t?
+ * For client circuits via relays, this is true for 2-hop or greater paths, + * for client circuits via bridges, this is true for 3-hop or greater paths.Trac:
Status: needs_review to needs_revisionReplying to dgoulet:
Some comments:
- We should break this assert() in two different ones else if triggered, we won't know which condition triggered it:
{{{
- /* A clever compiler might complain this is always true */
- tor_assert(TO_CONN(conn) && TO_CONN(conn)->linked); }}}
Agreed.
- How do we know that this is a
one-hopcircuit with this condition?
{{{
- /* Well, we won't be sending anything back on that, will we?
-
- (Avoid giving the wrong answer because state has been reset.) */
- if (TO_CONN(conn)->linked_conn_is_closed ||
-
!l_conn || l_conn->marked_for_close) { - log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
-
"Refusing %s one-hop encrypted directory connection.", -
TO_CONN(conn)->linked_conn_is_closed ? "closed linked" : -
!l_conn ? "NULL linked" : "marked for closed linked"); - return 0;
- } }}}
Same goes with these condition later:
{{{
- if (BUG(!exitconn) || !exitconn->on_circuit) { [...]
- if (BUG(!orcirc) || !orcirc->p_chan) { }}}
We don't.
- Would using
CIRCUIT_IS_ORCIRC()me more appropriate here?
{{{
- /* We should always be using an OR circuit */
- if (BUG(exitconn->on_circuit->purpose != CIRCUIT_PURPOSE_OR)) {
- return 0;
- } }}}
Agreed: I couldn't find it when I was writing the patch.
- I'm unclear on where this is checked? Maybe it's done through some indirect checks that I haven't spotted but is there a way you can know that with an
or_circuit_t?
{{{
-
- For client circuits via relays, this is true for 2-hop or greater paths,
-
- for client circuits via bridges, this is true for 3-hop or greater paths. }}}
This is checked at the end of the function using
!channel_is_client(p_chan).channel_is_client()is true if the channel is connected to a non-authenticated peer (something without a fingerprint). This can either be a client or a bridge.So to pass the
!channel_is_client(p_chan)check:- a client can't connect directly, so a client has to have a 2-hop path through a relay,
- a bridge can't connect directly, so a client has to have a 3-hop path through a bridge.
Does that make sense?
See #21406 (moved) for the recent changes to channel_is_client().
-
031 release approaches rapidly, moving this to 032. I think it's not critical to have in 031 but nice to have!
@teor, re-assign if you think otherwise.
Trac:
Milestone: Tor: 0.3.1.x-final to Tor: 0.3.2.x-final Latest code in #17945 (moved).
Trac:
Status: needs_revision to needs_review
Version: Tor: unspecified to N/A-
Changing ticket title to be for all HS version, not only v3. Assigning to teor because the code exists in his #17945 (moved) branch.
Also, removing backport for now, even single service should use a 3-hop circuit so I don't think this is mission critical. Feel free to add backport keyword if you think it is.
Trac:
Summary: Make sure HSDir3s never know service, client, or bridge IP addresses to hs: Make sure HSDir never know service, client, or bridge IP addresses
Keywords: maybe-030-backport-with-21406, prop224, 031-backport deleted, tor-hs added
Status: needs_review to assigned
