[tor-bugs] #22688 [Core Tor/Tor]: Make sure HSDir3s never know service, client, or bridge IP addresses

[Tor Bug Tracker & Wiki]

#22688: Make sure HSDir3s never know service, client, or bridge IP addresses
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.1.x-final
Component:  Core Tor/Tor                         |        Version:  Tor:
                                                 |  unspecified
 Severity:  Normal                               |     Resolution:
 Keywords:  prop224, relay-safety,               |  Actual Points:  0.3
  031-backport, maybe-030-backport-with-21406    |
Parent ID:  #17945                               |         Points:  0.3
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by teor):

 Replying to [comment:5 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?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22688#comment:6>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online