[tor-bugs] #23820 [Core Tor/Tor]: Make sure v3 single onion services and v3 onion service clients only send IPv4 addresses

Tor Bug Tracker & Wiki blackhole at torproject.org
Wed Nov 1 05:21:08 UTC 2017


#23820: Make sure v3 single onion services and v3 onion service clients only send
IPv4 addresses
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  teor
     Type:  defect                               |         Status:
                                                 |  needs_review
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.2.x-final
Component:  Core Tor/Tor                         |        Version:  Tor:
                                                 |  0.3.2.1-alpha
 Severity:  Normal                               |     Resolution:
 Keywords:  prop224, tor-hs, single-onion, ipv6  |  Actual Points:  1
Parent ID:  #23493                               |         Points:  1
 Reviewer:  dgoulet                              |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by teor):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:5 dgoulet]:
 > @teor, I can fix those if you have no time but just let me know what you
 think about them. If you do fix them, I propose we go in Gitlab mode next
 time.

 I have just resurrected my gitlab and pushed the updated branch to
 bug23820_032.
 (oniongit.eu will have to wait for #24106.)

 > * To be clear, I know Tor can't extend to an IPv6 so this should NEVER
 happened in theory right? (in get_lspecs_from_extend_info())
 >
 > {{{
 > +  if (BUG(!tor_addr_is_v4(&ei->addr))) {
 > +    return;
 > +  }
 > }}}

 See the function comment:
 {{{
    Clients never make
  * direct connections to rendezvous points, so they should always have an
  * IPv4 address in ei.
 }}}

 But this comment only describes the *current* implementation. I can't rely
 on the behaviour of future implementations, because we know we want to add
 IPv6 all over the place.

 Also, clients and services can already make direct IPv6 connections using
 the addr in ei, and mis-handling that caused #23493, so we need a check
 for IPv6.

 > * The `intro1_data` is initialized with `setup_introduce1_data()` which
 guarantee that the link specifier list will be a valid smartlist pointer
 and never uninit. So here, I would remove the NULL check so we don't hide
 bugs.
 >
 > {{{
 > +  if (!intro1_data.link_specifiers ||
 > +      !smartlist_len(intro1_data.link_specifiers)) {
 > }}}

 I don't think we should trust future implementations of
 `setup_introduce1_data()` to hand us a non-NULL pointer.

 We're logging a warning if either the NULL check or the length check
 fails. I wrapped the NULL check in a BUG(), so we can distinguish these
 two cases.

 [bug23820_032 f31094c1d5] fixup! Remove buggy IPv6 and ed25519 handling
 from get_lspecs_from_extend_info()

 > * We could have a function that returns a static string for this so we
 make sure that every logs will have the same keywords. Something like
 `service_type_str()`
 >
 > {{{
 > service->config.is_single_onion ? "direct" : "multi-hop"
 > }}}

 These are the wrong strings to log anyway: when we implement falling back
 to a 3-hop path in #23818, we will want to log both the anonymity of the
 service, and the number of paths in this circuit. Right now, we can only
 log single onion vs hidden.

 [bug23820_032 6157b05aad] fixup! Improve v3 onion service logging for
 intro and rend points

 > * I'm skeptical that this will help our logging. I think base32 would be
 closer to the onion address than the hex string:
 >
 > {{{
 > +               safe_str_client(hex_str((const char
 *)service_pk->pubkey,
 > +                                       ED25519_PUBKEY_LEN)));
 > }}}

 Someone wrote a really nice hs_build_address() function, so I'll just use
 that ;-)

 [bug23820_032 6157b05aad] fixup! Improve v3 onion service logging for
 intro and rend points

 > * Hmm this is possible that is SingleHopMode 0 and NonAnonymous 1 ? ...
 Looking at `rend_service_non_anonymous_mode_enabled()` seems to me that
 they have to be consistent?
 >
 > {{{
 > +      log_warn(LD_CONFIG, "IPv6-only v3 single onion services are not "
 > +               "supported. Set HiddenServiceSingleHopMode 0 and "
 > +               "HiddenServiceNonAnonymousMode 1, or set ClientUseIPv4
 1.");
 > }}}

 Yes, that's a typo.

 It does make me wonder whether having 2 options for the same thing is
 actually less safe, because it makes it more likely we will introduce
 bugs.

 [bug23820_032 ea1d68c857] fixup! Stop users configuring IPv6-only v3
 single onion services

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


More information about the tor-bugs mailing list