Activity
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.
Trac:
Actualpoints: N/A to 1
Version: N/A to Tor: 0.3.2.1-alpha
Status: assigned to needs_reviewTrac:
Cc: N/A to neel@neelc.org
@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_datais initialized withsetup_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.");Trac:
Status: needs_review to needs_revisionReplying 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 (moved).)
- 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 **Trac**: **Status**: needs_revision **to** needs_reviewAll lgtm! I've added a small fixup commit to memwipe()
onion_addressin 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_01Trac:
Status: needs_review to merge_ready
