#23820 closed defect (fixed)

Make sure v3 single onion services and v3 onion service clients only send IPv4 addresses

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version: Tor: 0.3.2.1-alpha
Severity: Normal Keywords: prop224, tor-hs, single-onion, ipv6
Cc: neel@… Actual Points: 1
Parent ID: #23493 Points: 1
Reviewer: dgoulet Sponsor:

Description

This fixes the bug warning in #23493, and allows our users to migrate to IPv4 v3 single onion services.

Child Tickets

TicketStatusOwnerSummaryComponent
#23589closedteorStop assuming that every extend_info contains an IPv4 address in get_lspecs_from_extend_info()Core Tor/Tor

Change History (9)

comment:1 Changed 10 months ago by teor

Owner: set to teor
Status: newassigned

comment:2 Changed 10 months ago by teor

Actual Points: 1
Status: assignedneeds_review
Version: Tor: 0.3.2.1-alpha

Please see my branch bug23820_032. It removes buggy IPv6 support from v3 single onion services, and adds an option check and some logging.

Somewhat alarmingly, I didn't break any unit tests with these changes.
But if we want unit tests here, someone else will need to write them, because I've run out of time to do this.

comment:3 Changed 10 months ago by neel

Cc: neel@… added

comment:4 Changed 10 months ago by dgoulet

Reviewer: dgoulet

comment:5 Changed 10 months ago by dgoulet

Status: needs_reviewneeds_revision

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

  • 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;
+  }
  • 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)) {
  • 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"
  • 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)));
  • 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.");

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

Status: needs_revisionneeds_review

Replying to 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

comment:7 Changed 10 months ago by dgoulet

Status: needs_reviewmerge_ready

All lgtm! I've added a small fixup commit to memwipe() onion_address in the hs client code that was added with a fixup for logging purposes.

Oniongit: https://oniongit.eu/dgoulet/tor/merge_requests/9
See branch: ticket23820_032_01

comment:8 Changed 10 months ago by nickm

Squashed and merged.

Added commit 6a9a118f9077c02ce60052275dcede88d9fd2aa3 to fix a minor but significant difference in a comment.

Please close or un-parent child ticket as appropriate, then close this?

comment:9 Changed 10 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.