Opened 11 months ago

Last modified 6 weeks ago

#22688 needs_revision defect

hs: Make sure HSDir never know service, client, or bridge IP addresses

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, relay-safety, 034-triage-20180328, 034-removed-20180328
Cc: Actual Points: 0.3
Parent ID: #17945 Points: 0.3
Reviewer: Sponsor:

Description

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 extra safety, we can check if the circuit is from a relay.

Child Tickets

Change History (19)

comment:1 Changed 11 months ago by teor

Actual Points: 0.20.3
Keywords: maybe-030-backport-with-21406 added; no-030-backport removed
Points: 0.20.3
Status: newneeds_review
Summary: HSDir3s should refuse direct client descriptor uploads and downloads, even if encryptedMake sure HSDir3s never know service, client, or bridge IP addresses

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, 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. 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.)

comment:2 Changed 11 months ago by teor

Parent ID: #17945

comment:3 Changed 11 months ago by 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.)

comment:4 in reply to:  3 Changed 11 months ago by teor

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.)

comment:5 Changed 11 months ago by dgoulet

Status: needs_reviewneeds_revision

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-hop circuit 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.

comment:6 in reply to:  5 Changed 11 months ago by teor

Replying 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-hop circuit 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?

comment:7 in reply to:  5 Changed 11 months ago by teor

See #21406 for the recent changes to channel_is_client().

comment:8 Changed 11 months ago by teor

Owner: set to teor
Status: needs_revisionassigned

comment:9 Changed 11 months ago by teor

Status: assignedneeds_revision

comment:10 Changed 9 months ago by dgoulet

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

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.

comment:11 Changed 8 months 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:12 Changed 4 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:13 Changed 3 months ago by nickm

Remove 030-backport from all open tickets that have it: 0.3.0 is now deprecated.

comment:14 Changed 3 months ago by teor

Status: needs_revisionneeds_review
Version: Tor: unspecified

Latest code in #17945.

comment:15 Changed 3 months ago by dgoulet

Keywords: tor-hs added; prop224 031-backport maybe-030-backport-with-21406 removed
Status: needs_reviewassigned
Summary: Make sure HSDir3s never know service, client, or bridge IP addresseshs: Make sure HSDir never know service, client, or bridge IP addresses

Changing ticket title to be for all HS version, not only v3. Assigning to teor because the code exists in his #17945 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.

comment:16 Changed 3 months ago by teor

Status: assignedneeds_revision

Do we want a consensus parameter to block Tor2web at HSDirs?
I think the answer is "yes, but not on by default, and not on right now".
I'll add that parameter when I revise the ticket.

comment:17 Changed 8 weeks ago by nickm

Keywords: 034-triage-20180328 added

comment:18 Changed 8 weeks ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:19 Changed 6 weeks ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These needs_revision, tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if somebody does the necessary revision.

Note: See TracTickets for help on using tickets.