Opened 8 months ago

Last modified 2 weeks ago

#22688 needs_revision defect

Make sure HSDir3s never know service, client, or bridge IP addresses

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: prop224, relay-safety, 031-backport, maybe-030-backport-with-21406
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 (13)

comment:1 Changed 8 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 8 months ago by teor

Parent ID: #17945

comment:3 Changed 8 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 8 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 8 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 8 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 8 months ago by teor

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

comment:8 Changed 8 months ago by teor

Owner: set to teor
Status: needs_revisionassigned

comment:9 Changed 8 months ago by teor

Status: assignedneeds_revision

comment:10 Changed 6 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 5 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 3 weeks 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 2 weeks ago by nickm

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

Note: See TracTickets for help on using tickets.