[tor-bugs] #23493 [Core Tor/Tor]: IPv6 v3 Single Onion Services fail with a bug warning

Tor Bug Tracker & Wiki blackhole at torproject.org
Fri Sep 22 02:05:47 UTC 2017

#23493: IPv6 v3 Single Onion Services fail with a bug warning
 Reporter:  teor                                 |          Owner:  teor
     Type:  defect                               |         Status:
                                                 |  needs_review
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.2.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  prop224, tor-hs, single-onion, ipv6  |  Actual Points:  0.7
Parent ID:                                       |         Points:  1
 Reviewer:                                       |        Sponsor:
Changes (by teor):

 * status:  needs_revision => needs_review
 * actualpoints:  0.5 => 0.7


 I've updated my bug23493 branch.

 Replying to [comment:13 asn]:
 > Here is a short review:
 > - Not a big fan of poking into the guts of service with
 `service->config.is_single_onion` to figure out if SoS or not. I suggest
 now that we are more serious about this feature to use a function like
 `int service_is_single_onion(hs_service_t *)`.

 I was always serious about this feature :-)

 And I agree, it's important to have a read-only accessor when we are
 passing inout references to a copy of that flag.

 See commit 76d25b0b74.

 That said, the existing code doesn't have accessors for any of the other
 flags in config, so it's up to you if you want this flag to be different.

 > - `direct_conn_inout` is a weird variable name. Why `inout` and not
 `out`? Also let's improve variable naming so that this `BUG` makes a bit
 more sense `if (BUG(direct_conn && direct_conn_inout &&
 !*direct_conn_inout)) {`.

 I rewrote the function comment and made the variable naming clearer in
 fixup b51005940d.

 > - `7c3ba98cd` is a bit sketch. I wonder how come that was not needed

 It's needed to make sure that we don't connect to an address that's banned
 by ReachableAddresses, ClientUseIPv4/6, or similar options, which have
 been around for a long time. In previous releases, we trusted calling code
 to do the right thing.

 > If it was not needed, is it just defense-in-depth?

 Yes, it's defense in depth.

 > Can we add a non-fatal assert to make sure it never triggers?

 We could, but I think a BUG() condition is the right way to go here - it
 allows us to log the error, and respond correctly by refusing to connect
 to the address.

 I also added a comment that explains the change in fixup commit

 > Also, is `extend_info_is_a_configured_bridge()` the right thing to do?
 What happens if the bridge has a PT on a different address than the

 `!extend_info_is_a_configured_bridge()` is the right thing to do, because
 bridges with PTs routinely lie about their addresses, and bridges without
 PTs have always been allowed to connect to their configured addresses. PT
 reachable addresses aren't controlled by tor, and are apparently out of
 scope for PT 2.0, too.

 > - It's kinda scary that there is no unittests for any of the SoS HSv3

 I agree, but I don't have any time to write any this week. Can we open a
 separate ticket for this in 0.3.2?

 Replying to [comment:14 nickm]:
 > Additional minor notes:
 >   * The log message "Launching a %s circuit" will say "launching a
 anonymous circuit" when "an anonymous circuit" would be correct.

 Fixup 898495e6bd.

 >   * Looks like "make check-spaces" will fail.

 Oops. I had that in my workflow, but it dropped out.

 Fixup 15bc97ee7d.

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

More information about the tor-bugs mailing list